Re: [PATCH] i386: wire up MSR_IA32_MISC_ENABLE

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]