[PATCH v2] WHPX: support for xcr0

Sunil Muthuswamy posted 1 patch 1 week ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/MW2PR2101MB1116F07C07A26FD7A7ED8DCFC0780@MW2PR2101MB1116.namprd21.prod.outlook.com
Maintainers: Richard Henderson <rth@twiddle.net>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>
target/i386/whp-dispatch.h |  3 ++
target/i386/whpx-all.c     | 88 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 91 insertions(+)

[PATCH v2] WHPX: support for xcr0

Posted by Sunil Muthuswamy 1 week ago
Support for xcr0 to be able to enable xsave/xrstor. This by itself
is not sufficient to enable xsave/xrstor. WHPX XSAVE API's also
needs to be hooked up.

Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
---
You will need the Windows 10 SDK for RS5 (build 17763) or above to
to be able to compile this patch because of the definition of the
XCR0 register.

Changes since v1:
- Added a sign-off line in the patch.

 target/i386/whp-dispatch.h |  3 ++
 target/i386/whpx-all.c     | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/target/i386/whp-dispatch.h b/target/i386/whp-dispatch.h
index 23791fbb47..b5d56b22a3 100644
--- a/target/i386/whp-dispatch.h
+++ b/target/i386/whp-dispatch.h
@@ -6,6 +6,9 @@
 #include <WinHvPlatform.h>
 #include <WinHvEmulation.h>
 
