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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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

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

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

Then the source and destination calculations would be done once for
each part, rather than twice, and the is_hvm_vcpu condition would be
done once rather than twice.

Thanks,
Ian.

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