[Qemu-devel] [PATCH] WHPX Add signature CPUID

[Qemu-devel] [PATCH] WHPX Add signature CPUID

[petrutlucian94]

From: Lucian Petrut <lpetrut@cloudbasesolutions.com>

Adds the CPUID trap for CPUID 0x40000000, sending the WHPX signature
to the guest upon request. This is consistent with other QEMU
accelerators (KVM).

Signed-off-by: Alessandro Pilotti <apilotti@cloudbasesolutions.com>
Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
---
As opposed to the previous patch, this one will set the result of
this specific CPUID leaf when initializing the accelerator so that
we avoid a vcpu exit.

 target/i386/whpx-all.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 5843517..c8310de 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -29,6 +29,8 @@
 #include <WinHvPlatform.h>
 #include <WinHvEmulation.h>
 
+#define WHPX_CPUID_SIGNATURE 0x40000000
+
 struct whpx_state {
     uint64_t mem_quota;
     WHV_PARTITION_HANDLE partition;
@@ -1342,6 +1344,27 @@ static int whpx_accel_init(MachineState *ms)
                                  cpuidExitList,
                                  RTL_NUMBER_OF(cpuidExitList) * sizeof(UINT32));
 
+    UINT32 signature[3] = {0};
+    memcpy(signature, "WHPXWHPXWHPX", 12);
+
+    WHV_X64_CPUID_RESULT cpuidResultList[1] = {0};
+    cpuidResultList[0].Function = WHPX_CPUID_SIGNATURE;
+    cpuidResultList[0].Eax = 0;
+    cpuidResultList[0].Ebx = signature[0];
+    cpuidResultList[0].Ecx = signature[1];
+    cpuidResultList[0].Edx = signature[2];
+    hr = WHvSetPartitionProperty(whpx->partition,
+                                 WHvPartitionPropertyCodeCpuidResultList,
+                                 cpuidResultList,
+                                 RTL_NUMBER_OF(cpuidResultList) *
+                                    sizeof(WHV_X64_CPUID_RESULT));
+    if (FAILED(hr)) {
+        error_report("WHPX: Failed to set partition CpuidResultList hr=%08lx",
+                     hr);
+        ret = -EINVAL;
+        goto error;
+    }
+
     if (FAILED(hr)) {
         error_report("WHPX: Failed to set partition CpuidExitList hr=%08lx",
                      hr);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID

[Justin Terry (VM)]

Thanks for the patch Lucian. Looks good to me!

-Justin

> -----Original Message-----
> From: petrutlucian94@gmail.com <petrutlucian94@gmail.com>
> Sent: Tuesday, May 15, 2018 4:38 AM
> Cc: Lucian Petrut <lpetrut@cloudbasesolutions.com>; apilotti
> <apilotti@cloudbasesolutions.com>; Justin Terry (VM)
> <juterry@microsoft.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard
> Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> open list:All patches CC here <qemu-devel@nongnu.org>
> Subject: [PATCH] WHPX Add signature CPUID
> 
> From: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> 
> Adds the CPUID trap for CPUID 0x40000000, sending the WHPX signature to
> the guest upon request. This is consistent with other QEMU accelerators
> (KVM).
> 
> Signed-off-by: Alessandro Pilotti <apilotti@cloudbasesolutions.com>
> Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> ---
> As opposed to the previous patch, this one will set the result of this specific
> CPUID leaf when initializing the accelerator so that we avoid a vcpu exit.
> 
>  target/i386/whpx-all.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c index
> 5843517..c8310de 100644
> --- a/target/i386/whpx-all.c
> +++ b/target/i386/whpx-all.c
> @@ -29,6 +29,8 @@
>  #include <WinHvPlatform.h>
>  #include <WinHvEmulation.h>
> 
> +#define WHPX_CPUID_SIGNATURE 0x40000000
> +
>  struct whpx_state {
>      uint64_t mem_quota;
>      WHV_PARTITION_HANDLE partition;
> @@ -1342,6 +1344,27 @@ static int whpx_accel_init(MachineState *ms)
>                                   cpuidExitList,
>                                   RTL_NUMBER_OF(cpuidExitList) * sizeof(UINT32));
> 
> +    UINT32 signature[3] = {0};
> +    memcpy(signature, "WHPXWHPXWHPX", 12);
> +
> +    WHV_X64_CPUID_RESULT cpuidResultList[1] = {0};
> +    cpuidResultList[0].Function = WHPX_CPUID_SIGNATURE;
> +    cpuidResultList[0].Eax = 0;
> +    cpuidResultList[0].Ebx = signature[0];
> +    cpuidResultList[0].Ecx = signature[1];
> +    cpuidResultList[0].Edx = signature[2];
> +    hr = WHvSetPartitionProperty(whpx->partition,
> +                                 WHvPartitionPropertyCodeCpuidResultList,
> +                                 cpuidResultList,
> +                                 RTL_NUMBER_OF(cpuidResultList) *
> +                                    sizeof(WHV_X64_CPUID_RESULT));
> +    if (FAILED(hr)) {
> +        error_report("WHPX: Failed to set partition CpuidResultList hr=%08lx",
> +                     hr);
> +        ret = -EINVAL;
> +        goto error;
> +    }
> +
>      if (FAILED(hr)) {
>          error_report("WHPX: Failed to set partition CpuidExitList hr=%08lx",
>                       hr);
> --
> 2.7.4

Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID

[Paolo Bonzini]

On 15/05/2018 13:37, petrutlucian94@gmail.com wrote:
> From: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> 
> Adds the CPUID trap for CPUID 0x40000000, sending the WHPX signature
> to the guest upon request. This is consistent with other QEMU
> accelerators (KVM).
> 
> Signed-off-by: Alessandro Pilotti <apilotti@cloudbasesolutions.com>
> Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>

Is it worth defining a different signature?  Can WHPX implement part of
the Hyper-V spec, and if so would it be better to return the Hv
signature ("Hv#1") instead?

Thanks,

Paolo

> ---
> As opposed to the previous patch, this one will set the result of
> this specific CPUID leaf when initializing the accelerator so that
> we avoid a vcpu exit.
> 
>  target/i386/whpx-all.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
> index 5843517..c8310de 100644
> --- a/target/i386/whpx-all.c
> +++ b/target/i386/whpx-all.c
> @@ -29,6 +29,8 @@
>  #include <WinHvPlatform.h>
>  #include <WinHvEmulation.h>
>  
> +#define WHPX_CPUID_SIGNATURE 0x40000000
> +
>  struct whpx_state {
>      uint64_t mem_quota;
>      WHV_PARTITION_HANDLE partition;
> @@ -1342,6 +1344,27 @@ static int whpx_accel_init(MachineState *ms)
>                                   cpuidExitList,
>                                   RTL_NUMBER_OF(cpuidExitList) * sizeof(UINT32));
>  
> +    UINT32 signature[3] = {0};
> +    memcpy(signature, "WHPXWHPXWHPX", 12);
> +
> +    WHV_X64_CPUID_RESULT cpuidResultList[1] = {0};
> +    cpuidResultList[0].Function = WHPX_CPUID_SIGNATURE;
> +    cpuidResultList[0].Eax = 0;
> +    cpuidResultList[0].Ebx = signature[0];
> +    cpuidResultList[0].Ecx = signature[1];
> +    cpuidResultList[0].Edx = signature[2];
> +    hr = WHvSetPartitionProperty(whpx->partition,
> +                                 WHvPartitionPropertyCodeCpuidResultList,
> +                                 cpuidResultList,
> +                                 RTL_NUMBER_OF(cpuidResultList) *
> +                                    sizeof(WHV_X64_CPUID_RESULT));
> +    if (FAILED(hr)) {
> +        error_report("WHPX: Failed to set partition CpuidResultList hr=%08lx",
> +                     hr);
> +        ret = -EINVAL;
> +        goto error;
> +    }
> +
>      if (FAILED(hr)) {
>          error_report("WHPX: Failed to set partition CpuidExitList hr=%08lx",
>                       hr);
> 

Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID

[Alessandro Pilotti]

Hi Paolo,

The main reason for different signatures is to allow guest workloads to be aware of the differences between the two platforms (eg VirtIO vs VMBus).

Thanks,

Alessandro

> On 15 May 2018, at 16:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 15/05/2018 13:37, petrutlucian94@gmail.com wrote:
>> From: Lucian Petrut <lpetrut@cloudbasesolutions.com>
>> 
>> Adds the CPUID trap for CPUID 0x40000000, sending the WHPX signature
>> to the guest upon request. This is consistent with other QEMU
>> accelerators (KVM).
>> 
>> Signed-off-by: Alessandro Pilotti <apilotti@cloudbasesolutions.com>
>> Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
>> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> 
> Is it worth defining a different signature?  Can WHPX implement part of
> the Hyper-V spec, and if so would it be better to return the Hv
> signature ("Hv#1") instead?
> 
> Thanks,
> 
> Paolo
> 
>> ---
>> As opposed to the previous patch, this one will set the result of
>> this specific CPUID leaf when initializing the accelerator so that
>> we avoid a vcpu exit.
>> 
>> target/i386/whpx-all.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>> 
>> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
>> index 5843517..c8310de 100644
>> --- a/target/i386/whpx-all.c
>> +++ b/target/i386/whpx-all.c
>> @@ -29,6 +29,8 @@
>> #include <WinHvPlatform.h>
>> #include <WinHvEmulation.h>
>> 
>> +#define WHPX_CPUID_SIGNATURE 0x40000000
>> +
>> struct whpx_state {
>>     uint64_t mem_quota;
>>     WHV_PARTITION_HANDLE partition;
>> @@ -1342,6 +1344,27 @@ static int whpx_accel_init(MachineState *ms)
>>                                  cpuidExitList,
>>                                  RTL_NUMBER_OF(cpuidExitList) * sizeof(UINT32));
>> 
>> +    UINT32 signature[3] = {0};
>> +    memcpy(signature, "WHPXWHPXWHPX", 12);
>> +
>> +    WHV_X64_CPUID_RESULT cpuidResultList[1] = {0};
>> +    cpuidResultList[0].Function = WHPX_CPUID_SIGNATURE;
>> +    cpuidResultList[0].Eax = 0;
>> +    cpuidResultList[0].Ebx = signature[0];
>> +    cpuidResultList[0].Ecx = signature[1];
>> +    cpuidResultList[0].Edx = signature[2];
>> +    hr = WHvSetPartitionProperty(whpx->partition,
>> +                                 WHvPartitionPropertyCodeCpuidResultList,
>> +                                 cpuidResultList,
>> +                                 RTL_NUMBER_OF(cpuidResultList) *
>> +                                    sizeof(WHV_X64_CPUID_RESULT));
>> +    if (FAILED(hr)) {
>> +        error_report("WHPX: Failed to set partition CpuidResultList hr=%08lx",
>> +                     hr);
>> +        ret = -EINVAL;
>> +        goto error;
>> +    }
>> +
>>     if (FAILED(hr)) {
>>         error_report("WHPX: Failed to set partition CpuidExitList hr=%08lx",
>>                      hr);
>> 
> 

Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID

[Paolo Bonzini]

On 16/05/2018 01:55, Alessandro Pilotti wrote:
> Hi Paolo,
> 
> The main reason for different signatures is to allow guest workloads
> to be aware of the differences between the two platforms (eg VirtIO
> vs VMBus).

Why does it matter?  CPUID tells you about the enlightenments that the
hypervisor provides, not the availability of the VMBus.

VMBus requires some of the enlightenments, mostly related to the
synthetic interrupt controllers, but the opposite is not true---you can
have enlightenments without VMBus, and in fact you probably want WHPX to
enable the relaxed timing enlightenment.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID

[Justin Terry (VM)]

Hey Paolo,

I agree that in the future it would be great if the Windows Hypervisor Platform supported that and if that happens there is no reason to differentiate the two.

However, today WHPX actually doesn’t support any of the synthetic enlightenments that Hyper-V does. We are really trying to make the Windows Hypervisor Platform a generic hypervisor where as Hyper-V is a specific implementation on top of that hypervisor (and QEMU is another such implementation). For example, VMBus was mentioned but as it stands today, VMBus is not supported in WHPX due to lack of support for hypercalls. Additionally, timer enlightenments that you mentioned would not be supported through synthetic paths and instead run emulated as devices (hpet etc.).

I hope this adds some clarity here for why WHPX doesn’t implement hyperv-proto.h as Hyper-V != Windows Hypervisor Platform in many aspects. Please let me know if there is anything else I can explain to help clarify the two.

Thanks,
Justin

> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Wednesday, May 16, 2018 12:35 AM
> To: apilotti <apilotti@cloudbasesolutions.com>
> Cc: petrutlucian94@gmail.com; Lucian Petrut
> <lpetrut@cloudbasesolutions.com>; Eduardo Habkost
> <ehabkost@redhat.com>; open list:All patches CC here <qemu-
> devel@nongnu.org>; Justin Terry (VM) <juterry@microsoft.com>; Richard
> Henderson <rth@twiddle.net>
> Subject: Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID
> 
> On 16/05/2018 01:55, Alessandro Pilotti wrote:
> > Hi Paolo,
> >
> > The main reason for different signatures is to allow guest workloads
> > to be aware of the differences between the two platforms (eg VirtIO vs
> > VMBus).
> 
> Why does it matter?  CPUID tells you about the enlightenments that the
> hypervisor provides, not the availability of the VMBus.
> 
> VMBus requires some of the enlightenments, mostly related to the synthetic
> interrupt controllers, but the opposite is not true---you can have
> enlightenments without VMBus, and in fact you probably want WHPX to
> enable the relaxed timing enlightenment.
> 
> Thanks,
> 
> Paolo

Re: [Qemu-devel] [PATCH] WHPX Add signature CPUID

[Paolo Bonzini]

On 16/05/2018 16:18, Justin Terry (VM) via Qemu-devel wrote:
> Hey Paolo,
> 
> I agree that in the future it would be great if the Windows
> Hypervisor Platform supported that and if that happens there is no
> reason to differentiate the two.
> 
> However, today WHPX actually doesn’t support any of the synthetic
> enlightenments that Hyper-V does. We are really trying to make the
> Windows Hypervisor Platform a generic hypervisor where as Hyper-V is
> a specific implementation on top of that hypervisor (and QEMU is
> another such implementation). For example, VMBus was mentioned but as
> it stands today, VMBus is not supported in WHPX due to lack of
> support for hypercalls. Additionally, timer enlightenments that you
> mentioned would not be supported through synthetic paths and instead
> run emulated as devices (hpet etc.).

Sorry I missed this email.  Note that I didn't mention timer
enlightenments, I mentioned relaxed timing, CPUID[0x40000004].EAX[bit
5].  These are the main reason to implement Hyper-V interfaces in
non-Hyper-V hypervisors, otherwise Windows will bugcheck with STOP 0x101
under load.

The question is why you need to implement a CPUID signature.  The guest
OS should not care about the hypervisor that hosts it, and indeed
Windows works well without detecting Xen, KVM, etc.  It only detects the
Hyper-V interface that is presented by those hypervisors.  Implementing
and documenting a QEMU-specific hypervisor interface has a potentially
large future cost, and it would be preferrable to implement an existing
hypervisor interface for QEMU with WHPX.

> I hope this adds some clarity here for why WHPX doesn’t implement
> hyperv-proto.h as Hyper-V != Windows Hypervisor Platform in many
> aspects. Please let me know if there is anything else I can explain
> to help clarify the two.

Non-Hyper-V hypervisors can provide the Hyper-V interfaces with no
hypercalls and the three synthetic MSRs HV_X64_MSR_GUEST_OS_ID,
HV_X64_MSR_HYPERCALL and HV_X64_MSR_VP_INDEX.

This should not be hard.  For example at least KVM and Xen don't make
the hypercall page an overlay; they just replace the old contents with a
vmcall+ret sequence, which simplifies noticeably the implementation.

Does WHPX allow trapping hypercalls?  If not, a stopgap implementation
could be to put something like this in the hypercall page

    xor eax, eax
    // The next byte sets eax=1 and zf=0 in 32-bit mode
    // In 64-bit mode it is decoded together with the JZ
    db 0x40
    jz is_64_bit
    // Zero edx in 32-bit mode
    cdq
is_64_bit:
    // HV_STATUS_INVALID_HYPERCALL_CODE
    mov al,2
    ret

It would not satisfy the requirement of trapping in real mode or at
CPL=3, and it would clobber the zero flag, but perhaps it's better than
nothing.

Paolo

> 
>> -----Original Message----- From: Paolo Bonzini
>> <pbonzini@redhat.com> Sent: Wednesday, May 16, 2018 12:35 AM To:
>> apilotti <apilotti@cloudbasesolutions.com> Cc:
>> petrutlucian94@gmail.com; Lucian Petrut 
>> <lpetrut@cloudbasesolutions.com>; Eduardo Habkost 
>> <ehabkost@redhat.com>; open list:All patches CC here <qemu- 
>> devel@nongnu.org>; Justin Terry (VM) <juterry@microsoft.com>;
>> Richard Henderson <rth@twiddle.net> Subject: Re: [Qemu-devel]
>> [PATCH] WHPX Add signature CPUID
>> 
>> On 16/05/2018 01:55, Alessandro Pilotti wrote:
>>> Hi Paolo,
>>> 
>>> The main reason for different signatures is to allow guest
>>> workloads to be aware of the differences between the two
>>> platforms (eg VirtIO vs VMBus).
>> 
>> Why does it matter?  CPUID tells you about the enlightenments that
>> the hypervisor provides, not the availability of the VMBus.
>> 
>> VMBus requires some of the enlightenments, mostly related to the
>> synthetic interrupt controllers, but the opposite is not true---you
>> can have enlightenments without VMBus, and in fact you probably
>> want WHPX to enable the relaxed timing enlightenment.
>> 
>> Thanks,
>> 
>> Paolo