Patchwork [v4,2/5] kvm: vmx: Document the need for MSR_STAR in i386 builds

login
register
mail settings
Submitter Christopherson, Sean J
Date Dec. 6, 2018, 5:02 p.m.
Message ID <20181206170215.GE31263@linux.intel.com>
Download mbox | patch
Permalink /patch/674389/
State New
Headers show

Comments

Christopherson, Sean J - Dec. 6, 2018, 5:02 p.m.
On Wed, Dec 05, 2018 at 03:28:59PM -0800, Jim Mattson wrote:
> Add a comment explaining why MSR_STAR must be included in
> vmx_msr_index[] even for i386 builds.
> 
> The elided comment has not been relevant since move_msr_up() was
> introduced in commit a75beee6e4f5d ("KVM: VMX: Avoid saving and
> restoring msrs on lightweight vmexit").
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>  arch/x86/kvm/vmx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ad1753e8a85e5..b58c3952c5dfb 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1390,8 +1390,11 @@ static u64 host_efer;
>  static void ept_save_pdptrs(struct kvm_vcpu *vcpu);
>  
>  /*
> - * Keep MSR_STAR at the end, as setup_msrs() will try to optimize it
> - * away by decrementing the array size.
> + * Though SYSCALL is only supported in 64-bit mode on Intel CPUs, kvm
> + * will emulate SYSCALL in legacy mode if the vendor string in guest
> + * CPUID.0:{EBX,ECX,EDX} is "AuthenticAMD" or "AMDisbetter!" To
> + * support this emulation, IA32_STAR must always be included in
> + * vmx_msr_index[], even in i386 builds.
>   */

Hmm, so this isn't wrong per se, but relying on the detection of hardware
MSRs for software emulation is weird, e.g. vmx_vcpu_setup() will only
"find" MSR_STAR if it exists in hardware even though an i386 build will
never actually touch the hardware MSR.  This also applies to MSR_CSTAR in
the next patch (for all builds).

What about making vmx_msr_index an array of structs and adding a flag to
denote an emulation-only MSR?  That'd document why we keep certain MSRs
around and would also avoid probing them in vmx_vcpu_setup(), e.g. I'm
assuming MSR_EFER is kept in i386 build for emulation too.

I'd also vote to remove the comment above MSR_STAR in setup_msrs().

E.g. (not complete and obviously untested):

---
 arch/x86/kvm/vmx.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)
