[Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX enabled

[Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX enabled

[Stefan Weil]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 target/i386/whpx-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 31d47320e4..ed95105eae 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -1396,7 +1396,7 @@ static int whpx_accel_init(MachineState *ms)
     }
 
     memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
-    prop.ProcessorCount = smp_cpus;
+    prop.ProcessorCount = ms->smp.cpus;
     hr = whp_dispatch.WHvSetPartitionProperty(
         whpx->partition,
         WHvPartitionPropertyCodeProcessorCount,
@@ -1405,7 +1405,7 @@ static int whpx_accel_init(MachineState *ms)
 
     if (FAILED(hr)) {
         error_report("WHPX: Failed to set partition core count to %d,"
-                     " hr=%08lx", smp_cores, hr);
+                     " hr=%08lx", ms->smp.cores, hr);
         ret = -EINVAL;
         goto error;
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX enabled

[Philippe Mathieu-Daudé]

Cc'ing Justin

Maybe we should add a MAINTAINERS section for the WHPX files.

On 7/12/19 3:26 PM, Stefan Weil wrote:
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  target/i386/whpx-all.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
> index 31d47320e4..ed95105eae 100644
> --- a/target/i386/whpx-all.c
> +++ b/target/i386/whpx-all.c
> @@ -1396,7 +1396,7 @@ static int whpx_accel_init(MachineState *ms)
>      }
>  
>      memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
> -    prop.ProcessorCount = smp_cpus;
> +    prop.ProcessorCount = ms->smp.cpus;

I tried to understand how the Windows Hypervisor would answer to an
invalid or zeroed property, but I can't find doc for
WHvPartitionPropertyCodeProcessorCount.

There is a funny comment in VirtualBox although:

 /**
  * @todo Someone at Microsoft please explain another weird API:
  *  - Why this API doesn't take the WHV_PARTITION_PROPERTY_CODE value as an
  *    argument rather than as part of the struct.  That is so weird if
you've
  *    used any other NT or windows API,  including WHvGetCapability().
  *  - Why use PVOID when WHV_PARTITION_PROPERTY is what's expected.  We
  *    technically only need 9 bytes for setting/getting
  *    WHVPartitionPropertyCodeProcessorClFlushSize, but the API insists
on 16. */

https://www.virtualbox.org/svn/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-win.cpp

>      hr = whp_dispatch.WHvSetPartitionProperty(
>          whpx->partition,
>          WHvPartitionPropertyCodeProcessorCount,
> @@ -1405,7 +1405,7 @@ static int whpx_accel_init(MachineState *ms)
>  
>      if (FAILED(hr)) {
>          error_report("WHPX: Failed to set partition core count to %d,"
> -                     " hr=%08lx", smp_cores, hr);
> +                     " hr=%08lx", ms->smp.cores, hr);
>          ret = -EINVAL;
>          goto error;
>      }
> 

Re: [Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX enabled

[Paolo Bonzini]

On 12/07/19 15:26, Stefan Weil wrote:
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  target/i386/whpx-all.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
> index 31d47320e4..ed95105eae 100644
> --- a/target/i386/whpx-all.c
> +++ b/target/i386/whpx-all.c
> @@ -1396,7 +1396,7 @@ static int whpx_accel_init(MachineState *ms)
>      }
>  
>      memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
> -    prop.ProcessorCount = smp_cpus;
> +    prop.ProcessorCount = ms->smp.cpus;
>      hr = whp_dispatch.WHvSetPartitionProperty(
>          whpx->partition,
>          WHvPartitionPropertyCodeProcessorCount,
> @@ -1405,7 +1405,7 @@ static int whpx_accel_init(MachineState *ms)
>  
>      if (FAILED(hr)) {
>          error_report("WHPX: Failed to set partition core count to %d,"
> -                     " hr=%08lx", smp_cores, hr);
> +                     " hr=%08lx", ms->smp.cores, hr);
>          ret = -EINVAL;
>          goto error;
>      }
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX enabled

[Justin Terry (VM) via Qemu-devel]

Thanks Philippe, LGTM (not a maintainer)

FYI here are some docs on WHvSetPartitionProperty method. I know they aren’t comprehensive but they help: https://docs.microsoft.com/en-us/virtualization/api/hypervisor-platform/funcs/whvsetpartitionproperty

Not sure if you are asking for something with the link to the VirtualBox comment but the API allows for setting any of WHV_PARTITION_PROPERTY found at: https://docs.microsoft.com/en-us/virtualization/api/hypervisor-platform/funcs/whvpartitionpropertydatatypes (refer to .h file for most up to date in the sdk matching your build). This is why its not a simple "9 bytes" as indicated below.

Thanks,
Justin

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: Friday, July 12, 2019 7:35 AM
> To: Stefan Weil <sw@weilnetz.de>; qemu-devel@nongnu.org
> Cc: Paolo Bonzini <pbonzini@redhat.com>; Eduardo Habkost
> <ehabkost@redhat.com>; Like Xu <like.xu@linux.intel.com>; Richard
> Henderson <rth@twiddle.net>; Justin Terry (VM) <juterry@microsoft.com>
> Subject: Re: [Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX
> enabled
> 
> Cc'ing Justin
> 
> Maybe we should add a MAINTAINERS section for the WHPX files.
> 
> On 7/12/19 3:26 PM, Stefan Weil wrote:
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> >  target/i386/whpx-all.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c index
> > 31d47320e4..ed95105eae 100644
> > --- a/target/i386/whpx-all.c
> > +++ b/target/i386/whpx-all.c
> > @@ -1396,7 +1396,7 @@ static int whpx_accel_init(MachineState *ms)
> >      }
> >
> >      memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
> > -    prop.ProcessorCount = smp_cpus;
> > +    prop.ProcessorCount = ms->smp.cpus;
> 
> I tried to understand how the Windows Hypervisor would answer to an
> invalid or zeroed property, but I can't find doc for
> WHvPartitionPropertyCodeProcessorCount.
> 
> There is a funny comment in VirtualBox although:
> 
>  /**
>   * @todo Someone at Microsoft please explain another weird API:
>   *  - Why this API doesn't take the WHV_PARTITION_PROPERTY_CODE value
> as an
>   *    argument rather than as part of the struct.  That is so weird if
> you've
>   *    used any other NT or windows API,  including WHvGetCapability().
>   *  - Why use PVOID when WHV_PARTITION_PROPERTY is what's expected.
> We
>   *    technically only need 9 bytes for setting/getting
>   *    WHVPartitionPropertyCodeProcessorClFlushSize, but the API insists
> on 16. */
> 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w.virtualbox.org%2Fsvn%2Fvbox%2Ftrunk%2Fsrc%2FVBox%2FVMM%2FVM
> MR3%2FNEMR3Native-
> win.cpp&amp;data=02%7C01%7Cjuterry%40microsoft.com%7C792c50df03a8
> 4b0343cd08d706d61f24%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%
> 7C636985389026273493&amp;sdata=DftQGby%2BhjojZkXthjxja0zLRfueRzi%2
> F2DUE%2FsI8m5s%3D&amp;reserved=0
> 
> >      hr = whp_dispatch.WHvSetPartitionProperty(
> >          whpx->partition,
> >          WHvPartitionPropertyCodeProcessorCount,
> > @@ -1405,7 +1405,7 @@ static int whpx_accel_init(MachineState *ms)
> >
> >      if (FAILED(hr)) {
> >          error_report("WHPX: Failed to set partition core count to %d,"
> > -                     " hr=%08lx", smp_cores, hr);
> > +                     " hr=%08lx", ms->smp.cores, hr);
> >          ret = -EINVAL;
> >          goto error;
> >      }
> >