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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 12, 2018 at 03:28:37AM -0700, Jan Beulich wrote:
> >>> On 09.11.18 at 18:22, <roger.pau@xxxxxxxxxx> 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",

Yes, sorry.

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

Currently the list of modules is returned by physical address, and
then __start_xen does a __va(mbi->mods_addr) to get the virtual
address.

This works fine when booted form multiboot, because the multiboot
metadata is relocated to the low 1MB by mbi{2}_reloc. But that's not
fine for PVH because the physical address in mbi->mods_addr points to
an address in the Xen .init section (see convert_pvh_info). Then when
__start_xen performs the relocation of Xen itself this address gets
out of sync because the .init section has relocated somewhere else,
and both mbi->mods_addr and it's associated virtual address point to a
stale .init section.

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

The patch basically avoids using __va against physical addresses in
the .init section, so that they don't become stable once Xen relocates
itself.

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

No, __va was currently used by __start_xen in order to get the virtual
address of the module list (__va(mbi->mods_addr)), which becomes stale
after relocating Xen itself because in the PVH case the mods_addrs
points to a physical address in the .init section of 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 *"?

Yes, this is a mistake, please see v2 of this patch which is already
on the list.

Thanks, Roger.

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