Re: [Xen-devel] [PATCH v3.1 13/15] xen/x86: parse Dom0 kernel for PVHv2

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

On Fri, Nov 11, 2016 at 03:30:17PM -0500, Konrad Rzeszutek Wilk wrote:
> On Sat, Oct 29, 2016 at 10:59:59AM +0200, Roger Pau Monne wrote:
> > Introduce a helper to parse the Dom0 kernel.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> > Changes since v2:
> >  - Remove debug messages.
> >  - Don't hardcode the number of modules to 1.
> > ---
> >  xen/arch/x86/domain_build.c | 138 
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 138 insertions(+)
> > 
> > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> > index ec1ac89..168be62 100644
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -39,6 +39,7 @@
> >  #include <asm/hpet.h>
> >  
> >  #include <public/version.h>
> > +#include <public/arch-x86/hvm/start_info.h>
> >  
> >  static long __initdata dom0_nrpages;
> >  static long __initdata dom0_min_nrpages;
> > @@ -1895,12 +1896,141 @@ static int __init hvm_setup_p2m(struct domain *d)
> >      return 0;
> >  }
> >  
> > +static int __init hvm_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;
> > +    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;
> > +    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");
> 
> Perhaps s/ELFNOTES/PT_NOTEs/?

They can also be in a SHT_NOTE section, for example FreeBSD doesn't have a 
PT_NOTE program header, and just places the notes inside of a SHT_NOTE section.

> > +        return rc;
> > +    }
> > +
> > +    if ( parms.phys_entry == UNSET_ADDR32 ) {
> > +        printk("Unable to find kernel entry point, aborting\n");
> 
> Perhaps: Unable to find XEN_ELFNOTE_PHYS32_ENTRY point.

Right, I've changed "point" to "address" because I think it's easier to 
understand.

> > +        return -EINVAL;
> > +    }
> > +
> > +    printk("OS: %s version: %s loader: %s bitness: %s\n", parms.guest_os,
> > +           parms.guest_ver, parms.loader,
> 
> Hm, I don't know if XEN_ELFNOTE_GUEST_VERSION or XEN_ELFNOTE_GUEST_OS
> are mandated.
> 
> Perhaps you should do memset(&params) before you pass it to elf_xen_parse?

elf_xen_parse already does a memset of params.
 
> > +           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;
> > +
> > +    saved_current = current;
> > +    set_current(v);
> > +
> > +    rc = elf_load_binary(&elf);
> > +    if ( rc < 0 )
> > +    {
> > +        printk("Failed to load kernel: %d\n", rc);
> > +        printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
> > +        goto out;
> > +    }
> > +
> > +    last_addr = ROUNDUP(parms.virt_kend - parms.virt_base, PAGE_SIZE);
> > +
> > +    if ( initrd != NULL )
> > +    {
> > +        rc = hvm_copy_to_guest_phys(last_addr, 
> > mfn_to_virt(initrd->mod_start),
> > +                                    initrd->mod_end);
> > +        if ( rc != HVMCOPY_okay )
> > +        {
> > +            printk("Unable to copy initrd to guest\n");
> > +            rc = -EFAULT;
> > +            goto out;
> > +        }
> > +
> > +        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));
> > +    if ( cmdline != NULL )
> > +    {
> > +        rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 
> > 1);
> > +        if ( rc != HVMCOPY_okay )
> > +        {
> > +            printk("Unable to copy guest command line\n");
> > +            rc = -EFAULT;
> > +            goto out;
> > +        }
> > +        start_info.cmdline_paddr = last_addr;
> > +        last_addr += ROUNDUP(strlen(cmdline) + 1, 8);
> > +    }
> > +    if ( initrd != NULL )
> 
> It may be better if this is an array. You can have multiple initrd's.

Hm, I'm not really sure I can do anything about this here. ATM it seems like 
Dom0 build is limited to a single initrd blob, and that's how it's done in the 
classic PV path.

I'm also very interested in being able to pass more than one module to Dom0, 
that would make booting FreeBSD much easier, since there's not initrd there and 
I basically have to create a fake one, being able to pass more than one module 
would simplify the code there. In any case I think this is out of the scope of 
this series.

> > +    {
> > +        rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod));
> > +        if ( rc != HVMCOPY_okay )
> > +        {
> > +            printk("Unable to copy guest modules\n");
> > +            rc = -EFAULT;
> > +            goto out;
> > +        }
> > +        start_info.modlist_paddr = last_addr;
> > +        start_info.nr_modules = 1;
> > +        last_addr += sizeof(mod);
> > +    }
> > +
> > +    start_info.magic = XEN_HVM_START_MAGIC_VALUE;
> > +    start_info.flags = SIF_PRIVILEGED | SIF_INITDOMAIN;
> > +    rc = hvm_copy_to_guest_phys(last_addr, &start_info, 
> > sizeof(start_info));
> > +    if ( rc != HVMCOPY_okay )
> > +    {
> > +        printk("Unable to copy start info to guest\n");
> > +        rc = -EFAULT;
> > +        goto out;
> > +    }
> > +
> > +    *entry = parms.phys_entry;
> > +    *start_info_addr = last_addr;
> > +    rc = 0;
> > +
> > +out:
> 
> Extra space in front of the label.

Fixed, thanks!

Roger.

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