Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list

["Jan Beulich" <JBeulich@suse.com>]

>>> On 09.11.18 at 18:22, <roger.pau@citrix.com> wrote:
> When booting Xen as a PVH guest the data in the PVH start info
> structure is copied over to a multiboot structure and a module list
> array that resides in the .init section of the Xen image. The
> resulting multiboot structures are then handled to the generic boot
> process using their physical address.
> 
> This works fine as long as the Xen image doesn't relocate itself, if
> there's such a relocation the physical addresses of the multiboot
> structure and the module array are no longer valid.
> 
> Fix this by handling the virtual address of the multiboot structure
> and module array to the generic boot process instead of it's physical
> address.

Besides you presumably meaning "handing" instead of "handling",
I'm having trouble seeing where you convert from physical to
virtual addresses: What have been pointers before continue to
be pointers, and what have been numbers (commonly
representing physical addresses) continue to be numbers.

> --- a/xen/arch/x86/guest/pvh-boot.c
> +++ b/xen/arch/x86/guest/pvh-boot.c
> @@ -35,11 +35,11 @@ static multiboot_info_t __initdata pvh_mbi;
>  static module_t __initdata pvh_mbi_mods[8];
>  static const char *__initdata pvh_loader = "PVH Directboot";
>  
> -static void __init convert_pvh_info(void)
> +static void __init convert_pvh_info(multiboot_info_t **mbi,
> +                                    module_t **mod)
>  {
>      const struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
>      const struct hvm_modlist_entry *entry;
> -    module_t *mod;
>      unsigned int i;
>  
>      if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
> @@ -68,20 +68,22 @@ static void __init convert_pvh_info(void)
>      pvh_mbi.mods_count = pvh_info->nr_modules;
>      pvh_mbi.mods_addr = __pa(pvh_mbi_mods);
>  
> -    mod = pvh_mbi_mods;

pvh_mbi_mods is itself not changed, and now used below instead
of the original return value from pvh_init().

>      entry = __va(pvh_info->modlist_paddr);
>      for ( i = 0; i < pvh_info->nr_modules; i++ )
>      {
>          BUG_ON(entry[i].paddr >> 32);
>          BUG_ON(entry[i].cmdline_paddr >> 32);
>  
> -        mod[i].mod_start = entry[i].paddr;
> -        mod[i].mod_end   = entry[i].paddr + entry[i].size;
> -        mod[i].string    = entry[i].cmdline_paddr;
> +        pvh_mbi_mods[i].mod_start = entry[i].paddr;
> +        pvh_mbi_mods[i].mod_end   = entry[i].paddr + entry[i].size;
> +        pvh_mbi_mods[i].string    = entry[i].cmdline_paddr;
>      }
>  
>      BUG_ON(!pvh_info->rsdp_paddr);
>      rsdp_hint = pvh_info->rsdp_paddr;
> +
> +    *mbi = &pvh_mbi;
> +    *mod = pvh_mbi_mods;

And there are no __va() uses or alike getting added here (not that
it would make any sense for static variables, i.e. things sitting inside
the Xen image).

> --- a/xen/include/asm-x86/guest/pvh-boot.h
> +++ b/xen/include/asm-x86/guest/pvh-boot.h
> @@ -25,17 +25,16 @@
>  
>  extern bool pvh_boot;
>  
> -multiboot_info_t *pvh_init(void);
> +void pvh_init(multiboot_info_t **mbi, module_t **mod);
>  void pvh_print_info(void);
>  
>  #else
>  
>  #define pvh_boot 0
>  
> -static inline multiboot_info_t *pvh_init(void)
> +static inline void *pvh_init(multiboot_info_t **mbi, module_t **mod)
>  {
>      ASSERT_UNREACHABLE();
> -    return NULL;
>  }

Please don't remove the return statement. Or wait - don't you
mean the function to return "void" rather than "void *"?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel