[PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup

[PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup

[Andrew Cooper]

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

[PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()

[Andrew Cooper]

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

Re: [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()

[Wei Liu]

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

Re: [PATCH 1/3] x86/pvh-dom0: Remove unnecessary function pointer call from modify_identity_mmio()

[Jan Beulich]

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

[PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Andrew Cooper]

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

Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Wei Liu]

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

Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Andrew Cooper]

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

Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Jan Beulich]

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

Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Andrew Cooper]

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

Re: [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Jan Beulich]

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

[PATCH 3/3] x86/pvh: Print the PVH start info more concisely

[Andrew Cooper]

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

Re: [PATCH 3/3] x86/pvh: Print the PVH start info more concisely

[Wei Liu]

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

Re: [PATCH 3/3] x86/pvh: Print the PVH start info more concisely

[Jan Beulich]

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

Re: [PATCH 3/3] x86/pvh: Print the PVH start info more concisely

[Andrew Cooper]

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

Re: [PATCH for-4.12 0/3] x86/pvh: Misc fixes and cleanup

[Juergen Gross]

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