* [PATCH 0/2] guest/pvh: fixes for idle memory scrubbing @ 2018-11-09 17:22 Roger Pau Monne 2018-11-09 17:22 ` [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list Roger Pau Monne 2018-11-09 17:22 ` [PATCH 2/2] guest/pvh: special case the low 1MB Roger Pau Monne 0 siblings, 2 replies; 13+ messages in thread From: Roger Pau Monne @ 2018-11-09 17:22 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-09 17:22 [PATCH 0/2] guest/pvh: fixes for idle memory scrubbing Roger Pau Monne @ 2018-11-09 17:22 ` Roger Pau Monne 2018-11-09 17:54 ` [PATCH v2 " Roger Pau Monne 2018-11-12 10:28 ` [PATCH " Jan Beulich 2018-11-09 17:22 ` [PATCH 2/2] guest/pvh: special case the low 1MB Roger Pau Monne 1 sibling, 2 replies; 13+ messages in thread From: Roger Pau Monne @ 2018-11-09 17:22 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-09 17:22 ` [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list Roger Pau Monne @ 2018-11-09 17:54 ` Roger Pau Monne 2018-11-12 10:28 ` [PATCH " Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Roger Pau Monne @ 2018-11-09 17:54 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-09 17:22 ` [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list Roger Pau Monne 2018-11-09 17:54 ` [PATCH v2 " Roger Pau Monne @ 2018-11-12 10:28 ` Jan Beulich 2018-11-12 11:49 ` Roger Pau Monné 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2018-11-12 10:28 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-12 10:28 ` [PATCH " Jan Beulich @ 2018-11-12 11:49 ` Roger Pau Monné 2018-11-12 12:40 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-11-12 11:49 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-12 11:49 ` Roger Pau Monné @ 2018-11-12 12:40 ` Jan Beulich 2018-11-12 15:00 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2018-11-12 12:40 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-12 12:40 ` Jan Beulich @ 2018-11-12 15:00 ` Roger Pau Monné 2018-11-12 15:12 ` Jan Beulich 2018-11-12 15:54 ` Wei Liu 0 siblings, 2 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-11-12 15:00 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-12 15:00 ` Roger Pau Monné @ 2018-11-12 15:12 ` Jan Beulich 2018-11-12 15:17 ` Roger Pau Monné 2018-11-12 15:54 ` Wei Liu 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2018-11-12 15:12 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-12 15:12 ` Jan Beulich @ 2018-11-12 15:17 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-11-12 15:17 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list 2018-11-12 15:00 ` Roger Pau Monné 2018-11-12 15:12 ` Jan Beulich @ 2018-11-12 15:54 ` Wei Liu 1 sibling, 0 replies; 13+ messages in thread From: Wei Liu @ 2018-11-12 15:54 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] guest/pvh: special case the low 1MB 2018-11-09 17:22 [PATCH 0/2] guest/pvh: fixes for idle memory scrubbing Roger Pau Monne 2018-11-09 17:22 ` [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list Roger Pau Monne @ 2018-11-09 17:22 ` Roger Pau Monne 2018-11-12 10:31 ` Jan Beulich 2018-11-12 15:54 ` Wei Liu 1 sibling, 2 replies; 13+ messages in thread From: Roger Pau Monne @ 2018-11-09 17:22 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] guest/pvh: special case the low 1MB 2018-11-09 17:22 ` [PATCH 2/2] guest/pvh: special case the low 1MB Roger Pau Monne @ 2018-11-12 10:31 ` Jan Beulich 2018-11-12 15:54 ` Wei Liu 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2018-11-12 10:31 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] guest/pvh: special case the low 1MB 2018-11-09 17:22 ` [PATCH 2/2] guest/pvh: special case the low 1MB Roger Pau Monne 2018-11-12 10:31 ` Jan Beulich @ 2018-11-12 15:54 ` Wei Liu 1 sibling, 0 replies; 13+ messages in thread From: Wei Liu @ 2018-11-12 15:54 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-11-12 15:55 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-09 17:22 [PATCH 0/2] guest/pvh: fixes for idle memory scrubbing Roger Pau Monne 2018-11-09 17:22 ` [PATCH 1/2] guest/pvh: fix handling of multiboot info and module list Roger Pau Monne 2018-11-09 17:54 ` [PATCH v2 " Roger Pau Monne 2018-11-12 10:28 ` [PATCH " Jan Beulich 2018-11-12 11:49 ` Roger Pau Monné 2018-11-12 12:40 ` Jan Beulich 2018-11-12 15:00 ` Roger Pau Monné 2018-11-12 15:12 ` Jan Beulich 2018-11-12 15:17 ` Roger Pau Monné 2018-11-12 15:54 ` Wei Liu 2018-11-09 17:22 ` [PATCH 2/2] guest/pvh: special case the low 1MB Roger Pau Monne 2018-11-12 10:31 ` Jan Beulich 2018-11-12 15:54 ` Wei Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.