Re: [Xen-devel] [PATCH v6 4/7] xen/x86: parse Dom0 kernel for PVHv2

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

On Fri, Feb 10, 2017 at 02:34:16PM +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v6 4/7] xen/x86: parse Dom0 kernel for 
> PVHv2"):
> > Introduce a helper to parse the Dom0 kernel.
> > 
> > A new helper is also introduced to libelf, that's used to store the
> > destination vcpu of the domain. This parameter is needed when
> > loading the kernel on a HVM domain (PVHv2), since
> > hvm_copy_to_guest_phys requires passing the destination vcpu.
> 
> The new helper and variable seems fine to me.
> 
> > While there also fix image_base and image_start to be of type "void
> > *", and do the necessary fixup of related functions.
> 
> IMO this should be separate patch(es).

Thanks, I've now splitted it.

> > +static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> > +                                  unsigned long image_headroom,
> > +                                  module_t *initrd, void *image_base,
> > +                                  char *cmdline, paddr_t *entry,
> > +                                  paddr_t *start_info_addr)
> > +{
> 
> FAOD this is used for dom0 only, right ?  In which case I don't feel
> the need to review it.

Right, this is Dom0 only.

> > diff --git a/xen/common/libelf/libelf-loader.c 
> > b/xen/common/libelf/libelf-loader.c
> > index 1644f16..de140ed 100644
> > --- a/xen/common/libelf/libelf-loader.c
> > +++ b/xen/common/libelf/libelf-loader.c
> > @@ -153,10 +153,19 @@ static elf_errorstatus elf_load_image(struct 
> > elf_binary *elf, elf_ptrval dst, el
> >          return -1;
> >      /* We trust the dom0 kernel image completely, so we don't care
> >       * about overruns etc. here. */
> > -    rc = raw_copy_to_guest(ELF_UNSAFE_PTR(dst), ELF_UNSAFE_PTR(src), 
> > filesz);
> > +    if ( is_hvm_vcpu(elf->vcpu) )
> > +        rc = hvm_copy_to_guest_phys((paddr_t)ELF_UNSAFE_PTR(dst),
> > +                                    ELF_UNSAFE_PTR(src), filesz, 
> > elf->vcpu);
> > +    else
> > +        rc = raw_copy_to_guest(ELF_UNSAFE_PTR(dst), ELF_UNSAFE_PTR(src),
> > +                               filesz);
> >      if ( rc != 0 )
> >          return -1;
> > -    rc = raw_clear_guest(ELF_UNSAFE_PTR(dst + filesz), memsz - filesz);
> > +    if ( is_hvm_vcpu(elf->vcpu) )
> > +        rc = hvm_copy_to_guest_phys((paddr_t)ELF_UNSAFE_PTR(dst + filesz),
> > +                                    NULL, filesz, elf->vcpu);
> > +    else
> > +        rc = raw_clear_guest(ELF_UNSAFE_PTR(dst + filesz), memsz - filesz);
> 
> This seems to involve open coding all four elements of a 2x2 matrix.
> Couldn't you provide a helper function that:
>  * Checks is_hvm_vcpu
>  * Has the "NULL means clear" behaviour which I infer
>    hvm_copy_to_guest_phys has
>  * Calls hvm_copy_to_guest_phys or raw_{copy_to,clear}_guest
> (Does raw_copy_to_guest have the "NULL means clear" feature ?  Maybe
> that feature should be added, further lifting that into more general
> code.)

In the pv case raw_copy_to_guest calls clear_user which is a specific function
to zero a memory area, and AFAICT raw_copy_to_guest doesn't have the NULL means
clear. I've added the following helper, let me know if that looks fine:

static elf_errorstatus elf_memcpy(struct vcpu *v, void *dst, void *src,
                                  uint64_t size)
{
    int rc = 0;

#ifdef CONFIG_X86
    if ( is_hvm_vcpu(v) &&
         hvm_copy_to_guest_phys((paddr_t)dst, src, size, v) != HVMCOPY_okay )
        rc = -1;
    else
#endif
        rc = src == NULL ? raw_clear_guest(dst, size) :
                           raw_copy_to_guest(dst, src, size);

    return rc;
}

Roger.

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