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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +    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.

> +        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.

> +    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.

> +    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?

Jan

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