kvm: vmx: Pass through IA32_TSC_AUX for read iff guest has RDTSCP
diff mbox series

Message ID 20181213001026.178037-1-jmattson@google.com
State New
Headers show
Series
  • kvm: vmx: Pass through IA32_TSC_AUX for read iff guest has RDTSCP
Related show

Commit Message

Jim Mattson Dec. 13, 2018, 12:10 a.m. UTC
If the guest supports RDTSCP, it already has read access to the
hardware IA32_TSC_AUX MSR via RDTSCP, so we can allow it read-access
via RDMSR as well. If the guest doesn't support RDTSCP, then we
should not allow it read access to the hardware IA32_TSC_AUX MSR.

Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Marc Orr <marcorr@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---
 arch/x86/kvm/vmx.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Krish Sadhukhan Dec. 14, 2018, 6:48 p.m. UTC | #1
On 12/12/2018 04:10 PM, Jim Mattson wrote:
> If the guest supports RDTSCP, it already has read access to the
> hardware IA32_TSC_AUX MSR via RDTSCP, so we can allow it read-access
> via RDMSR as well. If the guest doesn't support RDTSCP, then we
> should not allow it read access to the hardware IA32_TSC_AUX MSR.
>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Marc Orr <marcorr@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>   arch/x86/kvm/vmx.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c379d0bfdcba9..69deab6f37953 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6522,6 +6522,10 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
>   
>   	if (vmx_rdtscp_supported()) {
>   		bool rdtscp_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP);
> +
> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_TSC_AUX,
> +					  MSR_TYPE_R, !rdtscp_enabled);
> +
>   		if (!rdtscp_enabled)
>   			exec_control &= ~SECONDARY_EXEC_RDTSCP;
>   

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Sean Christopherson Dec. 14, 2018, 7:31 p.m. UTC | #2
On Wed, Dec 12, 2018 at 04:10:26PM -0800, Jim Mattson wrote:
> If the guest supports RDTSCP, it already has read access to the
> hardware IA32_TSC_AUX MSR via RDTSCP, so we can allow it read-access
> via RDMSR as well. If the guest doesn't support RDTSCP, then we
> should not allow it read access to the hardware IA32_TSC_AUX MSR.

Maybe tweak the last sentence?

    Intercept all accesses if the guest doesn't support RDTSCP in order
    to inject #GP, IA32_TSC_AUX exists iff RDTSCP is supported.

> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Marc Orr <marcorr@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Jim Mattson Dec. 14, 2018, 7:42 p.m. UTC | #3
On Fri, Dec 14, 2018 at 11:31 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Dec 12, 2018 at 04:10:26PM -0800, Jim Mattson wrote:
> > If the guest supports RDTSCP, it already has read access to the
> > hardware IA32_TSC_AUX MSR via RDTSCP, so we can allow it read-access
> > via RDMSR as well. If the guest doesn't support RDTSCP, then we
> > should not allow it read access to the hardware IA32_TSC_AUX MSR.
>
> Maybe tweak the last sentence?
>
>     Intercept all accesses if the guest doesn't support RDTSCP in order
>     to inject #GP, IA32_TSC_AUX exists iff RDTSCP is supported.
>
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > Reviewed-by: Marc Orr <marcorr@google.com>
> > Reviewed-by: Peter Shier <pshier@google.com>
>
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

Offline, Krish had a reasonable complaint about where I stuck this bit
of code. Should I do another version with some refactoring, or are
people happy enough with this version?
Sean Christopherson Dec. 14, 2018, 7:50 p.m. UTC | #4
On Fri, Dec 14, 2018 at 11:42:31AM -0800, Jim Mattson wrote:
> On Fri, Dec 14, 2018 at 11:31 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Wed, Dec 12, 2018 at 04:10:26PM -0800, Jim Mattson wrote:
> > > If the guest supports RDTSCP, it already has read access to the
> > > hardware IA32_TSC_AUX MSR via RDTSCP, so we can allow it read-access
> > > via RDMSR as well. If the guest doesn't support RDTSCP, then we
> > > should not allow it read access to the hardware IA32_TSC_AUX MSR.
> >
> > Maybe tweak the last sentence?
> >
> >     Intercept all accesses if the guest doesn't support RDTSCP in order
> >     to inject #GP, IA32_TSC_AUX exists iff RDTSCP is supported.
> >
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > Reviewed-by: Marc Orr <marcorr@google.com>
> > > Reviewed-by: Peter Shier <pshier@google.com>
> >
> > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Offline, Krish had a reasonable complaint about where I stuck this bit
> of code. Should I do another version with some refactoring, or are
> people happy enough with this version?

What's the alternative location?  It seemed a bit odd to me too, but at
the same time there isn't much of a precedent for toggling intercept of
a single MSR based on guest CPUID.

And looking at the code, maybe it only seems odd because we have
vmx_update_msr_bitmap().  What if that and vmx_update_msr_bitmap_x2apic()
were renamed to e.g. vmx_update_intercept_for_x2apic_msrs() and
__vmx_update_intercept_for_x2apic_msrs() so that there isn't a somewhat
bogus implication that updating the bitmaps should only be done in
vmx_update_msr_bitmap().
Krish Sadhukhan Dec. 14, 2018, 9 p.m. UTC | #5
On 12/14/2018 11:50 AM, Sean Christopherson wrote:
> On Fri, Dec 14, 2018 at 11:42:31AM -0800, Jim Mattson wrote:
>> On Fri, Dec 14, 2018 at 11:31 AM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>> On Wed, Dec 12, 2018 at 04:10:26PM -0800, Jim Mattson wrote:
>>>> If the guest supports RDTSCP, it already has read access to the
>>>> hardware IA32_TSC_AUX MSR via RDTSCP, so we can allow it read-access
>>>> via RDMSR as well. If the guest doesn't support RDTSCP, then we
>>>> should not allow it read access to the hardware IA32_TSC_AUX MSR.
>>> Maybe tweak the last sentence?
>>>
>>>      Intercept all accesses if the guest doesn't support RDTSCP in order
>>>      to inject #GP, IA32_TSC_AUX exists iff RDTSCP is supported.
>>>
>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>> Reviewed-by: Marc Orr <marcorr@google.com>
>>>> Reviewed-by: Peter Shier <pshier@google.com>
>>> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Offline, Krish had a reasonable complaint about where I stuck this bit
>> of code. Should I do another version with some refactoring, or are
>> people happy enough with this version?
> What's the alternative location?  It seemed a bit odd to me too, but at
> the same time there isn't much of a precedent for toggling intercept of
> a single MSR based on guest CPUID.
>
> And looking at the code, maybe it only seems odd because we have
> vmx_update_msr_bitmap().  What if that and vmx_update_msr_bitmap_x2apic()
> were renamed to e.g. vmx_update_intercept_for_x2apic_msrs() and
> __vmx_update_intercept_for_x2apic_msrs() so that there isn't a somewhat
> bogus implication that updating the bitmaps should only be done in
> vmx_update_msr_bitmap().

The other possibility is to create a new function, say, 
vmx_cpuid_update_msr_bitmap(), which will be called in the CPUID update 
path, thereby leaving vmx_update_msr_bitmap() as is.

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c379d0bfdcba9..69deab6f37953 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6522,6 +6522,10 @@  static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
 
 	if (vmx_rdtscp_supported()) {
 		bool rdtscp_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP);
+
+		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_TSC_AUX,
+					  MSR_TYPE_R, !rdtscp_enabled);
+
 		if (!rdtscp_enabled)
 			exec_control &= ~SECONDARY_EXEC_RDTSCP;