Re: [Xen-devel] [PATCH v4 11/14] xen/x86: parse Dom0 kernel for PVHv2

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

On Fri, Dec 09, 2016 at 10:05:18AM -0700, Jan Beulich wrote:
> >>> On 30.11.16 at 17:49, <roger.pau@xxxxxxxxxx> wrote:
> > @@ -1930,12 +1931,148 @@ static int __init hvm_setup_p2m(struct domain *d)
> >  #undef MB1_PAGES
> >  }
> >  
> > +static int __init hvm_copy_to_phys(struct domain *d, paddr_t paddr, void 
> > *buf,
> > +                                   int size)
> 
> I guess you made size plain int because hvm_copy_to_guest_phys()
> has it that way, but please let's not spread such bogus things - sizes
> can't possibly be negative.
> 
> > +{
> > +    struct vcpu *saved_current;
> > +    int rc;
> > +
> > +    saved_current = current;
> > +    set_current(d->vcpu[0]);
> > +    rc = hvm_copy_to_guest_phys(paddr, buf, size);
> > +    set_current(saved_current);
> 
> I continue to be uncertain about the behavior of this if something
> inside hvm_copy_to_guest_phys() goes wrong: Did you either
> statically analyze the code or try in practice out whether the
> playing with current makes understanding the crash output any
> harder?

If you managed to somehow call hvm_copy_to_guest_phys with the idle vcpu as
current you would get this kind of error, which I admin is maybe not that
obvious (apart from the IDLEv0 prefix).

(XEN) IDLEv0 Error pfn 21bd: rd=32767 od=32756 caf=180000000000000 
taf=0000000000000000

See below.

> While there's going to be some work involved with it, I do think
> that the use here might be a reason for the whole hvm_copy()
> machinery to gain a struct vcpu* parameter.

I've gone that route and added a new param to __hvm_copy, and also introduced
hvm_copy_to_guest_phys_vcpu which takes an additional vcpu parameter. While
there I've also added an assert to __hvm_copy in order to make sure the
vcpu parameter is always a hvm/pvh vcpu.

> > +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");
> > +        return rc;
> > +    }
> > +
> > +    if ( parms.phys_entry == UNSET_ADDR32 ) {
> > +        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;
> > +
> > +    saved_current = current;
> > +    set_current(v);
> > +    rc = elf_load_binary(&elf);
> > +    set_current(saved_current);
> 
> Same reservations as above.

Right, this one however is more tricky to fix since elf_load_binary is shared
with libxc, so adding a vcpu/domain parameter here is problematic for the
toolstack side. That's quite similar to what happens on classic PV Dom0
creation, we need to switch to Dom0 page tables. I'm not trying to use that to
justify that this is the best way, but everything else seems quite convoluted
(either adding a new param to elf_load_binary or a new field to struct
elf_binary in order to store the domain/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_phys(d, last_addr, mfn_to_virt(initrd->mod_start),
> > +                              initrd->mod_end);
> > +        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);
> > +    }
> 
> mod is left uninitialized in the else case afaict - I don't think all
> compilers we support (plus Coverity) can spot the common
> dependency on initrd != NULL.

Clang doesn't seem to complain, but I will add an initialized to be sure.

Thanks, Roger.

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