Jim Mattson - Dec. 6, 2018, 8:02 p.m.
On Thu, Dec 6, 2018 at 9:02 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Dec 05, 2018 at 03:28:59PM -0800, Jim Mattson wrote:
> > Add a comment explaining why MSR_STAR must be included in
> > vmx_msr_index[] even for i386 builds.
> >
> > The elided comment has not been relevant since move_msr_up() was
> > introduced in commit a75beee6e4f5d ("KVM: VMX: Avoid saving and
> > restoring msrs on lightweight vmexit").
> >
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > Reviewed-by: Peter Shier <pshier@google.com>
> > ---
> >  arch/x86/kvm/vmx.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index ad1753e8a85e5..b58c3952c5dfb 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -1390,8 +1390,11 @@ static u64 host_efer;
> >  static void ept_save_pdptrs(struct kvm_vcpu *vcpu);
> >
> >  /*
> > - * Keep MSR_STAR at the end, as setup_msrs() will try to optimize it
> > - * away by decrementing the array size.
> > + * Though SYSCALL is only supported in 64-bit mode on Intel CPUs, kvm
> > + * will emulate SYSCALL in legacy mode if the vendor string in guest
> > + * CPUID.0:{EBX,ECX,EDX} is "AuthenticAMD" or "AMDisbetter!" To
> > + * support this emulation, IA32_STAR must always be included in
> > + * vmx_msr_index[], even in i386 builds.
> >   */
>
> Hmm, so this isn't wrong per se, but relying on the detection of hardware
> MSRs for software emulation is weird, e.g. vmx_vcpu_setup() will only
> "find" MSR_STAR if it exists in hardware even though an i386 build will
> never actually touch the hardware MSR.  This also applies to MSR_CSTAR in
> the next patch (for all builds).
>
> What about making vmx_msr_index an array of structs and adding a flag to
> denote an emulation-only MSR?  That'd document why we keep certain MSRs
> around and would also avoid probing them in vmx_vcpu_setup(), e.g. I'm
> assuming MSR_EFER is kept in i386 build for emulation too.

The management of this array seems overly convoluted already. Let me
see if I can get the existing patch set accepted first, and then maybe
we can come up with something just a little less baroque.
Christopherson, Sean J - Dec. 6, 2018, 8:05 p.m.
On Thu, Dec 06, 2018 at 12:02:10PM -0800, Jim Mattson wrote:
> On Thu, Dec 6, 2018 at 9:02 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Wed, Dec 05, 2018 at 03:28:59PM -0800, Jim Mattson wrote:
> > > Add a comment explaining why MSR_STAR must be included in
> > > vmx_msr_index[] even for i386 builds.
> > >
> > > The elided comment has not been relevant since move_msr_up() was
> > > introduced in commit a75beee6e4f5d ("KVM: VMX: Avoid saving and
> > > restoring msrs on lightweight vmexit").
> > >
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > Reviewed-by: Peter Shier <pshier@google.com>
> > > ---
> > >  arch/x86/kvm/vmx.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > index ad1753e8a85e5..b58c3952c5dfb 100644
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -1390,8 +1390,11 @@ static u64 host_efer;
> > >  static void ept_save_pdptrs(struct kvm_vcpu *vcpu);
> > >
> > >  /*
> > > - * Keep MSR_STAR at the end, as setup_msrs() will try to optimize it
> > > - * away by decrementing the array size.
> > > + * Though SYSCALL is only supported in 64-bit mode on Intel CPUs, kvm
> > > + * will emulate SYSCALL in legacy mode if the vendor string in guest
> > > + * CPUID.0:{EBX,ECX,EDX} is "AuthenticAMD" or "AMDisbetter!" To
> > > + * support this emulation, IA32_STAR must always be included in
> > > + * vmx_msr_index[], even in i386 builds.
> > >   */
> >
> > Hmm, so this isn't wrong per se, but relying on the detection of hardware
> > MSRs for software emulation is weird, e.g. vmx_vcpu_setup() will only
> > "find" MSR_STAR if it exists in hardware even though an i386 build will
> > never actually touch the hardware MSR.  This also applies to MSR_CSTAR in
> > the next patch (for all builds).
> >
> > What about making vmx_msr_index an array of structs and adding a flag to
> > denote an emulation-only MSR?  That'd document why we keep certain MSRs
> > around and would also avoid probing them in vmx_vcpu_setup(), e.g. I'm
> > assuming MSR_EFER is kept in i386 build for emulation too.
> 
> The management of this array seems overly convoluted already. Let me
> see if I can get the existing patch set accepted first, and then maybe
> we can come up with something just a little less baroque.

Works for me.

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 61aa13ba8e79..78be1c796291 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1389,15 +1389,22 @@  static u64 host_efer;
 
 static void ept_save_pdptrs(struct kvm_vcpu *vcpu);
 
-/*
- * Keep MSR_STAR at the end, as setup_msrs() will try to optimize it
- * away by decrementing the array size.
- */
-static const u32 vmx_msr_index[] = {
+struct vmx_guest_msr {
+	u32	index;
+	bool	emulation_only;
+}
+static const struct vmx_guest_msr vmx_msr_index[] = {
 #ifdef CONFIG_X86_64
-	MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR,
+	{MSR_SYSCALL_MASK, false},
+	{MSR_LSTAR, false},
+	{MSR_CSTAR, true},
+	{MSR_STAR, false},
+	{MSR_EFER, false},
+#else
+	{MSR_STAR, true},
+	{MSR_EFER, true},
 #endif
-	MSR_EFER, MSR_TSC_AUX, MSR_STAR,
+	{MSR_TSC_AUX, false}
 };
 
 DEFINE_STATIC_KEY_FALSE(enable_evmcs);
@@ -6669,14 +6676,16 @@  static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
 		vmcs_write64(GUEST_IA32_PAT, vmx->vcpu.arch.pat);
 
 	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
-		u32 index = vmx_msr_index[i];
+		u32 index = vmx_msr_index[i].index;
 		u32 data_low, data_high;
 		int j = vmx->nmsrs;
 
-		if (rdmsr_safe(index, &data_low, &data_high) < 0)
-			continue;
-		if (wrmsr_safe(index, data_low, data_high) < 0)
-			continue;
+		if (!vmx_msr_index[i].emulation_only) {
+			if (rdmsr_safe(index, &data_low, &data_high) < 0)
+				continue;
+			if (wrmsr_safe(index, data_low, data_high) < 0)
+				continue;
+		}
 		vmx->guest_msrs[j].index = i;
 		vmx->guest_msrs[j].data = 0;
 		vmx->guest_msrs[j].mask = -1ull;