+/* This should eventually come from the Windows SDK */
+#define WHV_E_UNKNOWN_PROPERTY 0x80370302
+
 #define LIST_WINHVPLATFORM_FUNCTIONS(X) \
   X(HRESULT, WHvGetCapability, (WHV_CAPABILITY_CODE CapabilityCode, VOID* CapabilityBuffer, UINT32 CapabilityBufferSizeInBytes, UINT32* WrittenSizeInBytes)) \
   X(HRESULT, WHvCreatePartition, (WHV_PARTITION_HANDLE* Partition)) \
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index ed95105eae..1abaac70db 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -161,10 +161,15 @@ struct whpx_vcpu {
 static bool whpx_allowed;
 static bool whp_dispatch_initialized;
 static HMODULE hWinHvPlatform, hWinHvEmulation;
+static WHV_PROCESSOR_XSAVE_FEATURES whpx_xsave_cap;
 
 struct whpx_state whpx_global;
 struct WHPDispatch whp_dispatch;
 
+static bool whpx_has_xsave(void)
+{
+    return whpx_xsave_cap.XsaveSupport;
+}
 
 /*
  * VP support
@@ -216,6 +221,28 @@ static SegmentCache whpx_seg_h2q(const WHV_X64_SEGMENT_REGISTER *hs)
     return qs;
 }
 
+/* X64 Extended Control Registers */
+static void whpx_set_xcrs(CPUState *cpu)
+{
+    struct CPUX86State *env = (CPUArchState *)(cpu->env_ptr);
+    HRESULT hr;
+    struct whpx_state *whpx = &whpx_global;
+    WHV_REGISTER_VALUE xcr0;
+    WHV_REGISTER_NAME xcr0_name = WHvX64RegisterXCr0;
+
+    if (!whpx_has_xsave()) {
+        return;
+    }
+
+    /* Only xcr0 is supported by the hypervisor currently */
+    xcr0.Reg64 = env->xcr0;
+    hr = whp_dispatch.WHvSetVirtualProcessorRegisters(
+        whpx->partition, cpu->cpu_index, &xcr0_name, 1, &xcr0);
+    if (FAILED(hr)) {
+        error_report("WHPX: Failed to set register xcr0, hr=%08lx", hr);
+    }
+}
+
 static void whpx_set_registers(CPUState *cpu)
 {
     struct whpx_state *whpx = &whpx_global;
@@ -291,6 +318,12 @@ static void whpx_set_registers(CPUState *cpu)
 
     /* 8 Debug Registers - Skipped */
 
+    /*
+     * Extended control registers needs to be handled separately depending
+     * on whether xsave is supported/enabled or not.
+     */
+    whpx_set_xcrs(cpu);
+
     /* 16 XMM registers */
     assert(whpx_register_names[idx] == WHvX64RegisterXmm0);
     idx_next = idx + 16;
@@ -380,6 +413,30 @@ static void whpx_set_registers(CPUState *cpu)
     return;
 }
 
+/* X64 Extended Control Registers */
+static void whpx_get_xcrs(CPUState *cpu)
+{
+    struct CPUX86State *env = (CPUArchState *)(cpu->env_ptr);
+    HRESULT hr;
+    struct whpx_state *whpx = &whpx_global;
+    WHV_REGISTER_VALUE xcr0;
+    WHV_REGISTER_NAME xcr0_name = WHvX64RegisterXCr0;
+
+    if (!whpx_has_xsave()) {
+        return;
+    }
+
+    /* Only xcr0 is supported by the hypervisor currently */
+    hr = whp_dispatch.WHvGetVirtualProcessorRegisters(
+        whpx->partition, cpu->cpu_index, &xcr0_name, 1, &xcr0);
+    if (FAILED(hr)) {
+        error_report("WHPX: Failed to get register xcr0, hr=%08lx", hr);
+        return;
+    }
+
+    env->xcr0 = xcr0.Reg64;
+}
+
 static void whpx_get_registers(CPUState *cpu)
 {
     struct whpx_state *whpx = &whpx_global;
@@ -457,6 +514,12 @@ static void whpx_get_registers(CPUState *cpu)
 
     /* 8 Debug Registers - Skipped */
 
+    /*
+     * Extended control registers needs to be handled separately depending
+     * on whether xsave is supported/enabled or not.
+     */
+    whpx_get_xcrs(cpu);
+
     /* 16 XMM registers */
     assert(whpx_register_names[idx] == WHvX64RegisterXmm0);
     idx_next = idx + 16;
@@ -1395,6 +1458,31 @@ static int whpx_accel_init(MachineState *ms)
         goto error;
     }
 
+    /*
+     * Query the XSAVE capability of the partition. Any error here is not
+     * considered fatal.
+     */
+    hr = whp_dispatch.WHvGetPartitionProperty(
+        whpx->partition,
+        WHvPartitionPropertyCodeProcessorXsaveFeatures,
+        &whpx_xsave_cap,
+        sizeof(whpx_xsave_cap),
+        &whpx_cap_size);
+
+    /*
+     * Windows version which don't support this property will return with the
+     * specific error code.
+     */
+    if (FAILED(hr) && hr != WHV_E_UNKNOWN_PROPERTY) {
+        error_report("WHPX: Failed to query XSAVE capability, hr=%08lx", hr);
+    }
+
+    if (whpx_has_xsave()) {
+        printf("WHPX: Partition XSAVE capable\n");
+    } else {
+        printf("WHPX: Partition is not XSAVE capable\n");
+    }
+
     memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
     prop.ProcessorCount = ms->smp.cpus;
     hr = whp_dispatch.WHvSetPartitionProperty(
-- 
2.16.4


RE: [PATCH v2] WHPX: support for xcr0

Posted by Sunil Muthuswamy 2 days ago

> -----Original Message-----
> From: Sunil Muthuswamy
> Sent: Thursday, November 7, 2019 11:49 AM
> To: Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>
> Cc: qemu-devel@nongnu.org
> Subject: [PATCH v2] WHPX: support for xcr0
> 
> Support for xcr0 to be able to enable xsave/xrstor. This by itself
> is not sufficient to enable xsave/xrstor. WHPX XSAVE API's also
> needs to be hooked up.
> 
> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> ---
> You will need the Windows 10 SDK for RS5 (build 17763) or above to
> to be able to compile this patch because of the definition of the
> XCR0 register.
> 
> Changes since v1:
> - Added a sign-off line in the patch.
> 

Is it possible to get some eyes on this?

Re: [PATCH v2] WHPX: support for xcr0

Posted by Paolo Bonzini 2 days ago
On 12/11/19 19:52, Sunil Muthuswamy wrote:
> 
> 
>> -----Original Message-----
>> From: Sunil Muthuswamy
>> Sent: Thursday, November 7, 2019 11:49 AM
>> To: Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Subject: [PATCH v2] WHPX: support for xcr0
>>
>> Support for xcr0 to be able to enable xsave/xrstor. This by itself
>> is not sufficient to enable xsave/xrstor. WHPX XSAVE API's also
>> needs to be hooked up.
>>
>> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
>> ---
>> You will need the Windows 10 SDK for RS5 (build 17763) or above to
>> to be able to compile this patch because of the definition of the
>> XCR0 register.
>>
>> Changes since v1:
>> - Added a sign-off line in the patch.
>>
> 
> Is it possible to get some eyes on this?
> 

Sure, we're currently in freeze and I was also busy with yesterday's
processor CVEs.  But I will review the patch soon.

Thanks,

Paolo


Re: [PATCH v2] WHPX: support for xcr0

Posted by Stefan Weil 1 week ago
Am 07.11.19 um 20:48 schrieb Sunil Muthuswamy:

> Support for xcr0 to be able to enable xsave/xrstor. This by itself
> is not sufficient to enable xsave/xrstor. WHPX XSAVE API's also
> needs to be hooked up.
>
> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> ---
> You will need the Windows 10 SDK for RS5 (build 17763) or above to
> to be able to compile this patch because of the definition of the
> XCR0 register.
>
> Changes since v1:
> - Added a sign-off line in the patch.


I am not very happy with the current situation which suggests using non
free header files from the Microsoft Windows SDK, thus making it
impossible to produce QEMU executables for Windows with WHPX support
without having legal complications.

Could you please add the required headers with a suitable license to the
QEMU source code? That would clarify the license issue and make builds
with WHPX much easier because those files would not have to be extracted
from a very large SDK installation.

It would also be acceptable if Microsoft could update the license
comments in those files and use a QEMU compatible license.

Kind regards
Stefan Weil




RE: [PATCH v2] WHPX: support for xcr0

Posted by Sunil Muthuswamy 1 week ago
> > You will need the Windows 10 SDK for RS5 (build 17763) or above to
> > to be able to compile this patch because of the definition of the
> > XCR0 register.
> >
> > Changes since v1:
> > - Added a sign-off line in the patch.
> 
> 
> I am not very happy with the current situation which suggests using non
> free header files from the Microsoft Windows SDK, thus making it
> impossible to produce QEMU executables for Windows with WHPX support
> without having legal complications.
> 
> Could you please add the required headers with a suitable license to the
> QEMU source code? That would clarify the license issue and make builds
> with WHPX much easier because those files would not have to be extracted
> from a very large SDK installation.
> 
> It would also be acceptable if Microsoft could update the license
> comments in those files and use a QEMU compatible license.
>
I agree in principle that there should be an easier way to consume the Windows
SDK headers without having to play around with the licenses. I also agree that
that will make life lot easier for many developers. I am reaching out
internally here to see what can be done about this, but, that might take some
time. Meanwhile, is it possible to make some progress on this patch?

> Kind regards
> Stefan Weil
> 
>