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

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

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

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

Oh, I see. I didn't even notice the v2 in the title - it being a standalone
1/2 patch, I thought this was a mailing artifact.  The more that it also
doesn't list what has changed in v2.

Jan



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