* Re: [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU"
2019-05-08 16:08 [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU" Sean Christopherson
@ 2019-05-08 16:55 ` Liran Alon
2019-05-08 17:15 ` Sean Christopherson
2019-05-08 16:57 ` Jim Mattson
2019-05-15 21:37 ` Paolo Bonzini
2 siblings, 1 reply; 7+ messages in thread
From: Liran Alon @ 2019-05-08 16:55 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Radim Krčmář, kvm, David Hill,
Saar Amar, Mihai Carabas, Jim Mattson
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU"
2019-05-08 16:55 ` Liran Alon
@ 2019-05-08 17:15 ` Sean Christopherson
0 siblings, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2019-05-08 17:15 UTC (permalink / raw)
To: Liran Alon
Cc: Paolo Bonzini, Radim Krčmář, kvm, David Hill,
Saar Amar, Mihai Carabas, Jim Mattson
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU"
2019-05-08 16:08 [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU" Sean Christopherson
2019-05-08 16:55 ` Liran Alon
@ 2019-05-08 16:57 ` Jim Mattson
2019-05-08 17:36 ` Sean Christopherson
2019-05-15 21:37 ` Paolo Bonzini
2 siblings, 1 reply; 7+ messages in thread
From: Jim Mattson @ 2019-05-08 16:57 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Radim Krčmář, kvm list, David Hill,
Saar Amar, Mihai Carabas, Liran Alon
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU"
2019-05-08 16:57 ` Jim Mattson
@ 2019-05-08 17:36 ` Sean Christopherson
2019-05-08 19:09 ` Nadav Amit
0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2019-05-08 17:36 UTC (permalink / raw)
To: Jim Mattson
Cc: Paolo Bonzini, Radim Krčmář, kvm list, David Hill,
Saar Amar, Mihai Carabas, Liran Alon
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU"
2019-05-08 17:36 ` Sean Christopherson
@ 2019-05-08 19:09 ` Nadav Amit
0 siblings, 0 replies; 7+ messages in thread
From: Nadav Amit @ 2019-05-08 19:09 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jim Mattson, Paolo Bonzini, Radim Krčmář, kvm list,
David Hill, Saar Amar, Mihai Carabas, Liran Alon
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU"
2019-05-08 16:08 [PATCH] Revert "KVM: nVMX: Expose RDPMC-exiting only when guest supports PMU" Sean Christopherson
2019-05-08 16:55 ` Liran Alon
2019-05-08 16:57 ` Jim Mattson
@ 2019-05-15 21:37 ` Paolo Bonzini
2 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2019-05-15 21:37 UTC (permalink / raw)
To: Sean Christopherson, Radim Krčmář
Cc: kvm, David Hill, Saar Amar, Mihai Carabas, Jim Mattson,
Liran Alon
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
^ permalink raw reply [flat|nested] 7+ messages in thread