* [PATCH 1/5] nSVM: Expose FLUSHBYASID CPUID feature to nested guests
2021-09-20 23:51 [PATCH 0/5] nSVM: Check for optional commands and reserved encodins of TLB_CONTROL in nested guests Krish Sadhukhan
@ 2021-09-20 23:51 ` Krish Sadhukhan
2021-09-24 10:58 ` Paolo Bonzini
2021-09-20 23:51 ` [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB Krish Sadhukhan
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Krish Sadhukhan @ 2021-09-20 23:51 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, vkuznets, wanpengli, joro
The FLUSHBYASID CPUID feature controls the availability of commands
0x3 and 0x7 of TLB_CONTROL. If FLUSHBYASID is supported by the VCPU,
those TLB_CONTROL commands will be available to nested guests. Therefore,
expose FLUSHBYASID CPUID feature to nested guests.
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
arch/x86/kvm/svm/svm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 1a70e11f0487..0f8748af8569 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -931,6 +931,9 @@ static __init void svm_set_cpu_caps(void)
/* Nested VM can receive #VMEXIT instead of triggering #GP */
kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
+
+ if (boot_cpu_has(X86_FEATURE_FLUSHBYASID))
+ kvm_cpu_cap_set(X86_FEATURE_FLUSHBYASID);
}
/* CPUID 0x80000008 */
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/5] nSVM: Expose FLUSHBYASID CPUID feature to nested guests
2021-09-20 23:51 ` [PATCH 1/5] nSVM: Expose FLUSHBYASID CPUID feature to " Krish Sadhukhan
@ 2021-09-24 10:58 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2021-09-24 10:58 UTC (permalink / raw)
To: Krish Sadhukhan, kvm; +Cc: jmattson, seanjc, vkuznets, wanpengli, joro
On 21/09/21 01:51, Krish Sadhukhan wrote:
> The FLUSHBYASID CPUID feature controls the availability of commands
> 0x3 and 0x7 of TLB_CONTROL. If FLUSHBYASID is supported by the VCPU,
> those TLB_CONTROL commands will be available to nested guests.
The hypervisor would have to implement them. Right now the VMCB12
tlb_ctl is not used.
/* Also overwritten later if necessary. */
svm->vmcb->control.tlb_ctl = TLB_CONTROL_DO_NOTHING;
So this won't work.
Paolo
> Therefore,
> expose FLUSHBYASID CPUID feature to nested guests.
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
> arch/x86/kvm/svm/svm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 1a70e11f0487..0f8748af8569 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -931,6 +931,9 @@ static __init void svm_set_cpu_caps(void)
>
> /* Nested VM can receive #VMEXIT instead of triggering #GP */
> kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
> +
> + if (boot_cpu_has(X86_FEATURE_FLUSHBYASID))
> + kvm_cpu_cap_set(X86_FEATURE_FLUSHBYASID);
> }
>
> /* CPUID 0x80000008 */
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB
2021-09-20 23:51 [PATCH 0/5] nSVM: Check for optional commands and reserved encodins of TLB_CONTROL in nested guests Krish Sadhukhan
2021-09-20 23:51 ` [PATCH 1/5] nSVM: Expose FLUSHBYASID CPUID feature to " Krish Sadhukhan
@ 2021-09-20 23:51 ` Krish Sadhukhan
2021-09-28 16:55 ` Paolo Bonzini
2021-09-20 23:51 ` [TEST PATCH 3/5] SVM: Add #defines for valid encodings of TLB_CONTROL VMCB field Krish Sadhukhan
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Krish Sadhukhan @ 2021-09-20 23:51 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, vkuznets, wanpengli, joro
According to section "TLB Flush" in APM vol 2,
"Support for TLB_CONTROL commands other than the first two, is
optional and is indicated by CPUID Fn8000_000A_EDX[FlushByAsid].
All encodings of TLB_CONTROL not defined in the APM are reserved."
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
arch/x86/kvm/svm/nested.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 5e13357da21e..028cc2a1f028 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -235,6 +235,22 @@ static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size)
kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);
}
+static bool nested_svm_check_tlb_ctl(struct kvm_vcpu *vcpu, u8 tlb_ctl)
+{
+ switch(tlb_ctl) {
+ case TLB_CONTROL_DO_NOTHING:
+ case TLB_CONTROL_FLUSH_ALL_ASID:
+ return true;
+ case TLB_CONTROL_FLUSH_ASID:
+ case TLB_CONTROL_FLUSH_ASID_LOCAL:
+ if (guest_cpuid_has(vcpu, X86_FEATURE_FLUSHBYASID))
+ return true;
+ fallthrough;
+ default:
+ return false;
+ }
+}
+
static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
struct vmcb_control_area *control)
{
@@ -254,6 +270,9 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
IOPM_SIZE)))
return false;
+ if (CC(!nested_svm_check_tlb_ctl(vcpu, control->tlb_ctl)))
+ return false;
+
return true;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB
2021-09-20 23:51 ` [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB Krish Sadhukhan
@ 2021-09-28 16:55 ` Paolo Bonzini
2023-09-06 15:59 ` Stefan Sterz
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2021-09-28 16:55 UTC (permalink / raw)
To: Krish Sadhukhan, kvm; +Cc: jmattson, seanjc, vkuznets, wanpengli, joro
On 21/09/21 01:51, Krish Sadhukhan wrote:
> According to section "TLB Flush" in APM vol 2,
>
> "Support for TLB_CONTROL commands other than the first two, is
> optional and is indicated by CPUID Fn8000_000A_EDX[FlushByAsid].
>
> All encodings of TLB_CONTROL not defined in the APM are reserved."
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
> arch/x86/kvm/svm/nested.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 5e13357da21e..028cc2a1f028 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -235,6 +235,22 @@ static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size)
> kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);
> }
>
> +static bool nested_svm_check_tlb_ctl(struct kvm_vcpu *vcpu, u8 tlb_ctl)
> +{
> + switch(tlb_ctl) {
> + case TLB_CONTROL_DO_NOTHING:
> + case TLB_CONTROL_FLUSH_ALL_ASID:
> + return true;
> + case TLB_CONTROL_FLUSH_ASID:
> + case TLB_CONTROL_FLUSH_ASID_LOCAL:
> + if (guest_cpuid_has(vcpu, X86_FEATURE_FLUSHBYASID))
> + return true;
> + fallthrough;
Since nested FLUSHBYASID is not supported yet, this second set of case
labels can go away.
Queued with that change, thanks.
Paolo
> + default:
> + return false;
> + }
> +}
> +
> static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
> struct vmcb_control_area *control)
> {
> @@ -254,6 +270,9 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
> IOPM_SIZE)))
> return false;
>
> + if (CC(!nested_svm_check_tlb_ctl(vcpu, control->tlb_ctl)))
> + return false;
> +
> return true;
> }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB
2021-09-28 16:55 ` Paolo Bonzini
@ 2023-09-06 15:59 ` Stefan Sterz
2023-09-06 20:40 ` Sean Christopherson
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Sterz @ 2023-09-06 15:59 UTC (permalink / raw)
To: Paolo Bonzini, Krish Sadhukhan, kvm
Cc: jmattson, seanjc, vkuznets, wanpengli, joro
On 28.09.21 18:55, Paolo Bonzini wrote:
> On 21/09/21 01:51, Krish Sadhukhan wrote:
>> According to section "TLB Flush" in APM vol 2,
>>
>> "Support for TLB_CONTROL commands other than the first two, is
>> optional and is indicated by CPUID Fn8000_000A_EDX[FlushByAsid].
>>
>> All encodings of TLB_CONTROL not defined in the APM are reserved."
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>> arch/x86/kvm/svm/nested.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 5e13357da21e..028cc2a1f028 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -235,6 +235,22 @@ static bool nested_svm_check_bitmap_pa(struct
>> kvm_vcpu *vcpu, u64 pa, u32 size)
>> kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);
>> }
>> +static bool nested_svm_check_tlb_ctl(struct kvm_vcpu *vcpu
, u8
>> tlb_ctl)
>> +{
>> + switch(tlb_ctl) {
>> + case TLB_CONTROL_DO_NOTHING:
>> + case TLB_CONTROL_FLUSH_ALL_ASID:
>> + return true;
>> + case TLB_CONTROL_FLUSH_ASID:
>> + case TLB_CONTROL_FLUSH_ASID_LOCAL:
>> + if (guest_cpuid_has(vcpu, X86_FEATURE_FLUSHBYASID))
>> + return true;
>> + fallthrough;
>
> Since nested FLUSHBYASID is not supported yet, this second set of case
> labels can go away.
>
> Queued with that change, thanks.
>
> Paolo
>
Are there any plans to support FLUSHBYASID in the future? It seems
VMWare Workstation and ESXi require this feature to run on top of KVM
[1]. This means that after the introduction of this check these VMs fail
to boot and report missing features. Hence, upgrading to a newer kernel
version is not an option for some users.
Sorry if I misunderstood something or if
this is not the right place to
ask.
[1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2008583
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB
2023-09-06 15:59 ` Stefan Sterz
@ 2023-09-06 20:40 ` Sean Christopherson
0 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2023-09-06 20:40 UTC (permalink / raw)
To: Stefan Sterz
Cc: Paolo Bonzini, Krish Sadhukhan, kvm, jmattson, vkuznets, wanpengli, joro
On Wed, Sep 06, 2023, Stefan Sterz wrote:
> On 28.09.21 18:55, Paolo Bonzini wrote:
> > On 21/09/21 01:51, Krish Sadhukhan wrote:
> >> +{
> >> + switch(tlb_ctl) {
> >> + case TLB_CONTROL_DO_NOTHING:
> >> + case TLB_CONTROL_FLUSH_ALL_ASID:
> >> + return true;
> >> + case TLB_CONTROL_FLUSH_ASID:
> >> + case TLB_CONTROL_FLUSH_ASID_LOCAL:
> >> + if (guest_cpuid_has(vcpu, X86_FEATURE_FLUSHBYASID))
> >> + return true;
> >> + fallthrough;
> >
> > Since nested FLUSHBYASID is not supported yet, this second set of case
> > labels can go away.
> >
> > Queued with that change, thanks.
> >
> > Paolo
> >
>
> Are there any plans to support FLUSHBYASID in the future? It seems
> VMWare Workstation and ESXi require this feature to run on top of KVM
> [1]. This means that after the introduction of this check these VMs fail
> to boot and report missing features. Hence, upgrading to a newer kernel
> version is not an option for some users.
IIUC, there's two different issues. KVM "broke" Workstation 16 by adding a bogus
consistency check. And Workstation 17 managed to make things worse by trying to
do the right thing (assert that a feature is supported instead of blindly using it).
I say the above consistency check is bogus because I can't find anything in the APM
that states that TLB_CONTROL is actually checked. Unless something is called out
in "Canonicalization and Consistency Checks" or listed as MBZ (Must Be Zero), AMD
behavior is typically to let software shoot itself in the foot.
So unless I'm missing something, commit 174a921b6975 ("nSVM: Check for reserved
encodings of TLB_CONTROL in nested VMCB") should be reverted.
However, that doesn't help Workstation 17. On the other hand, I don't see how
Workstation 17 could ever have worked on KVM, since KVM has never advertised
FLUSHBYASID to L1.
That said, "supporting" FLUSHBYASID is trivial, KVM just needs to advertise the
bit to userspace. That'll work because KVM's TLB flush "handling" for nSVM is
to just flush everything on every transition (it's been a TODO for a long, long
time).
> Sorry if I misunderstood something or if this is not the right place to ask.
>
> [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2008583
^ permalink raw reply [flat|nested] 10+ messages in thread
* [TEST PATCH 3/5] SVM: Add #defines for valid encodings of TLB_CONTROL VMCB field
2021-09-20 23:51 [PATCH 0/5] nSVM: Check for optional commands and reserved encodins of TLB_CONTROL in nested guests Krish Sadhukhan
2021-09-20 23:51 ` [PATCH 1/5] nSVM: Expose FLUSHBYASID CPUID feature to " Krish Sadhukhan
2021-09-20 23:51 ` [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB Krish Sadhukhan
@ 2021-09-20 23:51 ` Krish Sadhukhan
2021-09-20 23:51 ` [TEST PATCH 4/5] X86: Add #define for FLUSHBYASID CPUID bit Krish Sadhukhan
2021-09-20 23:51 ` [TEST PATCH 5/5] nSVM: Test optional commands and reserved encodings of TLB_CONTROL in nested VMCB Krish Sadhukhan
4 siblings, 0 replies; 10+ messages in thread
From: Krish Sadhukhan @ 2021-09-20 23:51 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, vkuznets, wanpengli, joro
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
x86/svm.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/x86/svm.h b/x86/svm.h
index ae35d08..660956d 100644
--- a/x86/svm.h
+++ b/x86/svm.h
@@ -360,6 +360,11 @@ struct __attribute__ ((__packed__)) vmcb {
#define MSR_BITMAP_SIZE 8192
+#define TLB_CONTROL_DO_NOTHING 0
+#define TLB_CONTROL_FLUSH_ALL_ASID 1
+#define TLB_CONTROL_FLUSH_ASID 3
+#define TLB_CONTROL_FLUSH_ASID_LOCAL 7
+
struct svm_test {
const char *name;
bool (*supported)(void);
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [TEST PATCH 4/5] X86: Add #define for FLUSHBYASID CPUID bit
2021-09-20 23:51 [PATCH 0/5] nSVM: Check for optional commands and reserved encodins of TLB_CONTROL in nested guests Krish Sadhukhan
` (2 preceding siblings ...)
2021-09-20 23:51 ` [TEST PATCH 3/5] SVM: Add #defines for valid encodings of TLB_CONTROL VMCB field Krish Sadhukhan
@ 2021-09-20 23:51 ` Krish Sadhukhan
2021-09-20 23:51 ` [TEST PATCH 5/5] nSVM: Test optional commands and reserved encodings of TLB_CONTROL in nested VMCB Krish Sadhukhan
4 siblings, 0 replies; 10+ messages in thread
From: Krish Sadhukhan @ 2021-09-20 23:51 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, vkuznets, wanpengli, joro
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
lib/x86/processor.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index f380321..9c716de 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -188,6 +188,7 @@ static inline bool is_intel(void)
#define X86_FEATURE_AMD_IBPB (CPUID(0x80000008, 0, EBX, 12))
#define X86_FEATURE_NPT (CPUID(0x8000000A, 0, EDX, 0))
#define X86_FEATURE_NRIPS (CPUID(0x8000000A, 0, EDX, 3))
+#define X86_FEATURE_FLUSHBYASID (CPUID(0x8000000A, 0, EDX, 6))
#define X86_FEATURE_VGIF (CPUID(0x8000000A, 0, EDX, 16))
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [TEST PATCH 5/5] nSVM: Test optional commands and reserved encodings of TLB_CONTROL in nested VMCB
2021-09-20 23:51 [PATCH 0/5] nSVM: Check for optional commands and reserved encodins of TLB_CONTROL in nested guests Krish Sadhukhan
` (3 preceding siblings ...)
2021-09-20 23:51 ` [TEST PATCH 4/5] X86: Add #define for FLUSHBYASID CPUID bit Krish Sadhukhan
@ 2021-09-20 23:51 ` Krish Sadhukhan
4 siblings, 0 replies; 10+ messages in thread
From: Krish Sadhukhan @ 2021-09-20 23:51 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, vkuznets, wanpengli, joro
According to section "TLB Flush" in APM vol 2,
"Support for TLB_CONTROL commands, other than 0x0 and 0x1, is optional
and is indicated by CPUID Fn8000_000A_EDX[FlushByAsid].
All encodings of TLB_CONTROL not defined in the APM are reserved."
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
x86/svm_tests.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index b998b24..fa1bb89 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2562,6 +2562,41 @@ static void guest_rflags_test_db_handler(struct ex_regs *r)
r->rflags &= ~X86_EFLAGS_TF;
}
+/*
+ * Support for TLB_CONTROL commands, other than 0x0 and 0x1, is optional
+ * and is indicated by CPUID Fn8000_000A_EDX[FlushByAsid].
+ * All encodings of TLB_CONTROL not defined in the APM are reserved.
+ */
+static void test_tlb_ctl(void)
+{
+ u64 tlb_ctl_saved = vmcb->control.tlb_ctl;
+ u64 i;
+
+ if (!this_cpu_has(X86_FEATURE_FLUSHBYASID)) {
+ int ret;
+ vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
+ ret = svm_vmrun();
+ report(ret == SVM_EXIT_ERR, "Test TLB_CONTROL: %x, CPU doesn't support FLUSH_ASID (encoding 0x3), VMRUN failure expected",
+ vmcb->control.tlb_ctl);
+
+ vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID_LOCAL;
+ report(svm_vmrun() == SVM_EXIT_ERR, "Test TLB_CONTROL: %x, CPU doesn't support FLUSH_ASID_LOCAL (encoding 0x7), VMRUN failure expected",
+ vmcb->control.tlb_ctl);
+ }
+
+ /*
+ * Test reserved encodings up to 0xf only
+ */
+ for (i = 0; i <= 0xf; i++) {
+ if (i == 0x2 || (i > 3 && i < 7) || i > 7) {
+ vmcb->control.tlb_ctl = i;
+ report(svm_vmrun() == SVM_EXIT_ERR, "Test TLB_CONTROL: %x, reserved encoding used, VMRUN failure expected", vmcb->control.tlb_ctl);
+ }
+ }
+
+ vmcb->control.tlb_ctl = tlb_ctl_saved;
+}
+
static void svm_guest_state_test(void)
{
test_set_guest(basic_guest_main);
@@ -2572,6 +2607,7 @@ static void svm_guest_state_test(void)
test_dr();
test_msrpm_iopm_bitmap_addrs();
test_canonicalization();
+ test_tlb_ctl();
}
extern void guest_rflags_test_guest(struct svm_test *test);
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread