Re: [Xen-devel] [PATCH v5 6/9] xen/x86: parse Dom0 kernel for PVHv2

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Thu, Jan 26, 2017 at 06:37:00AM -0700, Jan Beulich wrote:
> >>> On 19.01.17 at 18:29, <roger.pau@xxxxxxxxxx> wrote:
> > @@ -1959,12 +1960,146 @@ static int __init pvh_setup_p2m(struct domain *d)
> >  #undef MB1_PAGES
> >  }
> >  
> > +static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> > +                                  unsigned long image_headroom,
> > +                                  module_t *initrd, char *image_base,
> > +                                  char *cmdline, paddr_t *entry,
> > +                                  paddr_t *start_info_addr)
> > +{
> > +    char *image_start = image_base + image_headroom;
> 
> While for cmdline plain char is certainly fine, I think we should stop
> abusing this type for image and image_base, even if this means
> some further adjustments elsewhere. This really ought to be u8 or
> unsigned char.

Done.

> > +    unsigned long image_len = image->mod_end;
> > +    struct elf_binary elf;
> > +    struct elf_dom_parms parms;
> > +    paddr_t last_addr;
> > +    struct hvm_start_info start_info;
> > +    struct hvm_modlist_entry mod = { 0 };
> > +    struct vcpu *saved_current, *v = d->vcpu[0];
> > +    int rc;
> > +
> > +    if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
> > +    {
> > +        printk("Error trying to detect bz compressed kernel\n");
> > +        return rc;
> > +    }
> > +
> > +    if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
> > +    {
> > +        printk("Unable to init ELF\n");
> > +        return rc;
> > +    }
> > +#ifdef VERBOSE
> > +    elf_set_verbose(&elf);
> > +#endif
> > +    elf_parse_binary(&elf);
> > +    if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
> > +    {
> > +        printk("Unable to parse kernel for ELFNOTES\n");
> > +        return rc;
> > +    }
> > +
> > +    if ( parms.phys_entry == UNSET_ADDR32 ) {
> 
> Please move the brace to its own line.

Done.

> > +        printk("Unable to find XEN_ELFNOTE_PHYS32_ENTRY address\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    printk("OS: %s version: %s loader: %s bitness: %s\n", parms.guest_os,
> > +           parms.guest_ver, parms.loader,
> > +           elf_64bit(&elf) ? "64-bit" : "32-bit");
> > +
> > +    /* Copy the OS image and free temporary buffer. */
> > +    elf.dest_base = (void *)(parms.virt_kstart - parms.virt_base);
> > +    elf.dest_size = parms.virt_kend - parms.virt_kstart;
> > +
> > +    /*
> > +     * NB: we need to set vCPU 0 of Dom0 as the current vCPU (instead of 
> > the
> > +     * idle one) because elf_load_binary calls raw_{copy_to/clear}_guest, 
> > and
> > +     * the target domain is not passed anywhere. This is very similar to 
> > what
> > +     * is done during classic PV Dom0 creation, where Xen switches to the 
> > Dom0
> > +     * page tables. We also cannot pass a struct domain or vcpu to
> > +     * elf_load_binary, since the interface is shared with the toolstack, 
> > and
> > +     * it doesn't have any notion of a domain or vcpu struct.
> > +     */
> > +    saved_current = current;
> > +    set_current(v);
> > +    rc = elf_load_binary(&elf);
> > +    set_current(saved_current);
> 
> While with the comment it's not as bad anymore, I'm still not happy
> with this code. The tool stack portion of the explanation in particular
> does not really hold: We would add an opaque void * parameter to
> the functions involved, which the tool stack could pass NULL for, and
> you'd use it to convey the vcpu.

As spoken on IRC, I will add a new field to elf_binary only visible to the
hypervisor and that will be used to pass the destination vcpu down to
elf_load_image.

> > +    if ( rc < 0 )
> > +    {
> > +        printk("Failed to load kernel: %d\n", rc);
> > +        printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
> > +        return rc;
> > +    }
> > +
> > +    last_addr = ROUNDUP(parms.virt_kend - parms.virt_base, PAGE_SIZE);
> > +
> > +    if ( initrd != NULL )
> > +    {
> > +        rc = hvm_copy_to_guest_phys_vcpu(last_addr,
> > +                                         mfn_to_virt(initrd->mod_start),
> > +                                         initrd->mod_end, v);
> > +        if ( rc )
> > +        {
> > +            printk("Unable to copy initrd to guest\n");
> > +            return rc;
> > +        }
> > +
> > +        mod.paddr = last_addr;
> > +        mod.size = initrd->mod_end;
> > +        last_addr += ROUNDUP(initrd->mod_end, PAGE_SIZE);
> > +    }
> > +
> > +    /* Free temporary buffers. */
> > +    discard_initial_images();
> > +
> > +    memset(&start_info, 0, sizeof(start_info));
> 
> You clear "mod" with an initializer (which btw could be just the two
> braces), but you memset() start_info - please be consistent.

Done, I've added an initializer to it also.

> > +    if ( cmdline != NULL )
> > +    {
> > +        rc = hvm_copy_to_guest_phys_vcpu(last_addr, cmdline,
> > +                                         strlen(cmdline) + 1, v);
> > +        if ( rc )
> > +        {
> > +            printk("Unable to copy guest command line\n");
> > +            return rc;
> > +        }
> > +        start_info.cmdline_paddr = last_addr;
> > +        last_addr += ROUNDUP(strlen(cmdline) + 1, 8);
> 
> Where is this 8 coming from?

So that the next struct is aligned to the cache line size? It's also done in
xc_dom_x86.c.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel