[PATCH 0/2] guest/pvh: fixes for idle memory scrubbing

[PATCH 0/2] guest/pvh: fixes for idle memory scrubbing

[Roger Pau Monne]

Hello,

The patch to perform memory scrubbing by the idle CPUs has revealed two
latent bugs when running Xen as a PVH guest which this series attempts
to fix.

This should allow the PV shim to be functional again. The series can be
found at:

git://xenbits.xen.org/people/royger/xen.git fix_memory_scrub_v1

Roger Pau Monne (2):
  guest/pvh: fix handling of multiboot info and module list
  guest/pvh: special case the low 1MB

 xen/arch/x86/guest/pvh-boot.c        | 20 ++++++++++----------
 xen/arch/x86/mm.c                    | 13 +++++--------
 xen/arch/x86/setup.c                 |  7 ++++---
 xen/include/asm-x86/guest/pvh-boot.h |  5 ++---
 4 files changed, 21 insertions(+), 24 deletions(-)

-- 
2.19.1


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

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

[Roger Pau Monne]

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.

Reported-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/guest/pvh-boot.c        | 20 ++++++++++----------
 xen/arch/x86/setup.c                 |  7 ++++---
 xen/include/asm-x86/guest/pvh-boot.h |  5 ++---
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 6e81b32b92..544775eeb4 100644
--- 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;
     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;
 }
 
 static void __init get_memory_map(void)
@@ -98,16 +100,14 @@ static void __init get_memory_map(void)
     sanitize_e820_map(e820_raw.map, &e820_raw.nr_map);
 }
 
-multiboot_info_t *__init pvh_init(void)
+void __init pvh_init(multiboot_info_t **mbi, module_t **mod)
 {
-    convert_pvh_info();
+    convert_pvh_info(mbi, mod);
 
     probe_hypervisor();
     ASSERT(xen_guest);
 
     get_memory_map();
-
-    return &pvh_mbi;
 }
 
 void __init pvh_print_info(void)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 55a288f332..9cbff22fb3 100644
--- 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";
diff --git a/xen/include/asm-x86/guest/pvh-boot.h b/xen/include/asm-x86/guest/pvh-boot.h
index 1b429f9401..922b6e68c5 100644
--- 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;
 }
 
 static inline void pvh_print_info(void)
-- 
2.19.1


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

[PATCH 2/2] guest/pvh: special case the low 1MB

[Roger Pau Monne]

When running as a PVH guest Xen only special cases the trampoline
code in the low 1MB, without also reserving the space used by the
relocated metadata or the trampoline stack.

Fix this by always reserving the low 1MB regardless of whether Xen is
running as a guest or natively.

Reported-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2c450cc208..16c7d88a8e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -291,16 +291,13 @@ void __init arch_init_memory(void)
     BUG_ON(IS_ERR(dom_cow));
 
     /*
-     * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
-     * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
-     * of 0 being a very common default value. Also reserve the RAM needed by
-     * the trampoline on PVH starting at MFN 1.
+     * First 1MB of RAM is historically marked as I/O.
+     * Note that apart from IO Xen also uses the low 1MB to store the AP boot
+     * trampoline and boot information metadata. Due to this always special
+     * case the low 1MB.
      */
     BUG_ON(pvh_boot && trampoline_phys != 0x1000);
-    for ( i = 0;
-          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
-                        : 0x100);
-          i++ )
+    for ( i = 0; i < 0x100; i++ )
         share_xen_page_with_guest(mfn_to_page(_mfn(i)), dom_io, SHARE_rw);
 
     /* Any areas not specified as RAM by the e820 map are considered I/O. */
-- 
2.19.1


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

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

[Roger Pau Monne]

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.

Reported-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/guest/pvh-boot.c        | 20 ++++++++++----------
 xen/arch/x86/setup.c                 |  7 ++++---
 xen/include/asm-x86/guest/pvh-boot.h |  5 ++---
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 6e81b32b92..544775eeb4 100644
--- 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;
     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;
 }
 
 static void __init get_memory_map(void)
@@ -98,16 +100,14 @@ static void __init get_memory_map(void)
     sanitize_e820_map(e820_raw.map, &e820_raw.nr_map);
 }
 
-multiboot_info_t *__init pvh_init(void)
+void __init pvh_init(multiboot_info_t **mbi, module_t **mod)
 {
-    convert_pvh_info();
+    convert_pvh_info(mbi, mod);
 
     probe_hypervisor();
     ASSERT(xen_guest);
 
     get_memory_map();
-
-    return &pvh_mbi;
 }
 
 void __init pvh_print_info(void)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 55a288f332..9cbff22fb3 100644
--- 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";
diff --git a/xen/include/asm-x86/guest/pvh-boot.h b/xen/include/asm-x86/guest/pvh-boot.h
index 1b429f9401..b8a76c4eed 100644
--- 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;
 }
 
 static inline void pvh_print_info(void)
-- 
2.19.1


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

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

[Jan Beulich]

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

Re: [PATCH 2/2] guest/pvh: special case the low 1MB

[Jan Beulich]

>>> On 09.11.18 at 18:22, <roger.pau@citrix.com> wrote:
> When running as a PVH guest Xen only special cases the trampoline
> code in the low 1MB, without also reserving the space used by the
> relocated metadata or the trampoline stack.
> 
> Fix this by always reserving the low 1MB regardless of whether Xen is
> running as a guest or natively.
> 
> Reported-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Not overly nice to waste some space here, but likely better than
trying to cover all of the bits and pieces individually.

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

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

[Roger Pau Monné]

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:
> > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 12.11.18 at 16:00, <roger.pau@citrix.com> wrote:
> 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 fine. But didn't Sergey spot two regions that were not
taken care of? Or is the other part dealt with in patch 2?

Jan



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

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

[Roger Pau Monné]

On Mon, Nov 12, 2018 at 08:12:56AM -0700, Jan Beulich wrote:
> >>> On 12.11.18 at 16:00, <roger.pau@citrix.com> wrote:
> > 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 fine. But didn't Sergey spot two regions that were not
> taken care of? Or is the other part dealt with in patch 2?

IIRC the other one was the module(s) command line, which are relocated
in the low 1MB by pvh_info_reloc which should be fixed by patch #2.

Thanks, Roger.

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

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

[Wei Liu]

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

Re: [PATCH 2/2] guest/pvh: special case the low 1MB

[Wei Liu]

On Fri, Nov 09, 2018 at 06:22:50PM +0100, Roger Pau Monne wrote:
> When running as a PVH guest Xen only special cases the trampoline
> code in the low 1MB, without also reserving the space used by the
> relocated metadata or the trampoline stack.
> 
> Fix this by always reserving the low 1MB regardless of whether Xen is
> running as a guest or natively.
> 
> Reported-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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