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@xxxxxxxxxx>
> Reviewed-by: Peter Shier <pshier@xxxxxxxxxx>
> ---
> 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;
--
2.19.2
> static const u32 vmx_msr_index[] = {
> #ifdef CONFIG_X86_64
> --
> 2.20.0.rc1.387.gf8505762e3-goog
>
