* [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset
@ 2014-08-14 21:39 Alex Williamson
2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 1/3] x86: Use common variable range MTRR counts Alex Williamson
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Alex Williamson @ 2014-08-14 21:39 UTC (permalink / raw)
To: qemu-devel, kvm; +Cc: lersek, qemu-stable
v3:
- Fix off-by-one identified by Laszlo in 2/3
- Add R-b in 1 & 3
It turns out that not only do we not follow the SDM guidelines for
reseting MTRR state on vCPU reset, but we really don't even attempt
to keep KVM MTRR state synchronized with QEMU, which affects not
only reset, but migration. This series implements the get/put MSR
support for KVM, then goes on to properly re-initialize the state on
vCPU reset. This resolves the problem described in the last patch
as well as some potential mismatches around migration. The migration
state is unchanged, other than actually passing valid data.
Thanks to Laszlo for his help debugging this and realization of how
terribly broken MTRR synchronization is. Thanks,
Alex
---
Alex Williamson (3):
x86: Clear MTRRs on vCPU reset
x86: kvm: Add MTRR support for kvm_get|put_msrs()
x86: Use common variable range MTRR counts
target-i386/cpu.c | 10 +++++
target-i386/cpu.h | 4 +-
target-i386/kvm.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++-
target-i386/machine.c | 2 -
4 files changed, 113 insertions(+), 4 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH v3 1/3] x86: Use common variable range MTRR counts 2014-08-14 21:39 [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Alex Williamson @ 2014-08-14 21:39 ` Alex Williamson 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs() Alex Williamson ` (2 subsequent siblings) 3 siblings, 0 replies; 6+ messages in thread From: Alex Williamson @ 2014-08-14 21:39 UTC (permalink / raw) To: qemu-devel, kvm; +Cc: lersek, qemu-stable We currently define the number of variable range MTRR registers as 8 in the CPUX86State structure and vmstate, but use MSR_MTRRcap_VCNT (also 8) to report to guests the number available. Change this to use MSR_MTRRcap_VCNT consistently. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Cc: qemu-stable@nongnu.org --- target-i386/cpu.h | 2 +- target-i386/machine.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/cpu.h b/target-i386/cpu.h index e634d83..d37d857 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -930,7 +930,7 @@ typedef struct CPUX86State { /* MTRRs */ uint64_t mtrr_fixed[11]; uint64_t mtrr_deftype; - MTRRVar mtrr_var[8]; + MTRRVar mtrr_var[MSR_MTRRcap_VCNT]; /* For KVM */ uint32_t mp_state; diff --git a/target-i386/machine.c b/target-i386/machine.c index 16d2f6a..fb89065 100644 --- a/target-i386/machine.c +++ b/target-i386/machine.c @@ -677,7 +677,7 @@ VMStateDescription vmstate_x86_cpu = { /* MTRRs */ VMSTATE_UINT64_ARRAY_V(env.mtrr_fixed, X86CPU, 11, 8), VMSTATE_UINT64_V(env.mtrr_deftype, X86CPU, 8), - VMSTATE_MTRR_VARS(env.mtrr_var, X86CPU, 8, 8), + VMSTATE_MTRR_VARS(env.mtrr_var, X86CPU, MSR_MTRRcap_VCNT, 8), /* KVM-related states */ VMSTATE_INT32_V(env.interrupt_injected, X86CPU, 9), VMSTATE_UINT32_V(env.mp_state, X86CPU, 9), ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs() 2014-08-14 21:39 [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Alex Williamson 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 1/3] x86: Use common variable range MTRR counts Alex Williamson @ 2014-08-14 21:39 ` Alex Williamson 2014-08-14 22:18 ` Laszlo Ersek 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 3/3] x86: Clear MTRRs on vCPU reset Alex Williamson 2014-08-25 16:49 ` [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Paolo Bonzini 3 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2014-08-14 21:39 UTC (permalink / raw) To: qemu-devel, kvm; +Cc: lersek, qemu-stable The MTRR state in KVM currently runs completely independent of the QEMU state in CPUX86State.mtrr_*. This means that on migration, the target loses MTRR state from the source. Generally that's ok though because KVM ignores it and maps everything as write-back anyway. The exception to this rule is when we have an assigned device and an IOMMU that doesn't promote NoSnoop transactions from that device to be cache coherent. In that case KVM trusts the guest mapping of memory as configured in the MTRR. This patch updates kvm_get|put_msrs() so that we retrieve the actual vCPU MTRR settings and therefore keep CPUX86State synchronized for migration. kvm_put_msrs() is also used on vCPU reset and therefore allows future modificaitons of MTRR state at reset to be realized. Note that the entries array used by both functions was already slightly undersized for holding every possible MSR, so this patch increases it beyond the 28 new entries necessary for MTRR state. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: qemu-stable@nongnu.org --- target-i386/cpu.h | 2 + target-i386/kvm.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/target-i386/cpu.h b/target-i386/cpu.h index d37d857..3460b12 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -337,6 +337,8 @@ #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg)) #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1) +#define MSR_MTRRphysIndex(addr) ((((addr) & ~1u) - 0x200) / 2) + #define MSR_MTRRfix64K_00000 0x250 #define MSR_MTRRfix16K_80000 0x258 #define MSR_MTRRfix16K_A0000 0x259 diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 097fe11..ddedc73 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -79,6 +79,7 @@ static int lm_capable_kernel; static bool has_msr_hv_hypercall; static bool has_msr_hv_vapic; static bool has_msr_hv_tsc; +static bool has_msr_mtrr; static bool has_msr_architectural_pmu; static uint32_t num_architectural_pmu_counters; @@ -739,6 +740,10 @@ int kvm_arch_init_vcpu(CPUState *cs) env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); } + if (env->features[FEAT_1_EDX] & CPUID_MTRR) { + has_msr_mtrr = true; + } + return 0; } @@ -1183,7 +1188,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level) CPUX86State *env = &cpu->env; struct { struct kvm_msrs info; - struct kvm_msr_entry entries[100]; + struct kvm_msr_entry entries[150]; } msr_data; struct kvm_msr_entry *msrs = msr_data.entries; int n = 0, i; @@ -1278,6 +1283,37 @@ static int kvm_put_msrs(X86CPU *cpu, int level) kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_REFERENCE_TSC, env->msr_hv_tsc); } + if (has_msr_mtrr) { + kvm_msr_entry_set(&msrs[n++], MSR_MTRRdefType, env->mtrr_deftype); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix64K_00000, env->mtrr_fixed[0]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix16K_80000, env->mtrr_fixed[1]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix16K_A0000, env->mtrr_fixed[2]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_C0000, env->mtrr_fixed[3]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_C8000, env->mtrr_fixed[4]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_D0000, env->mtrr_fixed[5]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_D8000, env->mtrr_fixed[6]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_E0000, env->mtrr_fixed[7]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_E8000, env->mtrr_fixed[8]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]); + for (i = 0; i < MSR_MTRRcap_VCNT; i++) { + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRphysBase(i), env->mtrr_var[i].base); + kvm_msr_entry_set(&msrs[n++], + MSR_MTRRphysMask(i), env->mtrr_var[i].mask); + } + } /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see * kvm_put_msr_feature_control. */ @@ -1484,7 +1520,7 @@ static int kvm_get_msrs(X86CPU *cpu) CPUX86State *env = &cpu->env; struct { struct kvm_msrs info; - struct kvm_msr_entry entries[100]; + struct kvm_msr_entry entries[150]; } msr_data; struct kvm_msr_entry *msrs = msr_data.entries; int ret, i, n; @@ -1572,6 +1608,24 @@ static int kvm_get_msrs(X86CPU *cpu) if (has_msr_hv_tsc) { msrs[n++].index = HV_X64_MSR_REFERENCE_TSC; } + if (has_msr_mtrr) { + msrs[n++].index = MSR_MTRRdefType; + msrs[n++].index = MSR_MTRRfix64K_00000; + msrs[n++].index = MSR_MTRRfix16K_80000; + msrs[n++].index = MSR_MTRRfix16K_A0000; + msrs[n++].index = MSR_MTRRfix4K_C0000; + msrs[n++].index = MSR_MTRRfix4K_C8000; + msrs[n++].index = MSR_MTRRfix4K_D0000; + msrs[n++].index = MSR_MTRRfix4K_D8000; + msrs[n++].index = MSR_MTRRfix4K_E0000; + msrs[n++].index = MSR_MTRRfix4K_E8000; + msrs[n++].index = MSR_MTRRfix4K_F0000; + msrs[n++].index = MSR_MTRRfix4K_F8000; + for (i = 0; i < MSR_MTRRcap_VCNT; i++) { + msrs[n++].index = MSR_MTRRphysBase(i); + msrs[n++].index = MSR_MTRRphysMask(i); + } + } msr_data.info.nmsrs = n; ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data); @@ -1692,6 +1746,49 @@ static int kvm_get_msrs(X86CPU *cpu) case HV_X64_MSR_REFERENCE_TSC: env->msr_hv_tsc = msrs[i].data; break; + case MSR_MTRRdefType: + env->mtrr_deftype = msrs[i].data; + break; + case MSR_MTRRfix64K_00000: + env->mtrr_fixed[0] = msrs[i].data; + break; + case MSR_MTRRfix16K_80000: + env->mtrr_fixed[1] = msrs[i].data; + break; + case MSR_MTRRfix16K_A0000: + env->mtrr_fixed[2] = msrs[i].data; + break; + case MSR_MTRRfix4K_C0000: + env->mtrr_fixed[3] = msrs[i].data; + break; + case MSR_MTRRfix4K_C8000: + env->mtrr_fixed[4] = msrs[i].data; + break; + case MSR_MTRRfix4K_D0000: + env->mtrr_fixed[5] = msrs[i].data; + break; + case MSR_MTRRfix4K_D8000: + env->mtrr_fixed[6] = msrs[i].data; + break; + case MSR_MTRRfix4K_E0000: + env->mtrr_fixed[7] = msrs[i].data; + break; + case MSR_MTRRfix4K_E8000: + env->mtrr_fixed[8] = msrs[i].data; + break; + case MSR_MTRRfix4K_F0000: + env->mtrr_fixed[9] = msrs[i].data; + break; + case MSR_MTRRfix4K_F8000: + env->mtrr_fixed[10] = msrs[i].data; + break; + case MSR_MTRRphysBase(0) ... MSR_MTRRphysMask(MSR_MTRRcap_VCNT - 1): + if (index & 1) { + env->mtrr_var[MSR_MTRRphysIndex(index)].mask = msrs[i].data; + } else { + env->mtrr_var[MSR_MTRRphysIndex(index)].base = msrs[i].data; + } + break; } } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs() 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs() Alex Williamson @ 2014-08-14 22:18 ` Laszlo Ersek 0 siblings, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2014-08-14 22:18 UTC (permalink / raw) To: Alex Williamson, qemu-devel, kvm; +Cc: qemu-stable On 08/14/14 23:39, Alex Williamson wrote: > The MTRR state in KVM currently runs completely independent of the > QEMU state in CPUX86State.mtrr_*. This means that on migration, the > target loses MTRR state from the source. Generally that's ok though > because KVM ignores it and maps everything as write-back anyway. The > exception to this rule is when we have an assigned device and an IOMMU > that doesn't promote NoSnoop transactions from that device to be cache > coherent. In that case KVM trusts the guest mapping of memory as > configured in the MTRR. > > This patch updates kvm_get|put_msrs() so that we retrieve the actual > vCPU MTRR settings and therefore keep CPUX86State synchronized for > migration. kvm_put_msrs() is also used on vCPU reset and therefore > allows future modificaitons of MTRR state at reset to be realized. > > Note that the entries array used by both functions was already > slightly undersized for holding every possible MSR, so this patch > increases it beyond the 28 new entries necessary for MTRR state. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: qemu-stable@nongnu.org > --- > > target-i386/cpu.h | 2 + > target-i386/kvm.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 101 insertions(+), 2 deletions(-) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index d37d857..3460b12 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -337,6 +337,8 @@ > #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg)) > #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1) > > +#define MSR_MTRRphysIndex(addr) ((((addr) & ~1u) - 0x200) / 2) > + > #define MSR_MTRRfix64K_00000 0x250 > #define MSR_MTRRfix16K_80000 0x258 > #define MSR_MTRRfix16K_A0000 0x259 > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 097fe11..ddedc73 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -79,6 +79,7 @@ static int lm_capable_kernel; > static bool has_msr_hv_hypercall; > static bool has_msr_hv_vapic; > static bool has_msr_hv_tsc; > +static bool has_msr_mtrr; > > static bool has_msr_architectural_pmu; > static uint32_t num_architectural_pmu_counters; > @@ -739,6 +740,10 @@ int kvm_arch_init_vcpu(CPUState *cs) > env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); > } > > + if (env->features[FEAT_1_EDX] & CPUID_MTRR) { > + has_msr_mtrr = true; > + } > + > return 0; > } > > @@ -1183,7 +1188,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level) > CPUX86State *env = &cpu->env; > struct { > struct kvm_msrs info; > - struct kvm_msr_entry entries[100]; > + struct kvm_msr_entry entries[150]; > } msr_data; > struct kvm_msr_entry *msrs = msr_data.entries; > int n = 0, i; > @@ -1278,6 +1283,37 @@ static int kvm_put_msrs(X86CPU *cpu, int level) > kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_REFERENCE_TSC, > env->msr_hv_tsc); > } > + if (has_msr_mtrr) { > + kvm_msr_entry_set(&msrs[n++], MSR_MTRRdefType, env->mtrr_deftype); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix64K_00000, env->mtrr_fixed[0]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix16K_80000, env->mtrr_fixed[1]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix16K_A0000, env->mtrr_fixed[2]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_C0000, env->mtrr_fixed[3]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_C8000, env->mtrr_fixed[4]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_D0000, env->mtrr_fixed[5]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_D8000, env->mtrr_fixed[6]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_E0000, env->mtrr_fixed[7]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_E8000, env->mtrr_fixed[8]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]); > + for (i = 0; i < MSR_MTRRcap_VCNT; i++) { > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRphysBase(i), env->mtrr_var[i].base); > + kvm_msr_entry_set(&msrs[n++], > + MSR_MTRRphysMask(i), env->mtrr_var[i].mask); > + } > + } > > /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see > * kvm_put_msr_feature_control. */ > @@ -1484,7 +1520,7 @@ static int kvm_get_msrs(X86CPU *cpu) > CPUX86State *env = &cpu->env; > struct { > struct kvm_msrs info; > - struct kvm_msr_entry entries[100]; > + struct kvm_msr_entry entries[150]; > } msr_data; > struct kvm_msr_entry *msrs = msr_data.entries; > int ret, i, n; > @@ -1572,6 +1608,24 @@ static int kvm_get_msrs(X86CPU *cpu) > if (has_msr_hv_tsc) { > msrs[n++].index = HV_X64_MSR_REFERENCE_TSC; > } > + if (has_msr_mtrr) { > + msrs[n++].index = MSR_MTRRdefType; > + msrs[n++].index = MSR_MTRRfix64K_00000; > + msrs[n++].index = MSR_MTRRfix16K_80000; > + msrs[n++].index = MSR_MTRRfix16K_A0000; > + msrs[n++].index = MSR_MTRRfix4K_C0000; > + msrs[n++].index = MSR_MTRRfix4K_C8000; > + msrs[n++].index = MSR_MTRRfix4K_D0000; > + msrs[n++].index = MSR_MTRRfix4K_D8000; > + msrs[n++].index = MSR_MTRRfix4K_E0000; > + msrs[n++].index = MSR_MTRRfix4K_E8000; > + msrs[n++].index = MSR_MTRRfix4K_F0000; > + msrs[n++].index = MSR_MTRRfix4K_F8000; > + for (i = 0; i < MSR_MTRRcap_VCNT; i++) { > + msrs[n++].index = MSR_MTRRphysBase(i); > + msrs[n++].index = MSR_MTRRphysMask(i); > + } > + } > > msr_data.info.nmsrs = n; > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data); > @@ -1692,6 +1746,49 @@ static int kvm_get_msrs(X86CPU *cpu) > case HV_X64_MSR_REFERENCE_TSC: > env->msr_hv_tsc = msrs[i].data; > break; > + case MSR_MTRRdefType: > + env->mtrr_deftype = msrs[i].data; > + break; > + case MSR_MTRRfix64K_00000: > + env->mtrr_fixed[0] = msrs[i].data; > + break; > + case MSR_MTRRfix16K_80000: > + env->mtrr_fixed[1] = msrs[i].data; > + break; > + case MSR_MTRRfix16K_A0000: > + env->mtrr_fixed[2] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_C0000: > + env->mtrr_fixed[3] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_C8000: > + env->mtrr_fixed[4] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_D0000: > + env->mtrr_fixed[5] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_D8000: > + env->mtrr_fixed[6] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_E0000: > + env->mtrr_fixed[7] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_E8000: > + env->mtrr_fixed[8] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_F0000: > + env->mtrr_fixed[9] = msrs[i].data; > + break; > + case MSR_MTRRfix4K_F8000: > + env->mtrr_fixed[10] = msrs[i].data; > + break; > + case MSR_MTRRphysBase(0) ... MSR_MTRRphysMask(MSR_MTRRcap_VCNT - 1): > + if (index & 1) { > + env->mtrr_var[MSR_MTRRphysIndex(index)].mask = msrs[i].data; > + } else { > + env->mtrr_var[MSR_MTRRphysIndex(index)].base = msrs[i].data; > + } > + break; > } > } > > Compared it with v2 2/3. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] x86: Clear MTRRs on vCPU reset 2014-08-14 21:39 [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Alex Williamson 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 1/3] x86: Use common variable range MTRR counts Alex Williamson 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs() Alex Williamson @ 2014-08-14 21:39 ` Alex Williamson 2014-08-25 16:49 ` [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Paolo Bonzini 3 siblings, 0 replies; 6+ messages in thread From: Alex Williamson @ 2014-08-14 21:39 UTC (permalink / raw) To: qemu-devel, kvm; +Cc: lersek, qemu-stable The SDM specifies (June 2014 Vol3 11.11.5): On a hardware reset, the P6 and more recent processors clear the valid flags in variable-range MTRRs and clear the E flag in the IA32_MTRR_DEF_TYPE MSR to disable all MTRRs. All other bits in the MTRRs are undefined. We currently do none of that, so whatever MTRR settings you had prior to reset is what you have after reset. Usually this doesn't matter because KVM often ignores the guest mappings and uses write-back anyway. However, if you have an assigned device and an IOMMU that allows NoSnoop for that device, KVM defers to the guest memory mappings which are now stale after reset. The result is that OVMF rebooting on such a configuration takes a full minute to LZMA decompress the firmware volume, a process that is nearly instant on the initial boot. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Cc: qemu-stable@nongnu.org --- target-i386/cpu.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 6d008ab..9768be1 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2588,6 +2588,16 @@ static void x86_cpu_reset(CPUState *s) env->xcr0 = 1; + /* + * SDM 11.11.5 requires: + * - IA32_MTRR_DEF_TYPE MSR.E = 0 + * - IA32_MTRR_PHYSMASKn.V = 0 + * All other bits are undefined. For simplification, zero it all. + */ + env->mtrr_deftype = 0; + memset(env->mtrr_var, 0, sizeof(env->mtrr_var)); + memset(env->mtrr_fixed, 0, sizeof(env->mtrr_fixed)); + #if !defined(CONFIG_USER_ONLY) /* We hard-wire the BSP to the first CPU. */ if (s->cpu_index == 0) { ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset 2014-08-14 21:39 [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Alex Williamson ` (2 preceding siblings ...) 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 3/3] x86: Clear MTRRs on vCPU reset Alex Williamson @ 2014-08-25 16:49 ` Paolo Bonzini 3 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2014-08-25 16:49 UTC (permalink / raw) To: Alex Williamson, qemu-devel, kvm; +Cc: lersek, qemu-stable Il 14/08/2014 23:39, Alex Williamson ha scritto: > v3: > - Fix off-by-one identified by Laszlo in 2/3 > - Add R-b in 1 & 3 > > It turns out that not only do we not follow the SDM guidelines for > reseting MTRR state on vCPU reset, but we really don't even attempt > to keep KVM MTRR state synchronized with QEMU, which affects not > only reset, but migration. This series implements the get/put MSR > support for KVM, then goes on to properly re-initialize the state on > vCPU reset. This resolves the problem described in the last patch > as well as some potential mismatches around migration. The migration > state is unchanged, other than actually passing valid data. > > Thanks to Laszlo for his help debugging this and realization of how > terribly broken MTRR synchronization is. Thanks, Applying to uq/master, thanks. Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-25 16:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-14 21:39 [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Alex Williamson 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 1/3] x86: Use common variable range MTRR counts Alex Williamson 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs() Alex Williamson 2014-08-14 22:18 ` Laszlo Ersek 2014-08-14 21:39 ` [Qemu-devel] [PATCH v3 3/3] x86: Clear MTRRs on vCPU reset Alex Williamson 2014-08-25 16:49 ` [Qemu-devel] [PATCH v3 0/3] Sync MTRRs with KVM and disable on reset Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).