| Message ID | 1317738395-6488-1-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2011-10-04 16:26, Avi Kivity wrote: > It's needed for its default value - bit 0 specifies that "rep movs" is > good enough for memcpy, and Linux may use a slower memcpu if it is not set, > depending on cpu family/model. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > target-i386/cpu.h | 5 +++++ > target-i386/helper.c | 1 + > target-i386/kvm.c | 15 +++++++++++++++ > target-i386/machine.c | 21 +++++++++++++++++++++ > target-i386/op_helper.c | 6 ++++++ > 5 files changed, 48 insertions(+), 0 deletions(-) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index ae36489..5416809 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -299,6 +299,10 @@ > > #define MSR_IA32_PERF_STATUS 0x198 > > +#define MSR_IA32_MISC_ENABLE 0x1a0 I smell tabs... > +/* Indicates good rep/movs microcode on some processors: */ > +#define MSR_IA32_MISC_ENABLE_DEFAULT 1 > + > #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg)) > #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1) > > @@ -689,6 +693,7 @@ typedef struct CPUX86State { > uint64_t tsc; > > uint64_t mcg_status; > + uint64_t msr_ia32_misc_enable; > > /* exception/interrupt handling */ > int error_code; > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 5df40d4..6c6a167 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -98,6 +98,7 @@ void cpu_reset(CPUX86State *env) > env->mxcsr = 0x1f80; > > env->pat = 0x0007040600070406ULL; > + env->msr_ia32_misc_enable = MSR_IA32_MISC_ENABLE_DEFAULT; > > memset(env->dr, 0, sizeof(env->dr)); > env->dr[6] = DR6_FIXED_1; > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index b6eef04..b3a650b 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -60,6 +60,7 @@ > static bool has_msr_star; > static bool has_msr_hsave_pa; > static bool has_msr_async_pf_en; > +static bool has_msr_misc_enable; > static int lm_capable_kernel; > > static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > @@ -568,6 +569,10 @@ static int kvm_get_supported_msrs(KVMState *s) > has_msr_hsave_pa = true; > continue; > } > + if (kvm_msr_list->indices[i] == MSR_IA32_MISC_ENABLE) { > + has_msr_misc_enable = true; > + continue; > + } > } > } > > @@ -881,6 +886,10 @@ static int kvm_put_msrs(CPUState *env, int level) > if (has_msr_hsave_pa) { > kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave); > } > + if (has_msr_misc_enable) { > + kvm_msr_entry_set(&msrs[n++], MSR_IA32_MISC_ENABLE, > + env->msr_ia32_misc_enable); > + } > #ifdef TARGET_X86_64 > if (lm_capable_kernel) { > kvm_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar); > @@ -1127,6 +1136,9 @@ static int kvm_get_msrs(CPUState *env) > if (has_msr_hsave_pa) { > msrs[n++].index = MSR_VM_HSAVE_PA; > } > + if (has_msr_misc_enable) { > + msrs[n++].index = MSR_IA32_MISC_ENABLE; > + } > > if (!env->tsc_valid) { > msrs[n++].index = MSR_IA32_TSC; > @@ -1210,6 +1222,9 @@ static int kvm_get_msrs(CPUState *env) > case MSR_MCG_CTL: > env->mcg_ctl = msrs[i].data; > break; > + case MSR_IA32_MISC_ENABLE: > + env->msr_ia32_misc_enable = msrs[i].data; > + break; > default: > if (msrs[i].index >= MSR_MC0_CTL && > msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 4) { > diff --git a/target-i386/machine.c b/target-i386/machine.c > index 9aca8e0..73c1ffe 100644 > --- a/target-i386/machine.c > +++ b/target-i386/machine.c > @@ -310,6 +310,24 @@ static bool fpop_ip_dp_needed(void *opaque) > } > }; > > +static bool misc_enable_needed(void *opaque) > +{ > + CPUState *env = opaque; > + > + return env->msr_ia32_misc_enable != MSR_IA32_MISC_ENABLE_DEFAULT; > +} > + > +static const VMStateDescription vmstate_msr_ia32_misc_enable = { > + .name = "cpu/msr_ia32_misc_enable", > + .version_id = 1, > + .minimum_version_id = 1, > + .minimum_version_id_old = 1, > + .fields = (VMStateField []) { > + VMSTATE_UINT64(msr_ia32_misc_enable, CPUState), > + VMSTATE_END_OF_LIST() > + } > +}; > + We are about to bump the CPU_SAVE_VERSION for the sake of APIC deadline timer, so you can jump on that train and avoid this subsection. > static const VMStateDescription vmstate_cpu = { > .name = "cpu", > .version_id = CPU_SAVE_VERSION, > @@ -420,6 +438,9 @@ static bool fpop_ip_dp_needed(void *opaque) > } , { > .vmsd = &vmstate_fpop_ip_dp, > .needed = fpop_ip_dp_needed, > + }, { > + .vmsd = &vmstate_msr_ia32_misc_enable, > + .needed = misc_enable_needed, > } , { > /* empty */ > } > diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c > index 3bb5a91..c89e4a4 100644 > --- a/target-i386/op_helper.c > +++ b/target-i386/op_helper.c > @@ -3280,6 +3280,9 @@ void helper_wrmsr(void) > case MSR_TSC_AUX: > env->tsc_aux = val; > break; > + case MSR_IA32_MISC_ENABLE: > + env->msr_ia32_misc_enable = val; > + break; This MSR is Intel-specific, isn't it? Then I guess it should be limited to Intel CPU types. > default: > if ((uint32_t)ECX >= MSR_MC0_CTL > && (uint32_t)ECX < MSR_MC0_CTL + (4 * env->mcg_cap & 0xff)) { > @@ -3413,6 +3416,9 @@ void helper_rdmsr(void) > case MSR_MCG_STATUS: > val = env->mcg_status; > break; > + case MSR_IA32_MISC_ENABLE: > + val = env->msr_ia32_misc_enable; > + break; > default: > if ((uint32_t)ECX >= MSR_MC0_CTL > && (uint32_t)ECX < MSR_MC0_CTL + (4 * env->mcg_cap & 0xff)) { Jan
On 10/04/2011 06:05 PM, Jan Kiszka wrote: > On 2011-10-04 16:26, Avi Kivity wrote: > > It's needed for its default value - bit 0 specifies that "rep movs" is > > good enough for memcpy, and Linux may use a slower memcpu if it is not set, > > depending on cpu family/model. > > > > Signed-off-by: Avi Kivity<avi@redhat.com> > > --- > > target-i386/cpu.h | 5 +++++ > > target-i386/helper.c | 1 + > > target-i386/kvm.c | 15 +++++++++++++++ > > target-i386/machine.c | 21 +++++++++++++++++++++ > > target-i386/op_helper.c | 6 ++++++ > > 5 files changed, 48 insertions(+), 0 deletions(-) > > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index ae36489..5416809 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -299,6 +299,10 @@ > > > > #define MSR_IA32_PERF_STATUS 0x198 > > > > +#define MSR_IA32_MISC_ENABLE 0x1a0 > > I smell tabs... Oops. Cut'n'paste flew underneath the emacs radar. > > + > > +static const VMStateDescription vmstate_msr_ia32_misc_enable = { > > + .name = "cpu/msr_ia32_misc_enable", > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .minimum_version_id_old = 1, > > + .fields = (VMStateField []) { > > + VMSTATE_UINT64(msr_ia32_misc_enable, CPUState), > > + VMSTATE_END_OF_LIST() > > + } > > +}; > > + > > We are about to bump the CPU_SAVE_VERSION for the sake of APIC deadline > timer, so you can jump on that train and avoid this subsection. Must we do that? Considering that no guest will use the deadline timer, it seems to be an excellent candidates for subsections. > > diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c > > index 3bb5a91..c89e4a4 100644 > > --- a/target-i386/op_helper.c > > +++ b/target-i386/op_helper.c > > @@ -3280,6 +3280,9 @@ void helper_wrmsr(void) > > case MSR_TSC_AUX: > > env->tsc_aux = val; > > break; > > + case MSR_IA32_MISC_ENABLE: > > + env->msr_ia32_misc_enable = val; > > + break; > > This MSR is Intel-specific, isn't it? Then I guess it should be limited > to Intel CPU types. It's an "architectural MSR" that is only available on some Intel models. Either we do a full cpuid qualification of accessible MSRs (and bits within MSRs), or not. Qualifying just by vendor ID is pointless.
On 2011-10-04 19:08, Avi Kivity wrote: > On 10/04/2011 06:05 PM, Jan Kiszka wrote: >> On 2011-10-04 16:26, Avi Kivity wrote: >> > It's needed for its default value - bit 0 specifies that "rep movs" is >> > good enough for memcpy, and Linux may use a slower memcpu if it is >> not set, >> > depending on cpu family/model. >> > >> > Signed-off-by: Avi Kivity<avi@redhat.com> >> > --- >> > target-i386/cpu.h | 5 +++++ >> > target-i386/helper.c | 1 + >> > target-i386/kvm.c | 15 +++++++++++++++ >> > target-i386/machine.c | 21 +++++++++++++++++++++ >> > target-i386/op_helper.c | 6 ++++++ >> > 5 files changed, 48 insertions(+), 0 deletions(-) >> > >> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h >> > index ae36489..5416809 100644 >> > --- a/target-i386/cpu.h >> > +++ b/target-i386/cpu.h >> > @@ -299,6 +299,10 @@ >> > >> > #define MSR_IA32_PERF_STATUS 0x198 >> > >> > +#define MSR_IA32_MISC_ENABLE 0x1a0 >> >> I smell tabs... > > Oops. Cut'n'paste flew underneath the emacs radar. > >> > + >> > +static const VMStateDescription vmstate_msr_ia32_misc_enable = { >> > + .name = "cpu/msr_ia32_misc_enable", >> > + .version_id = 1, >> > + .minimum_version_id = 1, >> > + .minimum_version_id_old = 1, >> > + .fields = (VMStateField []) { >> > + VMSTATE_UINT64(msr_ia32_misc_enable, CPUState), >> > + VMSTATE_END_OF_LIST() >> > + } >> > +}; >> > + >> >> We are about to bump the CPU_SAVE_VERSION for the sake of APIC deadline >> timer, so you can jump on that train and avoid this subsection. > > Must we do that? Considering that no guest will use the deadline timer, > it seems to be an excellent candidates for subsections. I don't know, it was sent out for pull like that. And I thought subsections are still broken, aren't they? > >> > diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c >> > index 3bb5a91..c89e4a4 100644 >> > --- a/target-i386/op_helper.c >> > +++ b/target-i386/op_helper.c >> > @@ -3280,6 +3280,9 @@ void helper_wrmsr(void) >> > case MSR_TSC_AUX: >> > env->tsc_aux = val; >> > break; >> > + case MSR_IA32_MISC_ENABLE: >> > + env->msr_ia32_misc_enable = val; >> > + break; >> >> This MSR is Intel-specific, isn't it? Then I guess it should be limited >> to Intel CPU types. > > It's an "architectural MSR" that is only available on some Intel > models. Either we do a full cpuid qualification of accessible MSRs (and > bits within MSRs), or not. Qualifying just by vendor ID is pointless. Given that, when in conflict, we rather model after AMD than Intel for TCG, I would hesitate to expose this by default. Or are there precedences already? Jan
On 10/04/2011 07:14 PM, Jan Kiszka wrote: > > > >> > + > >> > +static const VMStateDescription vmstate_msr_ia32_misc_enable = { > >> > + .name = "cpu/msr_ia32_misc_enable", > >> > + .version_id = 1, > >> > + .minimum_version_id = 1, > >> > + .minimum_version_id_old = 1, > >> > + .fields = (VMStateField []) { > >> > + VMSTATE_UINT64(msr_ia32_misc_enable, CPUState), > >> > + VMSTATE_END_OF_LIST() > >> > + } > >> > +}; > >> > + > >> > >> We are about to bump the CPU_SAVE_VERSION for the sake of APIC deadline > >> timer, so you can jump on that train and avoid this subsection. > > > > Must we do that? Considering that no guest will use the deadline timer, > > it seems to be an excellent candidates for subsections. > > I don't know, it was sent out for pull like that. And I thought > subsections are still broken, aren't they? Well let's fix subsections instead of disabling migration to older versions. > >> > >> This MSR is Intel-specific, isn't it? Then I guess it should be limited > >> to Intel CPU types. > > > > It's an "architectural MSR" that is only available on some Intel > > models. Either we do a full cpuid qualification of accessible MSRs (and > > bits within MSRs), or not. Qualifying just by vendor ID is pointless. > > Given that, when in conflict, we rather model after AMD than Intel for > TCG, I would hesitate to expose this by default. Or are there > precedences already? Practically all MSRs. i486 doesn't have any, IIRC, for example. (and given this MSR has no effect, the only difference it makes to guests is the #GP we take or not; still it may be worthwhile to construct some table-driven thing to allow or reject MSR accesses, both for kvm and qemu)
On 2011-10-04 19:21, Avi Kivity wrote: > On 10/04/2011 07:14 PM, Jan Kiszka wrote: >> > >> >> > + >> >> > +static const VMStateDescription vmstate_msr_ia32_misc_enable = { >> >> > + .name = "cpu/msr_ia32_misc_enable", >> >> > + .version_id = 1, >> >> > + .minimum_version_id = 1, >> >> > + .minimum_version_id_old = 1, >> >> > + .fields = (VMStateField []) { >> >> > + VMSTATE_UINT64(msr_ia32_misc_enable, CPUState), >> >> > + VMSTATE_END_OF_LIST() >> >> > + } >> >> > +}; >> >> > + >> >> >> >> We are about to bump the CPU_SAVE_VERSION for the sake of APIC >> deadline >> >> timer, so you can jump on that train and avoid this subsection. >> > >> > Must we do that? Considering that no guest will use the deadline >> timer, >> > it seems to be an excellent candidates for subsections. >> >> I don't know, it was sent out for pull like that. And I thought >> subsections are still broken, aren't they? > > Well let's fix subsections instead of disabling migration to older > versions. > >> >> >> >> This MSR is Intel-specific, isn't it? Then I guess it should be >> limited >> >> to Intel CPU types. >> > >> > It's an "architectural MSR" that is only available on some Intel >> > models. Either we do a full cpuid qualification of accessible MSRs >> (and >> > bits within MSRs), or not. Qualifying just by vendor ID is pointless. >> >> Given that, when in conflict, we rather model after AMD than Intel for >> TCG, I would hesitate to expose this by default. Or are there >> precedences already? > > Practically all MSRs. i486 doesn't have any, IIRC, for example. Pre-Pentiums don't have instructions to access them as well, so that doesn't cause any harm. > > (and given this MSR has no effect, the only difference it makes to > guests is the #GP we take or not; still it may be worthwhile to > construct some table-driven thing to allow or reject MSR accesses, both > for kvm and qemu) Right. If this MSR is not the first bogus one on AMD, we can do this later. If it is, it should be done first. Jan
On 10/04/2011 07:24 PM, Jan Kiszka wrote: > >> > >> Given that, when in conflict, we rather model after AMD than Intel for > >> TCG, I would hesitate to expose this by default. Or are there > >> precedences already? > > > > Practically all MSRs. i486 doesn't have any, IIRC, for example. > > Pre-Pentiums don't have instructions to access them as well, so that > doesn't cause any harm. kvm doesn't detect this; does tcg? In any case, MSR availability varies widely with processor model. > > > > (and given this MSR has no effect, the only difference it makes to > > guests is the #GP we take or not; still it may be worthwhile to > > construct some table-driven thing to allow or reject MSR accesses, both > > for kvm and qemu) > > Right. If this MSR is not the first bogus one on AMD, we can do this > later. If it is, it should be done first. It's certainly not the first - they practically all are, depending on exact model.
diff --git a/target-i386/cpu.h b/target-i386/cpu.h index ae36489..5416809 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -299,6 +299,10 @@ #define MSR_IA32_PERF_STATUS 0x198 +#define MSR_IA32_MISC_ENABLE 0x1a0 +/* Indicates good rep/movs microcode on some processors: */ +#define MSR_IA32_MISC_ENABLE_DEFAULT 1 + #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg)) #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1) @@ -689,6 +693,7 @@ typedef struct CPUX86State { uint64_t tsc; uint64_t mcg_status; + uint64_t msr_ia32_misc_enable; /* exception/interrupt handling */ int error_code; diff --git a/target-i386/helper.c b/target-i386/helper.c index 5df40d4..6c6a167 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -98,6 +98,7 @@ void cpu_reset(CPUX86State *env) env->mxcsr = 0x1f80; env->pat = 0x0007040600070406ULL; + env->msr_ia32_misc_enable = MSR_IA32_MISC_ENABLE_DEFAULT; memset(env->dr, 0, sizeof(env->dr)); env->dr[6] = DR6_FIXED_1; diff --git a/target-i386/kvm.c b/target-i386/kvm.c index b6eef04..b3a650b 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -60,6 +60,7 @@ static bool has_msr_star; static bool has_msr_hsave_pa; static bool has_msr_async_pf_en; +static bool has_msr_misc_enable; static int lm_capable_kernel; static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) @@ -568,6 +569,10 @@ static int kvm_get_supported_msrs(KVMState *s) has_msr_hsave_pa = true; continue; } + if (kvm_msr_list->indices[i] == MSR_IA32_MISC_ENABLE) { + has_msr_misc_enable = true; + continue; + } } } @@ -881,6 +886,10 @@ static int kvm_put_msrs(CPUState *env, int level) if (has_msr_hsave_pa) { kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave); } + if (has_msr_misc_enable) { + kvm_msr_entry_set(&msrs[n++], MSR_IA32_MISC_ENABLE, + env->msr_ia32_misc_enable); + } #ifdef TARGET_X86_64 if (lm_capable_kernel) { kvm_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar); @@ -1127,6 +1136,9 @@ static int kvm_get_msrs(CPUState *env) if (has_msr_hsave_pa) { msrs[n++].index = MSR_VM_HSAVE_PA; } + if (has_msr_misc_enable) { + msrs[n++].index = MSR_IA32_MISC_ENABLE; + } if (!env->tsc_valid) { msrs[n++].index = MSR_IA32_TSC; @@ -1210,6 +1222,9 @@ static int kvm_get_msrs(CPUState *env) case MSR_MCG_CTL: env->mcg_ctl = msrs[i].data; break; + case MSR_IA32_MISC_ENABLE: + env->msr_ia32_misc_enable = msrs[i].data; + break; default: if (msrs[i].index >= MSR_MC0_CTL && msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 4) { diff --git a/target-i386/machine.c b/target-i386/machine.c index 9aca8e0..73c1ffe 100644 --- a/target-i386/machine.c +++ b/target-i386/machine.c @@ -310,6 +310,24 @@ static bool fpop_ip_dp_needed(void *opaque) } }; +static bool misc_enable_needed(void *opaque) +{ + CPUState *env = opaque; + + return env->msr_ia32_misc_enable != MSR_IA32_MISC_ENABLE_DEFAULT; +} + +static const VMStateDescription vmstate_msr_ia32_misc_enable = { + .name = "cpu/msr_ia32_misc_enable", + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField []) { + VMSTATE_UINT64(msr_ia32_misc_enable, CPUState), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_cpu = { .name = "cpu", .version_id = CPU_SAVE_VERSION, @@ -420,6 +438,9 @@ static bool fpop_ip_dp_needed(void *opaque) } , { .vmsd = &vmstate_fpop_ip_dp, .needed = fpop_ip_dp_needed, + }, { + .vmsd = &vmstate_msr_ia32_misc_enable, + .needed = misc_enable_needed, } , { /* empty */ } diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c index 3bb5a91..c89e4a4 100644 --- a/target-i386/op_helper.c +++ b/target-i386/op_helper.c @@ -3280,6 +3280,9 @@ void helper_wrmsr(void) case MSR_TSC_AUX: env->tsc_aux = val; break; + case MSR_IA32_MISC_ENABLE: + env->msr_ia32_misc_enable = val; + break; default: if ((uint32_t)ECX >= MSR_MC0_CTL && (uint32_t)ECX < MSR_MC0_CTL + (4 * env->mcg_cap & 0xff)) { @@ -3413,6 +3416,9 @@ void helper_rdmsr(void) case MSR_MCG_STATUS: val = env->mcg_status; break; + case MSR_IA32_MISC_ENABLE: + val = env->msr_ia32_misc_enable; + break; default: if ((uint32_t)ECX >= MSR_MC0_CTL && (uint32_t)ECX < MSR_MC0_CTL + (4 * env->mcg_cap & 0xff)) {
It's needed for its default value - bit 0 specifies that "rep movs" is good enough for memcpy, and Linux may use a slower memcpu if it is not set, depending on cpu family/model. Signed-off-by: Avi Kivity <avi@redhat.com> --- target-i386/cpu.h | 5 +++++ target-i386/helper.c | 1 + target-i386/kvm.c | 15 +++++++++++++++ target-i386/machine.c | 21 +++++++++++++++++++++ target-i386/op_helper.c | 6 ++++++ 5 files changed, 48 insertions(+), 0 deletions(-)