* [PATCH] target/i386: Return right size value after dynamic xfeature enabled
@ 2022-03-24 3:18 Yang Zhong
2022-03-24 7:35 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Yang Zhong @ 2022-03-24 3:18 UTC (permalink / raw)
To: qemu-devel; +Cc: yang.zhong, pbonzini
The kvm_arch_get_supported_cpuid() only call KVM_GET_SUPPORTED_CPUID one
time, so the cpuid buffer information still keep older value. Once Qemu
enable new dynamic xfeature, like XTILEDATA, the cpuid[0D,0].{EBX,ECX}
still return older value.
This patch can return right size value in kvm_init_xsave() if XTILEDATA
has been enabled by arch_prctl.
assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
env->xsave_buf_len);
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
target/i386/cpu.h | 3 +++
target/i386/kvm/kvm.c | 15 +++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5e406088a9..814ba4020b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -565,6 +565,9 @@ typedef enum X86Seg {
#define ESA_FEATURE_ALIGN64_MASK (1U << ESA_FEATURE_ALIGN64_BIT)
#define ESA_FEATURE_XFD_MASK (1U << ESA_FEATURE_XFD_BIT)
+#define ARCH_GET_XCOMP_GUEST_PERM 0x1024
+#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
+
/* CPUID feature words */
typedef enum FeatureWord {
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 06901c2a43..312d4fccf8 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -46,6 +46,7 @@
#include "hw/i386/intel_iommu.h"
#include "hw/i386/x86-iommu.h"
#include "hw/i386/e820_memory_layout.h"
+#include "target/i386/cpu.h"
#include "hw/pci/pci.h"
#include "hw/pci/msi.h"
@@ -437,6 +438,18 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
return ret;
}
ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
+ } else if (function == 0xd && index == 0 &&
+ (reg == R_EBX || reg == R_ECX)) {
+ /*
+ * The value returned by KVM_GET_SUPPORTED_CPUID does not include
+ * features that already be enabled with the arch_prctl system call.
+ */
+ int rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
+ if (rc) {
+ warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %d", rc);
+ } else if (bitmask & XSTATE_XTILE_DATA_MASK) {
+ ret += sizeof(XSaveXTILEDATA);
+ }
} else if (function == 0x80000001 && reg == R_ECX) {
/*
* It's safe to enable TOPOEXT even if it's not returned by
@@ -5214,8 +5227,6 @@ bool kvm_arch_cpu_check_are_resettable(void)
return !sev_es_enabled();
}
-#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
-
void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
{
KVMState *s = kvm_state;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] target/i386: Return right size value after dynamic xfeature enabled
2022-03-24 3:18 [PATCH] target/i386: Return right size value after dynamic xfeature enabled Yang Zhong
@ 2022-03-24 7:35 ` Paolo Bonzini
2022-03-24 7:38 ` Yang Zhong
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2022-03-24 7:35 UTC (permalink / raw)
To: Yang Zhong, qemu-devel
On 3/24/22 04:18, Yang Zhong wrote:
> The kvm_arch_get_supported_cpuid() only call KVM_GET_SUPPORTED_CPUID one
> time, so the cpuid buffer information still keep older value. Once Qemu
> enable new dynamic xfeature, like XTILEDATA, the cpuid[0D,0].{EBX,ECX}
> still return older value.
>
> This patch can return right size value in kvm_init_xsave() if XTILEDATA
> has been enabled by arch_prctl.
>
> assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
> env->xsave_buf_len);
>
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
I don't understand, is this a bugfix for an assertion failure or just a
cleanup?
Either way, while I like the idea of modifying
kvm_arch_get_supported_cpuid, I think the right thing to do is to just
use has_xsave2 as the return value if it is nonzero. And then
kvm_init_xsave can just do
if (!has_xsave) {
return;
}
env->xsave_buf_len = kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX);
without the assertion that is now obvious.
Paolo
> ---
> target/i386/cpu.h | 3 +++
> target/i386/kvm/kvm.c | 15 +++++++++++++--
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 5e406088a9..814ba4020b 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -565,6 +565,9 @@ typedef enum X86Seg {
> #define ESA_FEATURE_ALIGN64_MASK (1U << ESA_FEATURE_ALIGN64_BIT)
> #define ESA_FEATURE_XFD_MASK (1U << ESA_FEATURE_XFD_BIT)
>
> +#define ARCH_GET_XCOMP_GUEST_PERM 0x1024
> +#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
> +
>
> /* CPUID feature words */
> typedef enum FeatureWord {
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 06901c2a43..312d4fccf8 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -46,6 +46,7 @@
> #include "hw/i386/intel_iommu.h"
> #include "hw/i386/x86-iommu.h"
> #include "hw/i386/e820_memory_layout.h"
> +#include "target/i386/cpu.h"
>
> #include "hw/pci/pci.h"
> #include "hw/pci/msi.h"
> @@ -437,6 +438,18 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
> return ret;
> }
> ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
> + } else if (function == 0xd && index == 0 &&
> + (reg == R_EBX || reg == R_ECX)) {
> + /*
> + * The value returned by KVM_GET_SUPPORTED_CPUID does not include
> + * features that already be enabled with the arch_prctl system call.
> + */
> + int rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
> + if (rc) {
> + warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %d", rc);
> + } else if (bitmask & XSTATE_XTILE_DATA_MASK) {
> + ret += sizeof(XSaveXTILEDATA);
> + }
> } else if (function == 0x80000001 && reg == R_ECX) {
> /*
> * It's safe to enable TOPOEXT even if it's not returned by
> @@ -5214,8 +5227,6 @@ bool kvm_arch_cpu_check_are_resettable(void)
> return !sev_es_enabled();
> }
>
> -#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
> -
> void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
> {
> KVMState *s = kvm_state;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] target/i386: Return right size value after dynamic xfeature enabled
2022-03-24 7:35 ` Paolo Bonzini
@ 2022-03-24 7:38 ` Yang Zhong
0 siblings, 0 replies; 3+ messages in thread
From: Yang Zhong @ 2022-03-24 7:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: yang.zhong, qemu-devel
On Thu, Mar 24, 2022 at 08:35:10AM +0100, Paolo Bonzini wrote:
> On 3/24/22 04:18, Yang Zhong wrote:
> >The kvm_arch_get_supported_cpuid() only call KVM_GET_SUPPORTED_CPUID one
> >time, so the cpuid buffer information still keep older value. Once Qemu
> >enable new dynamic xfeature, like XTILEDATA, the cpuid[0D,0].{EBX,ECX}
> >still return older value.
> >
> >This patch can return right size value in kvm_init_xsave() if XTILEDATA
> >has been enabled by arch_prctl.
> >
> >assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
> > env->xsave_buf_len);
> >
> >Signed-off-by: Yang Zhong <yang.zhong@intel.com>
>
> I don't understand, is this a bugfix for an assertion failure or
> just a cleanup?
>
In fact, no assert issue here.
The issue is after we enable dynamic xfeature, and if we still use
kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) to get size,
the size is older value(size:2816), not the size(11008) we expected.
The code for cpuid[0D,0].{EBX,ECX} by kvm_arch_get_supported_cpuid()
need cleanup here, or we can't get the real value here. thanks!
Yang
> Either way, while I like the idea of modifying
> kvm_arch_get_supported_cpuid, I think the right thing to do is to
> just use has_xsave2 as the return value if it is nonzero. And then
> kvm_init_xsave can just do
>
> if (!has_xsave) {
> return;
> }
> env->xsave_buf_len = kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX);
>
> without the assertion that is now obvious.
>
> Paolo
>
> >---
> > target/i386/cpu.h | 3 +++
> > target/i386/kvm/kvm.c | 15 +++++++++++++--
> > 2 files changed, 16 insertions(+), 2 deletions(-)
> >
> >diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> >index 5e406088a9..814ba4020b 100644
> >--- a/target/i386/cpu.h
> >+++ b/target/i386/cpu.h
> >@@ -565,6 +565,9 @@ typedef enum X86Seg {
> > #define ESA_FEATURE_ALIGN64_MASK (1U << ESA_FEATURE_ALIGN64_BIT)
> > #define ESA_FEATURE_XFD_MASK (1U << ESA_FEATURE_XFD_BIT)
> >+#define ARCH_GET_XCOMP_GUEST_PERM 0x1024
> >+#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
> >+
> > /* CPUID feature words */
> > typedef enum FeatureWord {
> >diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >index 06901c2a43..312d4fccf8 100644
> >--- a/target/i386/kvm/kvm.c
> >+++ b/target/i386/kvm/kvm.c
> >@@ -46,6 +46,7 @@
> > #include "hw/i386/intel_iommu.h"
> > #include "hw/i386/x86-iommu.h"
> > #include "hw/i386/e820_memory_layout.h"
> >+#include "target/i386/cpu.h"
> > #include "hw/pci/pci.h"
> > #include "hw/pci/msi.h"
> >@@ -437,6 +438,18 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
> > return ret;
> > }
> > ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
> >+ } else if (function == 0xd && index == 0 &&
> >+ (reg == R_EBX || reg == R_ECX)) {
> >+ /*
> >+ * The value returned by KVM_GET_SUPPORTED_CPUID does not include
> >+ * features that already be enabled with the arch_prctl system call.
> >+ */
> >+ int rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
> >+ if (rc) {
> >+ warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %d", rc);
> >+ } else if (bitmask & XSTATE_XTILE_DATA_MASK) {
> >+ ret += sizeof(XSaveXTILEDATA);
> >+ }
> > } else if (function == 0x80000001 && reg == R_ECX) {
> > /*
> > * It's safe to enable TOPOEXT even if it's not returned by
> >@@ -5214,8 +5227,6 @@ bool kvm_arch_cpu_check_are_resettable(void)
> > return !sev_es_enabled();
> > }
> >-#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
> >-
> > void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
> > {
> > KVMState *s = kvm_state;
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-24 7:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 3:18 [PATCH] target/i386: Return right size value after dynamic xfeature enabled Yang Zhong
2022-03-24 7:35 ` Paolo Bonzini
2022-03-24 7:38 ` Yang Zhong
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.