| Message ID | 20190508160819.19603-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU" | expand |
> On 8 May 2019, at 19:08, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > The RDPMC-exiting control is dependent on the existence of the RDPMC > instruction itself, i.e. is not tied to the "Architectural Performance > Monitoring" feature. For all intents and purposes, the control exists > on all CPUs with VMX support since RDPMC also exists on all VCPUs with > VMX supported. Per Intel's SDM: > > The RDPMC instruction was introduced into the IA-32 Architecture in > the Pentium Pro processor and the Pentium processor with MMX technology. > The earlier Pentium processors have performance-monitoring counters, but > they must be read with the RDMSR instruction. > > Because RDPMC-exiting always exists, KVM requires the control and refuses > to load if it's not available. As a result, hiding the PMU from a guest > breaks nested virtualization if the guest attemts to use KVM. > If I understand correctly, you mean that there were CPUs at the past that had performance-counters but without PMU and could have been read by RDMSR instead of RDPMC? And there is no CPUID bit that expose if performance-counters even exists? You just need to try to RDPMC and see if it #GP? If the answer to all above questions is “yes” to all questions above then I’m sorry for my misunderstanding with this original commit and: Reviewed-by: Liran Alon <liran.alon@oracle.com> > While it's not explicitly stated in the RDPMC pseudocode, the VM-Exit > check for RDPMC-exiting follows standard fault vs. VM-Exit prioritization > for privileged instructions, e.g. occurs after the CPL/CR0.PE/CR4.PCE > checks, but before the counter referenced in ECX is checked for validity. > > In other words, the original KVM behavior of injecting a #GP was correct, > and the KVM unit test needs to be adjusted accordingly, e.g. eat the #GP > when the unit test guest (L3 in this case) executes RDPMC without > RDPMC-exiting set in the unit test host (L2). > > This reverts commit e51bfdb68725dc052d16241ace40ea3140f938aa. > > Fixes: e51bfdb68725 ("KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU") > Reported-by: David Hill <hilld@binarystorm.net> > Cc: Saar Amar <saaramar@microsoft.com> > Cc: Mihai Carabas <mihai.carabas@oracle.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: Liran Alon <liran.alon@oracle.com> > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 25 ------------------------- > 1 file changed, 25 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 60306f19105d..0db7ded18951 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6866,30 +6866,6 @@ static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu) > } > } > > -static bool guest_cpuid_has_pmu(struct kvm_vcpu *vcpu) > -{ > - struct kvm_cpuid_entry2 *entry; > - union cpuid10_eax eax; > - > - entry = kvm_find_cpuid_entry(vcpu, 0xa, 0); > - if (!entry) > - return false; > - > - eax.full = entry->eax; > - return (eax.split.version_id > 0); > -} > - > -static void nested_vmx_procbased_ctls_update(struct kvm_vcpu *vcpu) > -{ > - struct vcpu_vmx *vmx = to_vmx(vcpu); > - bool pmu_enabled = guest_cpuid_has_pmu(vcpu); > - > - if (pmu_enabled) > - vmx->nested.msrs.procbased_ctls_high |= CPU_BASED_RDPMC_EXITING; > - else > - vmx->nested.msrs.procbased_ctls_high &= ~CPU_BASED_RDPMC_EXITING; > -} > - > static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -6978,7 +6954,6 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > if (nested_vmx_allowed(vcpu)) { > nested_vmx_cr_fixed1_bits_update(vcpu); > nested_vmx_entry_exit_ctls_update(vcpu); > - nested_vmx_procbased_ctls_update(vcpu); > } > > if (boot_cpu_has(X86_FEATURE_INTEL_PT) && > -- > 2.21.0 >
On Wed, May 8, 2019 at 9:08 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > The RDPMC-exiting control is dependent on the existence of the RDPMC > instruction itself, i.e. is not tied to the "Architectural Performance > Monitoring" feature. For all intents and purposes, the control exists > on all CPUs with VMX support since RDPMC also exists on all VCPUs with > VMX supported. Per Intel's SDM: > > The RDPMC instruction was introduced into the IA-32 Architecture in > the Pentium Pro processor and the Pentium processor with MMX technology. > The earlier Pentium processors have performance-monitoring counters, but > they must be read with the RDMSR instruction. > > Because RDPMC-exiting always exists, KVM requires the control and refuses > to load if it's not available. As a result, hiding the PMU from a guest > breaks nested virtualization if the guest attemts to use KVM. Is it true that the existence of instruction <X> implies the availaibility of the VM-execution control <X>-exiting (if such a VM-execution control exists)? What about WBINVD? That instruction has certainly been around forever, but there were VMX-capable processors that did not support WBINVD-exiting. Having said that, I think our hands are tied by the assumptions made by existing hypervisors, whether or not those assumptions are true. (VMware's VMM, for instance, requires MONITOR-exiting and MWAIT-exiting even when MONITOR/MWAIT are not enumerated by CPUID.) > While it's not explicitly stated in the RDPMC pseudocode, the VM-Exit > check for RDPMC-exiting follows standard fault vs. VM-Exit prioritization > for privileged instructions, e.g. occurs after the CPL/CR0.PE/CR4.PCE > checks, but before the counter referenced in ECX is checked for validity. > > In other words, the original KVM behavior of injecting a #GP was correct, > and the KVM unit test needs to be adjusted accordingly, e.g. eat the #GP > when the unit test guest (L3 in this case) executes RDPMC without > RDPMC-exiting set in the unit test host (L2). > > This reverts commit e51bfdb68725dc052d16241ace40ea3140f938aa. > > Fixes: e51bfdb68725 ("KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU") > Reported-by: David Hill <hilld@binarystorm.net> > Cc: Saar Amar <saaramar@microsoft.com> > Cc: Mihai Carabas <mihai.carabas@oracle.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: Liran Alon <liran.alon@oracle.com> > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Reviewed-by: Jim Mattson <jmattson@google.com>
On Wed, May 08, 2019 at 07:55:13PM +0300, Liran Alon wrote: > > > > On 8 May 2019, at 19:08, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > The RDPMC-exiting control is dependent on the existence of the RDPMC > > instruction itself, i.e. is not tied to the "Architectural Performance > > Monitoring" feature. For all intents and purposes, the control exists > > on all CPUs with VMX support since RDPMC also exists on all VCPUs with > > VMX supported. Per Intel's SDM: > > > > The RDPMC instruction was introduced into the IA-32 Architecture in > > the Pentium Pro processor and the Pentium processor with MMX technology. > > The earlier Pentium processors have performance-monitoring counters, but > > they must be read with the RDMSR instruction. > > > > Because RDPMC-exiting always exists, KVM requires the control and refuses > > to load if it's not available. As a result, hiding the PMU from a guest > > breaks nested virtualization if the guest attemts to use KVM. > > > > If I understand correctly, you mean that there were CPUs at the past that had > performance-counters but without PMU and could have been read by RDMSR > instead of RDPMC? That's a true statement, but not what I meant. To try and reword, there are CPUs that have a PMU and RDPMC, and thus RDPMC-exiting, but do NOT report their PMU capabilities via CPUID 0xA. The kernel code to init the PMU is probably the best example (X86_FEATURE_ARCH_PERFMON is effectively a reflection of the existence of CPUID 0xA). __init int intel_pmu_init(void) { if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) { switch (boot_cpu_data.x86) { case 0x6: return p6_pmu_init(); case 0xb: return knc_pmu_init(); case 0xf: return p4_pmu_init(); } return -ENODEV; } } > And there is no CPUID bit that expose if performance-counters even exists? > You just need to try to RDPMC and see if it #GP? AFAIK the non-architectural perfmons aren't enumerated via CPUID. I'm pretty most of them don't have any enumeration beyond the CPU model, i.e. software needs to essentially hardcode support based on the CPU model and maybe some topology info. So from a unit test perspective, the most compatible approach is to just eat the #GP, but I think it's also completely reasonable to test RDPMC without interception if and only if architectural perfmons are supported. > If the answer to all above questions is “yes” to all questions above then I’m > sorry for my misunderstanding with this original commit and: Reviewed-by: > Liran Alon <liran.alon@oracle.com>
On Wed, May 08, 2019 at 09:57:11AM -0700, Jim Mattson wrote: > On Wed, May 8, 2019 at 9:08 AM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > The RDPMC-exiting control is dependent on the existence of the RDPMC > > instruction itself, i.e. is not tied to the "Architectural Performance > > Monitoring" feature. For all intents and purposes, the control exists > > on all CPUs with VMX support since RDPMC also exists on all VCPUs with > > VMX supported. Per Intel's SDM: > > > > The RDPMC instruction was introduced into the IA-32 Architecture in > > the Pentium Pro processor and the Pentium processor with MMX technology. > > The earlier Pentium processors have performance-monitoring counters, but > > they must be read with the RDMSR instruction. > > > > Because RDPMC-exiting always exists, KVM requires the control and refuses > > to load if it's not available. As a result, hiding the PMU from a guest > > breaks nested virtualization if the guest attemts to use KVM. > > Is it true that the existence of instruction <X> implies the > availaibility of the VM-execution control <X>-exiting (if such a > VM-execution control exists)? What about WBINVD? That instruction has > certainly been around forever, but there were VMX-capable processors > that did not support WBINVD-exiting. Technically no, but 99% of the time yes. It's kind of similar to KVM's live migration requirements: new features with "dangerous" instructions need an associated VMCS control, but there are some legacy cases where a VMCS control was added after the fact, WBINVD being the obvious example. > Having said that, I think our hands are tied by the assumptions made > by existing hypervisors, whether or not those assumptions are true. > (VMware's VMM, for instance, requires MONITOR-exiting and > MWAIT-exiting even when MONITOR/MWAIT are not enumerated by CPUID.) I'd say it's more of a requirement than an assumption, e.g. KVM *requires* RDPMC-exiting so that the guest can't glean info about the host. I guess technically KVM is assuming RDPMC itself exists, but it's existence is effectively guaranteed by the SDM. I can't speak to the VMWare behavior, e.g. it might be nothing more than a simple oversight that isn't worth fixing, or maybe it's paranoid and really wants to ensure the guest can't execute MONITOR/MWAIT :-) > > While it's not explicitly stated in the RDPMC pseudocode, the VM-Exit > > check for RDPMC-exiting follows standard fault vs. VM-Exit prioritization > > for privileged instructions, e.g. occurs after the CPL/CR0.PE/CR4.PCE > > checks, but before the counter referenced in ECX is checked for validity. > > > > In other words, the original KVM behavior of injecting a #GP was correct, > > and the KVM unit test needs to be adjusted accordingly, e.g. eat the #GP > > when the unit test guest (L3 in this case) executes RDPMC without > > RDPMC-exiting set in the unit test host (L2). > > > > This reverts commit e51bfdb68725dc052d16241ace40ea3140f938aa. > > > > Fixes: e51bfdb68725 ("KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU") > > Reported-by: David Hill <hilld@binarystorm.net> > > Cc: Saar Amar <saaramar@microsoft.com> > > Cc: Mihai Carabas <mihai.carabas@oracle.com> > > Cc: Jim Mattson <jmattson@google.com> > > Cc: Liran Alon <liran.alon@oracle.com> > > Cc: stable@vger.kernel.org > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Reviewed-by: Jim Mattson <jmattson@google.com>
> On May 8, 2019, at 10:36 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Wed, May 08, 2019 at 09:57:11AM -0700, Jim Mattson wrote: >> On Wed, May 8, 2019 at 9:08 AM Sean Christopherson >> <sean.j.christopherson@intel.com> wrote: >>> The RDPMC-exiting control is dependent on the existence of the RDPMC >>> instruction itself, i.e. is not tied to the "Architectural Performance >>> Monitoring" feature. For all intents and purposes, the control exists >>> on all CPUs with VMX support since RDPMC also exists on all VCPUs with >>> VMX supported. Per Intel's SDM: >>> >>> The RDPMC instruction was introduced into the IA-32 Architecture in >>> the Pentium Pro processor and the Pentium processor with MMX technology. >>> The earlier Pentium processors have performance-monitoring counters, but >>> they must be read with the RDMSR instruction. >>> >>> Because RDPMC-exiting always exists, KVM requires the control and refuses >>> to load if it's not available. As a result, hiding the PMU from a guest >>> breaks nested virtualization if the guest attemts to use KVM. >> >> Is it true that the existence of instruction <X> implies the >> availaibility of the VM-execution control <X>-exiting (if such a >> VM-execution control exists)? What about WBINVD? That instruction has >> certainly been around forever, but there were VMX-capable processors >> that did not support WBINVD-exiting. > > Technically no, but 99% of the time yes. It's kind of similar to KVM's > live migration requirements: new features with "dangerous" instructions > need an associated VMCS control, but there are some legacy cases where > a VMCS control was added after the fact, WBINVD being the obvious example. > >> Having said that, I think our hands are tied by the assumptions made >> by existing hypervisors, whether or not those assumptions are true. >> (VMware's VMM, for instance, requires MONITOR-exiting and >> MWAIT-exiting even when MONITOR/MWAIT are not enumerated by CPUID.) > > I'd say it's more of a requirement than an assumption, e.g. KVM > *requires* RDPMC-exiting so that the guest can't glean info about the > host. I guess technically KVM is assuming RDPMC itself exists, but > it's existence is effectively guaranteed by the SDM. > > I can't speak to the VMWare behavior, e.g. it might be nothing more > than a simple oversight that isn't worth fixing, or maybe it's paranoid > and really wants to ensure the guest can't execute MONITOR/MWAIT :-) I am sure Jim is more knowledgable than I am to talk about the reasons for VMware behavior. But I would send somewhen later a patch for kvm-unit-tests/vmx, since they assume the MONITOR/MWAIT are supported if the execution-control is supported. This is, as Jim indicated, not true on VMware.
On 08/05/19 18:08, Sean Christopherson wrote: > The RDPMC-exiting control is dependent on the existence of the RDPMC > instruction itself, i.e. is not tied to the "Architectural Performance > Monitoring" feature. For all intents and purposes, the control exists > on all CPUs with VMX support since RDPMC also exists on all VCPUs with > VMX supported. Per Intel's SDM: > > The RDPMC instruction was introduced into the IA-32 Architecture in > the Pentium Pro processor and the Pentium processor with MMX technology. > The earlier Pentium processors have performance-monitoring counters, but > they must be read with the RDMSR instruction. > > Because RDPMC-exiting always exists, KVM requires the control and refuses > to load if it's not available. As a result, hiding the PMU from a guest > breaks nested virtualization if the guest attemts to use KVM. > > While it's not explicitly stated in the RDPMC pseudocode, the VM-Exit > check for RDPMC-exiting follows standard fault vs. VM-Exit prioritization > for privileged instructions, e.g. occurs after the CPL/CR0.PE/CR4.PCE > checks, but before the counter referenced in ECX is checked for validity. > > In other words, the original KVM behavior of injecting a #GP was correct, > and the KVM unit test needs to be adjusted accordingly, e.g. eat the #GP > when the unit test guest (L3 in this case) executes RDPMC without > RDPMC-exiting set in the unit test host (L2). > > This reverts commit e51bfdb68725dc052d16241ace40ea3140f938aa. > > Fixes: e51bfdb68725 ("KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU") > Reported-by: David Hill <hilld@binarystorm.net> > Cc: Saar Amar <saaramar@microsoft.com> > Cc: Mihai Carabas <mihai.carabas@oracle.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: Liran Alon <liran.alon@oracle.com> > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 25 ------------------------- > 1 file changed, 25 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 60306f19105d..0db7ded18951 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6866,30 +6866,6 @@ static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu) > } > } > > -static bool guest_cpuid_has_pmu(struct kvm_vcpu *vcpu) > -{ > - struct kvm_cpuid_entry2 *entry; > - union cpuid10_eax eax; > - > - entry = kvm_find_cpuid_entry(vcpu, 0xa, 0); > - if (!entry) > - return false; > - > - eax.full = entry->eax; > - return (eax.split.version_id > 0); > -} > - > -static void nested_vmx_procbased_ctls_update(struct kvm_vcpu *vcpu) > -{ > - struct vcpu_vmx *vmx = to_vmx(vcpu); > - bool pmu_enabled = guest_cpuid_has_pmu(vcpu); > - > - if (pmu_enabled) > - vmx->nested.msrs.procbased_ctls_high |= CPU_BASED_RDPMC_EXITING; > - else > - vmx->nested.msrs.procbased_ctls_high &= ~CPU_BASED_RDPMC_EXITING; > -} > - > static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -6978,7 +6954,6 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > if (nested_vmx_allowed(vcpu)) { > nested_vmx_cr_fixed1_bits_update(vcpu); > nested_vmx_entry_exit_ctls_update(vcpu); > - nested_vmx_procbased_ctls_update(vcpu); > } > > if (boot_cpu_has(X86_FEATURE_INTEL_PT) && > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 60306f19105d..0db7ded18951 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6866,30 +6866,6 @@ static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu) } } -static bool guest_cpuid_has_pmu(struct kvm_vcpu *vcpu) -{ - struct kvm_cpuid_entry2 *entry; - union cpuid10_eax eax; - - entry = kvm_find_cpuid_entry(vcpu, 0xa, 0); - if (!entry) - return false; - - eax.full = entry->eax; - return (eax.split.version_id > 0); -} - -static void nested_vmx_procbased_ctls_update(struct kvm_vcpu *vcpu) -{ - struct vcpu_vmx *vmx = to_vmx(vcpu); - bool pmu_enabled = guest_cpuid_has_pmu(vcpu); - - if (pmu_enabled) - vmx->nested.msrs.procbased_ctls_high |= CPU_BASED_RDPMC_EXITING; - else - vmx->nested.msrs.procbased_ctls_high &= ~CPU_BASED_RDPMC_EXITING; -} - static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) { struct vcpu_vmx *vmx = to_vmx(vcpu); @@ -6978,7 +6954,6 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) if (nested_vmx_allowed(vcpu)) { nested_vmx_cr_fixed1_bits_update(vcpu); nested_vmx_entry_exit_ctls_update(vcpu); - nested_vmx_procbased_ctls_update(vcpu); } if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
The RDPMC-exiting control is dependent on the existence of the RDPMC instruction itself, i.e. is not tied to the "Architectural Performance Monitoring" feature. For all intents and purposes, the control exists on all CPUs with VMX support since RDPMC also exists on all VCPUs with VMX supported. Per Intel's SDM: The RDPMC instruction was introduced into the IA-32 Architecture in the Pentium Pro processor and the Pentium processor with MMX technology. The earlier Pentium processors have performance-monitoring counters, but they must be read with the RDMSR instruction. Because RDPMC-exiting always exists, KVM requires the control and refuses to load if it's not available. As a result, hiding the PMU from a guest breaks nested virtualization if the guest attemts to use KVM. While it's not explicitly stated in the RDPMC pseudocode, the VM-Exit check for RDPMC-exiting follows standard fault vs. VM-Exit prioritization for privileged instructions, e.g. occurs after the CPL/CR0.PE/CR4.PCE checks, but before the counter referenced in ECX is checked for validity. In other words, the original KVM behavior of injecting a #GP was correct, and the KVM unit test needs to be adjusted accordingly, e.g. eat the #GP when the unit test guest (L3 in this case) executes RDPMC without RDPMC-exiting set in the unit test host (L2). This reverts commit e51bfdb68725dc052d16241ace40ea3140f938aa. Fixes: e51bfdb68725 ("KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU") Reported-by: David Hill <hilld@binarystorm.net> Cc: Saar Amar <saaramar@microsoft.com> Cc: Mihai Carabas <mihai.carabas@oracle.com> Cc: Jim Mattson <jmattson@google.com> Cc: Liran Alon <liran.alon@oracle.com> Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/vmx.c | 25 ------------------------- 1 file changed, 25 deletions(-)