* [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup
@ 2019-01-21 15:37 Andrew Cooper
2019-01-21 15:37 ` [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio() Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Andrew Cooper @ 2019-01-21 15:37 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
These are all minor fixes from some recent PVH work. They are all limited to
the boot path, and low-risk nice-to-have's for 4.12.
Andrew Cooper (3):
x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()
x86/pvh: Fixes to convert_pvh_info()
x86/pvh: Print the PVH start info more concisely
xen/arch/x86/guest/pvh-boot.c | 48 +++++++++++++++++++++++++------------------
xen/arch/x86/hvm/dom0_build.c | 4 ++--
2 files changed, 30 insertions(+), 22 deletions(-)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()
2019-01-21 15:37 [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Andrew Cooper
@ 2019-01-21 15:37 ` Andrew Cooper
2019-01-21 15:52 ` Wei Liu
2019-01-21 15:37 ` [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info() Andrew Cooper
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2019-01-21 15:37 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
Function pointer calls are far more expensive in a post-Spectre world, and
this one doesn't need to be.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/hvm/dom0_build.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 303c44b..51cf490 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -79,8 +79,8 @@ static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
for ( ; ; )
{
- rc = (map ? map_mmio_regions : unmap_mmio_regions)
- (d, _gfn(pfn), nr_pages, _mfn(pfn));
+ rc = map ? map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
+ : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
if ( rc == 0 )
break;
if ( rc < 0 )
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()
2019-01-21 15:37 [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Andrew Cooper
2019-01-21 15:37 ` [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio() Andrew Cooper
@ 2019-01-21 15:37 ` Andrew Cooper
2019-01-21 15:52 ` Wei Liu
2019-01-23 11:35 ` Jan Beulich
2019-01-21 15:37 ` [PATCH 3/3] x86/pvh: Print the PVH start info more concisely Andrew Cooper
2019-01-23 11:27 ` [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Juergen Gross
3 siblings, 2 replies; 15+ messages in thread
From: Andrew Cooper @ 2019-01-21 15:37 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
panic() doesn't contain any caller information, so the sum output of:
(d1) (XEN)
(d1) (XEN) ****************************************
(d1) (XEN) Panic on CPU 0:
(d1) (XEN) Magic value is wrong: 336ec568
(d1) (XEN) ****************************************
(d1) (XEN)
isn't helpful at identifying what went wrong. Update the panic() strings to
identify PVH and aid with diagnostics.
The BUG_ON() check for ARRAY_SIZE(pvh_mbi_mods) is off-by-one, and redundant
with the earlier panic() which explains things in more detail. Drop it.
Finally, Xen takes nr_modules != 0 to mean that modlist_paddr is valid, but a
cleverly crafterd PVH start info layout can cause Xen to use modlist_paddr at
gaddr 0, in violation of the PVH spec.
As such a start info would be a domain builder bug, crosscheck and ignore
modules in such a case, rather than fixing up all of Xen to check both values.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/guest/pvh-boot.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 544775e..01f1376 100644
--- a/xen/arch/x86/guest/pvh-boot.c
+++ b/xen/arch/x86/guest/pvh-boot.c
@@ -38,12 +38,20 @@ static const char *__initdata pvh_loader = "PVH Directboot";
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);
+ struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
const struct hvm_modlist_entry *entry;
unsigned int i;
if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
- panic("Magic value is wrong: %x\n", pvh_info->magic);
+ panic("PVH magic value is wrong: %x\n", pvh_info->magic);
+
+ /* Check consistency between the modlist and number of modules. */
+ if ( (pvh_info->modlist_paddr == 0) != (pvh_info->nr_modules == 0) )
+ {
+ printk("PVH module mismatch: pa %08"PRIx64", nr %u - Ignoring\n",
+ pvh_info->modlist_paddr, pvh_info->nr_modules);
+ pvh_info->modlist_paddr = pvh_info->nr_modules = 0;
+ }
/*
* Temporary module array needs to be at least one element bigger than
@@ -51,8 +59,8 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
* arch/x86/setup.c:__start_xen().
*/
if ( ARRAY_SIZE(pvh_mbi_mods) <= pvh_info->nr_modules )
- panic("The module array is too small, size %zu, requested %u\n",
- ARRAY_SIZE(pvh_mbi_mods), pvh_info->nr_modules);
+ panic("Insufficient PVH module array entries. Have %zu, need %u\n",
+ ARRAY_SIZE(pvh_mbi_mods), pvh_info->nr_modules + 1);
/*
* Turn hvm_start_info into mbi. Luckily all modules are placed under 4GB
@@ -64,7 +72,6 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
pvh_mbi.cmdline = pvh_info->cmdline_paddr;
pvh_mbi.boot_loader_name = __pa(pvh_loader);
- BUG_ON(pvh_info->nr_modules >= ARRAY_SIZE(pvh_mbi_mods));
pvh_mbi.mods_count = pvh_info->nr_modules;
pvh_mbi.mods_addr = __pa(pvh_mbi_mods);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] x86/pvh: Print the PVH start info more concisely
2019-01-21 15:37 [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Andrew Cooper
2019-01-21 15:37 ` [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio() Andrew Cooper
2019-01-21 15:37 ` [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info() Andrew Cooper
@ 2019-01-21 15:37 ` Andrew Cooper
2019-01-21 15:53 ` Wei Liu
2019-01-23 11:42 ` Jan Beulich
2019-01-23 11:27 ` [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Juergen Gross
3 siblings, 2 replies; 15+ messages in thread
From: Andrew Cooper @ 2019-01-21 15:37 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
The current rendering of PVH start info in unnecessarily verbose, and doesn't
clearly separate decimal and hex numbers.
All addresses are expected to be below the 4G boundary, so drop 8 redundant
zeroes on all physical addresses. Properly prefix all hex numbers, and fold
related information onto the same line.
Before:
(XEN) PVH start info: (pa 0000ffc0)
(XEN) version: 1
(XEN) flags: 0
(XEN) nr_modules: 1
(XEN) modlist_pa: 000000000000ff80
(XEN) cmdline_pa: 000000000000ffa0
(XEN) cmdline: 'console=pv,xen'
(XEN) rsdp_pa: 00000000fc009000
(XEN) mod[0].pa: 0000000000599000
(XEN) mod[0].size: 0000000001204224
(XEN) mod[0].cmdline_pa: 0000000000000000
After:
(XEN) PVH start info: (pa 0x0000ffc0)
(XEN) version 1, flags 0
(XEN) cmdline 0x0000ffa0 'console=pv,xen'
(XEN) rsdp 0xfc009000
(XEN) modlist 0x0000ff80, nr 1
(XEN) mod0 pa 0x005f4000, sz 0x00126000, cmdline 0x00000000
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/guest/pvh-boot.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 01f1376..0845f0f 100644
--- a/xen/arch/x86/guest/pvh-boot.c
+++ b/xen/arch/x86/guest/pvh-boot.c
@@ -123,28 +123,29 @@ void __init pvh_print_info(void)
const struct hvm_modlist_entry *entry;
unsigned int i;
- ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
-
- printk("PVH start info: (pa %08x)\n", pvh_start_info_pa);
- printk(" version: %u\n", pvh_info->version);
- printk(" flags: %#"PRIx32"\n", pvh_info->flags);
- printk(" nr_modules: %u\n", pvh_info->nr_modules);
- printk(" modlist_pa: %016"PRIx64"\n", pvh_info->modlist_paddr);
- printk(" cmdline_pa: %016"PRIx64"\n", pvh_info->cmdline_paddr);
+ printk("PVH start info: (pa 0x%08x)\n", pvh_start_info_pa);
+ printk(" version %u, flags %#x\n", pvh_info->version, pvh_info->flags);
+
+ printk(" cmdline 0x%08"PRIx64, pvh_info->cmdline_paddr);
if ( pvh_info->cmdline_paddr )
- printk(" cmdline: '%s'\n", (char *)__va(pvh_info->cmdline_paddr));
- printk(" rsdp_pa: %016"PRIx64"\n", pvh_info->rsdp_paddr);
+ printk(" '%s'", (char *)__va(pvh_info->cmdline_paddr));
+ printk("\n");
+
+ printk(" rsdp 0x%08"PRIx64"\n", pvh_info->rsdp_paddr);
+
+ printk(" modlist 0x%08"PRIx64", nr %u\n",
+ pvh_info->modlist_paddr, pvh_info->nr_modules);
entry = __va(pvh_info->modlist_paddr);
for ( i = 0; i < pvh_info->nr_modules; i++ )
{
- printk(" mod[%u].pa: %016"PRIx64"\n", i, entry[i].paddr);
- printk(" mod[%u].size: %016"PRIu64"\n", i, entry[i].size);
- printk(" mod[%u].cmdline_pa: %016"PRIx64"\n",
- i, entry[i].cmdline_paddr);
+ printk(" mod%u pa 0x%08"PRIx64", sz 0x%08"PRIx64", cmdline 0x%08" PRIx64,
+ i, entry[i].paddr, entry[i].size, entry[i].cmdline_paddr);
+
if ( entry[i].cmdline_paddr )
- printk(" mod[%1u].cmdline: '%s'\n", i,
- (char *)__va(entry[i].cmdline_paddr));
+ printk(" '%s'\n", (char *)__va(entry[i].cmdline_paddr));
+
+ printk("\n");
}
}
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()
2019-01-21 15:37 ` [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info() Andrew Cooper
@ 2019-01-21 15:52 ` Wei Liu
2019-01-21 15:54 ` Andrew Cooper
2019-01-23 11:35 ` Jan Beulich
1 sibling, 1 reply; 15+ messages in thread
From: Wei Liu @ 2019-01-21 15:52 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Roger Pau Monné, Wei Liu, Jan Beulich, Xen-devel
On Mon, Jan 21, 2019 at 03:37:20PM +0000, Andrew Cooper wrote:
> panic() doesn't contain any caller information, so the sum output of:
>
> (d1) (XEN)
> (d1) (XEN) ****************************************
> (d1) (XEN) Panic on CPU 0:
> (d1) (XEN) Magic value is wrong: 336ec568
> (d1) (XEN) ****************************************
> (d1) (XEN)
>
> isn't helpful at identifying what went wrong. Update the panic() strings to
> identify PVH and aid with diagnostics.
>
> The BUG_ON() check for ARRAY_SIZE(pvh_mbi_mods) is off-by-one, and redundant
> with the earlier panic() which explains things in more detail. Drop it.
>
> Finally, Xen takes nr_modules != 0 to mean that modlist_paddr is valid, but a
> cleverly crafterd PVH start info layout can cause Xen to use modlist_paddr at
> gaddr 0, in violation of the PVH spec.
Do you mean "using gaddr 0" is violation of spec? But I don't seem to
find it written down in the header file.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()
2019-01-21 15:37 ` [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio() Andrew Cooper
@ 2019-01-21 15:52 ` Wei Liu
2019-01-23 11:22 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Wei Liu @ 2019-01-21 15:52 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Roger Pau Monné, Wei Liu, Jan Beulich, Xen-devel
On Mon, Jan 21, 2019 at 03:37:19PM +0000, Andrew Cooper wrote:
> Function pointer calls are far more expensive in a post-Spectre world, and
> this one doesn't need to be.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
This isn't a hot path though, so I wouldn't be too concerned about the
performance aspect of function pointer calls.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] x86/pvh: Print the PVH start info more concisely
2019-01-21 15:37 ` [PATCH 3/3] x86/pvh: Print the PVH start info more concisely Andrew Cooper
@ 2019-01-21 15:53 ` Wei Liu
2019-01-23 11:42 ` Jan Beulich
1 sibling, 0 replies; 15+ messages in thread
From: Wei Liu @ 2019-01-21 15:53 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Roger Pau Monné, Wei Liu, Jan Beulich, Xen-devel
On Mon, Jan 21, 2019 at 03:37:21PM +0000, Andrew Cooper wrote:
> The current rendering of PVH start info in unnecessarily verbose, and doesn't
> clearly separate decimal and hex numbers.
>
> All addresses are expected to be below the 4G boundary, so drop 8 redundant
> zeroes on all physical addresses. Properly prefix all hex numbers, and fold
> related information onto the same line.
>
> Before:
> (XEN) PVH start info: (pa 0000ffc0)
> (XEN) version: 1
> (XEN) flags: 0
> (XEN) nr_modules: 1
> (XEN) modlist_pa: 000000000000ff80
> (XEN) cmdline_pa: 000000000000ffa0
> (XEN) cmdline: 'console=pv,xen'
> (XEN) rsdp_pa: 00000000fc009000
> (XEN) mod[0].pa: 0000000000599000
> (XEN) mod[0].size: 0000000001204224
> (XEN) mod[0].cmdline_pa: 0000000000000000
>
> After:
> (XEN) PVH start info: (pa 0x0000ffc0)
> (XEN) version 1, flags 0
> (XEN) cmdline 0x0000ffa0 'console=pv,xen'
> (XEN) rsdp 0xfc009000
> (XEN) modlist 0x0000ff80, nr 1
> (XEN) mod0 pa 0x005f4000, sz 0x00126000, cmdline 0x00000000
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@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] 15+ messages in thread
* Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()
2019-01-21 15:52 ` Wei Liu
@ 2019-01-21 15:54 ` Andrew Cooper
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2019-01-21 15:54 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, Roger Pau Monné, Jan Beulich, Xen-devel
On 21/01/2019 15:52, Wei Liu wrote:
> On Mon, Jan 21, 2019 at 03:37:20PM +0000, Andrew Cooper wrote:
>> panic() doesn't contain any caller information, so the sum output of:
>>
>> (d1) (XEN)
>> (d1) (XEN) ****************************************
>> (d1) (XEN) Panic on CPU 0:
>> (d1) (XEN) Magic value is wrong: 336ec568
>> (d1) (XEN) ****************************************
>> (d1) (XEN)
>>
>> isn't helpful at identifying what went wrong. Update the panic() strings to
>> identify PVH and aid with diagnostics.
>>
>> The BUG_ON() check for ARRAY_SIZE(pvh_mbi_mods) is off-by-one, and redundant
>> with the earlier panic() which explains things in more detail. Drop it.
>>
>> Finally, Xen takes nr_modules != 0 to mean that modlist_paddr is valid, but a
>> cleverly crafterd PVH start info layout can cause Xen to use modlist_paddr at
>> gaddr 0, in violation of the PVH spec.
> Do you mean "using gaddr 0" is violation of spec? But I don't seem to
> find it written down in the header file.
a gaddr of 0 indicates no modlist (per the general statement of
addresses being zero), but Xen will use this if nr_modules != 0.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()
2019-01-21 15:52 ` Wei Liu
@ 2019-01-23 11:22 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2019-01-23 11:22 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 21.01.19 at 16:52, <wei.liu2@citrix.com> wrote:
> On Mon, Jan 21, 2019 at 03:37:19PM +0000, Andrew Cooper wrote:
>> Function pointer calls are far more expensive in a post-Spectre world, and
>> this one doesn't need to be.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Arguably a compiler could be intelligent enough to use direct calls
despite the ?: construct, though.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup
2019-01-21 15:37 [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Andrew Cooper
` (2 preceding siblings ...)
2019-01-21 15:37 ` [PATCH 3/3] x86/pvh: Print the PVH start info more concisely Andrew Cooper
@ 2019-01-23 11:27 ` Juergen Gross
3 siblings, 0 replies; 15+ messages in thread
From: Juergen Gross @ 2019-01-23 11:27 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Wei Liu, Jan Beulich, Roger Pau Monné
On 21/01/2019 16:37, Andrew Cooper wrote:
> These are all minor fixes from some recent PVH work. They are all limited to
> the boot path, and low-risk nice-to-have's for 4.12.
>
> Andrew Cooper (3):
> x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()
> x86/pvh: Fixes to convert_pvh_info()
> x86/pvh: Print the PVH start info more concisely
>
> xen/arch/x86/guest/pvh-boot.c | 48 +++++++++++++++++++++++++------------------
> xen/arch/x86/hvm/dom0_build.c | 4 ++--
> 2 files changed, 30 insertions(+), 22 deletions(-)
>
For the series:
Release-acked-by: Juergen Gross <jgross@suse.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()
2019-01-21 15:37 ` [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info() Andrew Cooper
2019-01-21 15:52 ` Wei Liu
@ 2019-01-23 11:35 ` Jan Beulich
2019-01-29 20:43 ` Andrew Cooper
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2019-01-23 11:35 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 21.01.19 at 16:37, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/guest/pvh-boot.c
> +++ b/xen/arch/x86/guest/pvh-boot.c
> @@ -38,12 +38,20 @@ static const char *__initdata pvh_loader = "PVH Directboot";
> 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);
> + struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
> const struct hvm_modlist_entry *entry;
> unsigned int i;
>
> if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
> - panic("Magic value is wrong: %x\n", pvh_info->magic);
> + panic("PVH magic value is wrong: %x\n", pvh_info->magic);
> +
> + /* Check consistency between the modlist and number of modules. */
> + if ( (pvh_info->modlist_paddr == 0) != (pvh_info->nr_modules == 0) )
> + {
> + printk("PVH module mismatch: pa %08"PRIx64", nr %u - Ignoring\n",
> + pvh_info->modlist_paddr, pvh_info->nr_modules);
> + pvh_info->modlist_paddr = pvh_info->nr_modules = 0;
> + }
While we don't consume memmap_{paddr,entries} (yet), wouldn't
it make sense to also check those for similar consistency?
Furthermore I'm not convinced the check above is correct: I don't
see anything wrong with a random modlist_paddr as long as
nr_modules is zero. In particular it is not uncommon for placement
implementations to assign the next sequential address to the next
item to process before looking at or iterating over the number of
associated entries.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] x86/pvh: Print the PVH start info more concisely
2019-01-21 15:37 ` [PATCH 3/3] x86/pvh: Print the PVH start info more concisely Andrew Cooper
2019-01-21 15:53 ` Wei Liu
@ 2019-01-23 11:42 ` Jan Beulich
2019-01-23 12:26 ` Andrew Cooper
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2019-01-23 11:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 21.01.19 at 16:37, <andrew.cooper3@citrix.com> wrote:
> The current rendering of PVH start info in unnecessarily verbose, and doesn't
> clearly separate decimal and hex numbers.
As expressed on earlier occasions, I think blindly adding 0x in
all cases goes too far. When context makes sufficiently clear
that it's a hex number (like when addresses get printed) I don't
see the need. Nevertheless, knowing you disagree,
Acked-by: Jan Beulich <jbeulich@suse.com>
with two instances of one further question below.
> --- a/xen/arch/x86/guest/pvh-boot.c
> +++ b/xen/arch/x86/guest/pvh-boot.c
> @@ -123,28 +123,29 @@ void __init pvh_print_info(void)
> const struct hvm_modlist_entry *entry;
> unsigned int i;
>
> - ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
> -
> - printk("PVH start info: (pa %08x)\n", pvh_start_info_pa);
> - printk(" version: %u\n", pvh_info->version);
> - printk(" flags: %#"PRIx32"\n", pvh_info->flags);
> - printk(" nr_modules: %u\n", pvh_info->nr_modules);
> - printk(" modlist_pa: %016"PRIx64"\n", pvh_info->modlist_paddr);
> - printk(" cmdline_pa: %016"PRIx64"\n", pvh_info->cmdline_paddr);
> + printk("PVH start info: (pa 0x%08x)\n", pvh_start_info_pa);
> + printk(" version %u, flags %#x\n", pvh_info->version, pvh_info->flags);
> +
> + printk(" cmdline 0x%08"PRIx64, pvh_info->cmdline_paddr);
> if ( pvh_info->cmdline_paddr )
> - printk(" cmdline: '%s'\n", (char *)__va(pvh_info->cmdline_paddr));
> - printk(" rsdp_pa: %016"PRIx64"\n", pvh_info->rsdp_paddr);
> + printk(" '%s'", (char *)__va(pvh_info->cmdline_paddr));
Is the cast here really necessary?
> + printk("\n");
> +
> + printk(" rsdp 0x%08"PRIx64"\n", pvh_info->rsdp_paddr);
> +
> + printk(" modlist 0x%08"PRIx64", nr %u\n",
> + pvh_info->modlist_paddr, pvh_info->nr_modules);
>
> entry = __va(pvh_info->modlist_paddr);
> for ( i = 0; i < pvh_info->nr_modules; i++ )
> {
> - printk(" mod[%u].pa: %016"PRIx64"\n", i, entry[i].paddr);
> - printk(" mod[%u].size: %016"PRIu64"\n", i, entry[i].size);
> - printk(" mod[%u].cmdline_pa: %016"PRIx64"\n",
> - i, entry[i].cmdline_paddr);
> + printk(" mod%u pa 0x%08"PRIx64", sz 0x%08"PRIx64", cmdline 0x%08" PRIx64,
> + i, entry[i].paddr, entry[i].size, entry[i].cmdline_paddr);
> +
> if ( entry[i].cmdline_paddr )
> - printk(" mod[%1u].cmdline: '%s'\n", i,
> - (char *)__va(entry[i].cmdline_paddr));
> + printk(" '%s'\n", (char *)__va(entry[i].cmdline_paddr));
Same here then.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] x86/pvh: Print the PVH start info more concisely
2019-01-23 11:42 ` Jan Beulich
@ 2019-01-23 12:26 ` Andrew Cooper
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2019-01-23 12:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monne
On 23/01/2019 11:42, Jan Beulich wrote:
>
>> --- a/xen/arch/x86/guest/pvh-boot.c
>> +++ b/xen/arch/x86/guest/pvh-boot.c
>> @@ -123,28 +123,29 @@ void __init pvh_print_info(void)
>> const struct hvm_modlist_entry *entry;
>> unsigned int i;
>>
>> - ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
>> -
>> - printk("PVH start info: (pa %08x)\n", pvh_start_info_pa);
>> - printk(" version: %u\n", pvh_info->version);
>> - printk(" flags: %#"PRIx32"\n", pvh_info->flags);
>> - printk(" nr_modules: %u\n", pvh_info->nr_modules);
>> - printk(" modlist_pa: %016"PRIx64"\n", pvh_info->modlist_paddr);
>> - printk(" cmdline_pa: %016"PRIx64"\n", pvh_info->cmdline_paddr);
>> + printk("PVH start info: (pa 0x%08x)\n", pvh_start_info_pa);
>> + printk(" version %u, flags %#x\n", pvh_info->version, pvh_info->flags);
>> +
>> + printk(" cmdline 0x%08"PRIx64, pvh_info->cmdline_paddr);
>> if ( pvh_info->cmdline_paddr )
>> - printk(" cmdline: '%s'\n", (char *)__va(pvh_info->cmdline_paddr));
>> - printk(" rsdp_pa: %016"PRIx64"\n", pvh_info->rsdp_paddr);
>> + printk(" '%s'", (char *)__va(pvh_info->cmdline_paddr));
> Is the cast here really necessary?
Yes. Omitting it causes -Wformat to object to passing void* into
something expecting char*.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()
2019-01-23 11:35 ` Jan Beulich
@ 2019-01-29 20:43 ` Andrew Cooper
2019-01-30 9:46 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2019-01-29 20:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monne
On 23/01/2019 11:35, Jan Beulich wrote:
>>>> On 21.01.19 at 16:37, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/guest/pvh-boot.c
>> +++ b/xen/arch/x86/guest/pvh-boot.c
>> @@ -38,12 +38,20 @@ static const char *__initdata pvh_loader = "PVH Directboot";
>> 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);
>> + struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
>> const struct hvm_modlist_entry *entry;
>> unsigned int i;
>>
>> if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
>> - panic("Magic value is wrong: %x\n", pvh_info->magic);
>> + panic("PVH magic value is wrong: %x\n", pvh_info->magic);
>> +
>> + /* Check consistency between the modlist and number of modules. */
>> + if ( (pvh_info->modlist_paddr == 0) != (pvh_info->nr_modules == 0) )
>> + {
>> + printk("PVH module mismatch: pa %08"PRIx64", nr %u - Ignoring\n",
>> + pvh_info->modlist_paddr, pvh_info->nr_modules);
>> + pvh_info->modlist_paddr = pvh_info->nr_modules = 0;
>> + }
> While we don't consume memmap_{paddr,entries} (yet), wouldn't
> it make sense to also check those for similar consistency?
Plausibly, but as you note, its not like we use any of that yet. Also,
it needs an ABI version check, so I'm not going to complicated this
patch with speculative work.
>
> Furthermore I'm not convinced the check above is correct: I don't
> see anything wrong with a random modlist_paddr as long as
> nr_modules is zero.
The problem case is the opposite way around - when nr_modules is nonzero
and paddr is 0.
Several of the loops in Xen will really go wrong if then encounter such
a malformed entry.
> In particular it is not uncommon for placement
> implementations to assign the next sequential address to the next
> item to process before looking at or iterating over the number of
> associated entries.
I'd put that firmly in the class of "buggy firmware". Leaving dangling
pointers is never a good idea, even if it isn't strictly speaking in
violation of the protocol in question.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()
2019-01-29 20:43 ` Andrew Cooper
@ 2019-01-30 9:46 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2019-01-30 9:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monne
>>> On 29.01.19 at 21:43, <andrew.cooper3@citrix.com> wrote:
> On 23/01/2019 11:35, Jan Beulich wrote:
>>>>> On 21.01.19 at 16:37, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/guest/pvh-boot.c
>>> +++ b/xen/arch/x86/guest/pvh-boot.c
>>> @@ -38,12 +38,20 @@ static const char *__initdata pvh_loader = "PVH
> Directboot";
>>> 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);
>>> + struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
>>> const struct hvm_modlist_entry *entry;
>>> unsigned int i;
>>>
>>> if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
>>> - panic("Magic value is wrong: %x\n", pvh_info->magic);
>>> + panic("PVH magic value is wrong: %x\n", pvh_info->magic);
>>> +
>>> + /* Check consistency between the modlist and number of modules. */
>>> + if ( (pvh_info->modlist_paddr == 0) != (pvh_info->nr_modules == 0) )
>>> + {
>>> + printk("PVH module mismatch: pa %08"PRIx64", nr %u - Ignoring\n",
>>> + pvh_info->modlist_paddr, pvh_info->nr_modules);
>>> + pvh_info->modlist_paddr = pvh_info->nr_modules = 0;
>>> + }
>> While we don't consume memmap_{paddr,entries} (yet), wouldn't
>> it make sense to also check those for similar consistency?
>
> Plausibly, but as you note, its not like we use any of that yet. Also,
> it needs an ABI version check, so I'm not going to complicated this
> patch with speculative work.
Okay.
>> Furthermore I'm not convinced the check above is correct: I don't
>> see anything wrong with a random modlist_paddr as long as
>> nr_modules is zero.
>
> The problem case is the opposite way around - when nr_modules is nonzero
> and paddr is 0.
>
> Several of the loops in Xen will really go wrong if then encounter such
> a malformed entry.
Right. My comment was to ask that you relax the check accordingly;
I'm not sure whether to interpret your reply that way ...
>> In particular it is not uncommon for placement
>> implementations to assign the next sequential address to the next
>> item to process before looking at or iterating over the number of
>> associated entries.
>
> I'd put that firmly in the class of "buggy firmware". Leaving dangling
> pointers is never a good idea, even if it isn't strictly speaking in
> violation of the protocol in question.
... while this actually makes it sound as if you'd rather want to keep
the too strict check. It's a matter of taste to some degree - to me,
a pointer to an array isn't "dangling" when the array size is known
to be zero. For debugging purposes one may even put in a non-
canonical pointer instead of a NULL one in such cases.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-01-30 9:46 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-21 15:37 [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Andrew Cooper
2019-01-21 15:37 ` [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio() Andrew Cooper
2019-01-21 15:52 ` Wei Liu
2019-01-23 11:22 ` Jan Beulich
2019-01-21 15:37 ` [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info() Andrew Cooper
2019-01-21 15:52 ` Wei Liu
2019-01-21 15:54 ` Andrew Cooper
2019-01-23 11:35 ` Jan Beulich
2019-01-29 20:43 ` Andrew Cooper
2019-01-30 9:46 ` Jan Beulich
2019-01-21 15:37 ` [PATCH 3/3] x86/pvh: Print the PVH start info more concisely Andrew Cooper
2019-01-21 15:53 ` Wei Liu
2019-01-23 11:42 ` Jan Beulich
2019-01-23 12:26 ` Andrew Cooper
2019-01-23 11:27 ` [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup Juergen Gross
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.