Re: [PATCH] i386: wire up MSR_IA32_MISC_ENABLE

[Avi Kivity <avi@redhat.com>]

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.

-- 
error compiling committee.c: too many arguments to function