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

Thanks, Roger.

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