| Message ID | 20181205232902.1431-3-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various guest_msrs cleanups | expand |
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(-) 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;
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.
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.
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. */ static const u32 vmx_msr_index[] = { #ifdef CONFIG_X86_64
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(-)