| Message ID | 20210504171734.1434054-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: RDPID/RDTSCP fixes and uret MSR cleanups | expand |
On Tue, May 4, 2021 at 10:17 AM Sean Christopherson <seanjc@google.com> wrote: > > Do not advertise emulation support for RDPID if RDTSCP is unsupported. > RDPID emulation subtly relies on MSR_TSC_AUX to exist in hardware, as > both vmx_get_msr() and svm_get_msr() will return an error if the MSR is > unsupported, i.e. ctxt->ops->get_msr() will fail and the emulator will > inject a #UD. > > Note, RDPID emulation also relies on RDTSCP being enabled in the guest, > but this is a KVM bug and will eventually be fixed. > > Fixes: fb6d4d340e05 ("KVM: x86: emulate RDPID") > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed by: Jim Mattson <jmattson@google.com>
> --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -637,7 +637,8 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) > case 7: > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > entry->eax = 0; > - entry->ecx = F(RDPID); > + if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP)) > + entry->ecx = F(RDPID); > ++array->nent; > default: > break; I'm wondering if entry->ecx should be set to F(RDPID) here even if the CPU supports RDPID natively. (i.e. kvm_cpu_cap_has(X86_FEATURE_RDPID) is true) The document "Documentation/virt/kvm/api.rst" says: --- 4.88 KVM_GET_EMULATED_CPUID --------------------------- <...> Userspace can use the information returned by this ioctl to query which features are emulated by kvm instead of being present natively. --- Thanks, Reiji
On 05/05/21 05:51, Reiji Watanabe wrote: >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -637,7 +637,8 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) >> case 7: >> entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; >> entry->eax = 0; >> - entry->ecx = F(RDPID); >> + if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP)) >> + entry->ecx = F(RDPID); >> ++array->nent; >> default: >> break; > I'm wondering if entry->ecx should be set to F(RDPID) here > even if the CPU supports RDPID natively. > (i.e. kvm_cpu_cap_has(X86_FEATURE_RDPID) is true) > > The document "Documentation/virt/kvm/api.rst" says: > --- > 4.88 KVM_GET_EMULATED_CPUID > --------------------------- > <...> > Userspace can use the information returned by this ioctl to query > which features are emulated by kvm instead of being present natively. > --- Setting it always is consistent with the treatment of MOVBE above. Either way is okay but it should be done for both bits. Paolo
On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > Do not advertise emulation support for RDPID if RDTSCP is unsupported. > RDPID emulation subtly relies on MSR_TSC_AUX to exist in hardware, as > both vmx_get_msr() and svm_get_msr() will return an error if the MSR is > unsupported, i.e. ctxt->ops->get_msr() will fail and the emulator will > inject a #UD. > > Note, RDPID emulation also relies on RDTSCP being enabled in the guest, > but this is a KVM bug and will eventually be fixed. > > Fixes: fb6d4d340e05 ("KVM: x86: emulate RDPID") > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index f765bf7a529c..c96f79c9fff2 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -637,7 +637,8 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) > case 7: > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > entry->eax = 0; > - entry->ecx = F(RDPID); > + if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP)) > + entry->ecx = F(RDPID); > ++array->nent; > default: > break; Just to make sure that I understand this correctly: This is what I know: Both RDTSCP and RDPID are instructions that read IA32_TSC_AUX (and RDTSCP also reads the TSC). Both instructions have their own CPUID bits (X86_FEATURE_RDPID, X86_FEATURE_RDTSCP) If either of these CPUID bits are present, IA32_TSC_AUX should be supported. RDPID is a newer feature, thus I can at least for the sanity sake assume that usually a CPU will either have neither of the features, have only RDTSCP, and IA32_AUX, or have both RDSCP and RDPID. If not supported in hardware KVM only emulates RDPID as I see. Why btw? Performance wise guest that only wants the IA32_AUX in userspace, is better to use RDTSCP and pay the penalty of saving/restoring of the unwanted registers, than use RDPID with a vmexit. My own guess for an answer to this question is that RDPID emulation is there to aid migration from a host that does support RDPID to a host that doesn't. Having said all that, assuming that we don't want to emulate the RDTSCP too, when it is not supported, then this patch does make sense. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Mon, May 10, 2021, Maxim Levitsky wrote: > On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > > Do not advertise emulation support for RDPID if RDTSCP is unsupported. > > RDPID emulation subtly relies on MSR_TSC_AUX to exist in hardware, as > > both vmx_get_msr() and svm_get_msr() will return an error if the MSR is > > unsupported, i.e. ctxt->ops->get_msr() will fail and the emulator will > > inject a #UD. > > > > Note, RDPID emulation also relies on RDTSCP being enabled in the guest, > > but this is a KVM bug and will eventually be fixed. > > > > Fixes: fb6d4d340e05 ("KVM: x86: emulate RDPID") > > Cc: stable@vger.kernel.org > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/cpuid.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index f765bf7a529c..c96f79c9fff2 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -637,7 +637,8 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) > > case 7: > > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > > entry->eax = 0; > > - entry->ecx = F(RDPID); > > + if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP)) > > + entry->ecx = F(RDPID); > > ++array->nent; > > default: > > break; > > Just to make sure that I understand this correctly: > > This is what I know: > > Both RDTSCP and RDPID are instructions that read IA32_TSC_AUX > (and RDTSCP also reads the TSC). > > Both instructions have their own CPUID bits (X86_FEATURE_RDPID, X86_FEATURE_RDTSCP) > If either of these CPUID bits are present, IA32_TSC_AUX should be supported. Yep. > RDPID is a newer feature, thus I can at least for the sanity sake assume that > usually a CPU will either have neither of the features, have only RDTSCP, > and IA32_AUX, or have both RDSCP and RDPID. Yep. > If not supported in hardware KVM only emulates RDPID as I see. Yep. > Why btw? Performance wise guest that only wants the IA32_AUX in userspace, > is better to use RDTSCP and pay the penalty of saving/restoring of the > unwanted registers, than use RDPID with a vmexit. FWIW, Linux doesn't even fall back to RDTSCP. If RDPID isn't supported, Linux throws the info into the limit of a dummy segment in the GDT and uses LSL to get at the data. Turns out that RDTSCP is too slow for its intended use case :-) > My own guess for an answer to this question is that RDPID emulation is there > to aid migration from a host that does support RDPID to a host that doesn't. That's my assumption as well. Paolo's commit is a bit light on why emulation was added in the first place, but emulating to allow migrating to old hardware is the only motivation I can come up with. commit fb6d4d340e0532032c808a9933eaaa7b8de435ab Author: Paolo Bonzini <pbonzini@redhat.com> Date: Tue Jul 12 11:04:26 2016 +0200 KVM: x86: emulate RDPID This is encoded as F3 0F C7 /7 with a register argument. The register argument is the second array in the group9 GroupDual, while F3 is the fourth element of a Prefix. > Having said all that, assuming that we don't want to emulate the RDTSCP too, > when it is not supported, then this patch does make sense. > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > Best regards, > Maxim Levitsky > >
On Mon, 2021-05-10 at 17:20 +0000, Sean Christopherson wrote: > On Mon, May 10, 2021, Maxim Levitsky wrote: > > On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > > > Do not advertise emulation support for RDPID if RDTSCP is unsupported. > > > RDPID emulation subtly relies on MSR_TSC_AUX to exist in hardware, as > > > both vmx_get_msr() and svm_get_msr() will return an error if the MSR is > > > unsupported, i.e. ctxt->ops->get_msr() will fail and the emulator will > > > inject a #UD. > > > > > > Note, RDPID emulation also relies on RDTSCP being enabled in the guest, > > > but this is a KVM bug and will eventually be fixed. > > > > > > Fixes: fb6d4d340e05 ("KVM: x86: emulate RDPID") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kvm/cpuid.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > index f765bf7a529c..c96f79c9fff2 100644 > > > --- a/arch/x86/kvm/cpuid.c > > > +++ b/arch/x86/kvm/cpuid.c > > > @@ -637,7 +637,8 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) > > > case 7: > > > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > > > entry->eax = 0; > > > - entry->ecx = F(RDPID); > > > + if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP)) > > > + entry->ecx = F(RDPID); > > > ++array->nent; > > > default: > > > break; > > > > Just to make sure that I understand this correctly: > > > > This is what I know: > > > > Both RDTSCP and RDPID are instructions that read IA32_TSC_AUX > > (and RDTSCP also reads the TSC). > > > > Both instructions have their own CPUID bits (X86_FEATURE_RDPID, X86_FEATURE_RDTSCP) > > If either of these CPUID bits are present, IA32_TSC_AUX should be supported. > > Yep. > > > RDPID is a newer feature, thus I can at least for the sanity sake assume that > > usually a CPU will either have neither of the features, have only RDTSCP, > > and IA32_AUX, or have both RDSCP and RDPID. > > Yep. > > > If not supported in hardware KVM only emulates RDPID as I see. > > Yep. > > > Why btw? Performance wise guest that only wants the IA32_AUX in userspace, > > is better to use RDTSCP and pay the penalty of saving/restoring of the > > unwanted registers, than use RDPID with a vmexit. > > FWIW, Linux doesn't even fall back to RDTSCP. If RDPID isn't supported, Linux > throws the info into the limit of a dummy segment in the GDT and uses LSL to get > at the data. Turns out that RDTSCP is too slow for its intended use case :-) > > > My own guess for an answer to this question is that RDPID emulation is there > > to aid migration from a host that does support RDPID to a host that doesn't. > > That's my assumption as well. Paolo's commit is a bit light on why emulation > was added in the first place, but emulating to allow migrating to old hardware > is the only motivation I can come up with. Cool thanks! Best regards, Maxim Levitsky > > commit fb6d4d340e0532032c808a9933eaaa7b8de435ab > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Tue Jul 12 11:04:26 2016 +0200 > > KVM: x86: emulate RDPID > > This is encoded as F3 0F C7 /7 with a register argument. The register > argument is the second array in the group9 GroupDual, while F3 is the > fourth element of a Prefix. > > > Having said all that, assuming that we don't want to emulate the RDTSCP too, > > when it is not supported, then this patch does make sense. > > > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > Best regards, > > Maxim Levitsky > > > >
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index f765bf7a529c..c96f79c9fff2 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -637,7 +637,8 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) case 7: entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; entry->eax = 0; - entry->ecx = F(RDPID); + if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP)) + entry->ecx = F(RDPID); ++array->nent; default: break;
Do not advertise emulation support for RDPID if RDTSCP is unsupported. RDPID emulation subtly relies on MSR_TSC_AUX to exist in hardware, as both vmx_get_msr() and svm_get_msr() will return an error if the MSR is unsupported, i.e. ctxt->ops->get_msr() will fail and the emulator will inject a #UD. Note, RDPID emulation also relies on RDTSCP being enabled in the guest, but this is a KVM bug and will eventually be fixed. Fixes: fb6d4d340e05 ("KVM: x86: emulate RDPID") Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)