| Message ID | 20181213001026.178037-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: vmx: Pass through IA32_TSC_AUX for read iff guest has RDTSCP | expand |
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>
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>
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?
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().
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.
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;