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

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Nov 12, 2018 at 04:00:06PM +0100, Roger Pau Monné wrote:
> On Mon, Nov 12, 2018 at 05:40:21AM -0700, Jan Beulich wrote:
> > >>> On 12.11.18 at 12:49, <roger.pau@citrix.com> wrote:
> > > On Mon, Nov 12, 2018 at 03:28:37AM -0700, Jan Beulich wrote:
> > >> >>> On 09.11.18 at 18:22, <roger.pau@citrix.com> wrote:
> > >> >      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.
> > 
> > So aiui you refer to this hunk:
> > 
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -719,12 +719,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >           */
> >          opt_console_xen = -1;
> >          ASSERT(mbi_p == 0);
> > -        mbi = pvh_init();
> > +        pvh_init(&mbi, &mod);
> >      }
> >      else
> > +    {
> >          mbi = __va(mbi_p);
> > -
> > -    mod = __va(mbi->mods_addr);
> > +        mod = __va(mbi->mods_addr);
> > +    }
> >  
> >      loader = (mbi->flags & MBI_LOADERNAME)
> >          ? (char *)__va(mbi->boot_loader_name) : "unknown";
> > 
> > Which indeed bypasses the passing through __va() for mods_addr,
> > but the last paragraph of the description suggests that you also
> > alter how mbi gets handled, which is perhaps part of my confusion.
> 
> Yes, it's only the module list that's passed by physical address and
> then mapped using __va. How about the following description:
> 
> "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 module list is then handed to the generic boot
> process using the physical address in mbi->mods_addr.
> 
> This works fine as long as the Xen image doesn't relocate itself, if
> there's such a relocation the physical addresses of multiboot module
> list is no longer valid.
> 
> Fix this by handing the virtual address of the module list to the
> generic boot process instead of it's physical address."

Sounds good to me FWIW.

Wei.

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