| Message ID | 20210920235134.101970-3-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nSVM: Check for optional commands and reserved encodins of TLB_CONTROL in nested guests | expand |
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; > } > >
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
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
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; }
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(+)