Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Eduardo Habkost]

On Tue, Jul 03, 2018 at 11:06:00AM +0200, Paolo Bonzini wrote:
> On 03/07/2018 10:48, Robert Hoo wrote:
> >>
> >> However, I suggest adding it to the FeatureWord enum, since everything
> >> that handles FeatureWord applies to this new kind of MSR as well.
> >> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
> >> be like that.
> >>
> > I think this will be changing struct FeatureWordInfo, which is designed
> > for cpuid enumerations. You must not want to do that. May I know more
> > details about your thought?
> 
> The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
> Then you can do
> 
>      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> +    FEATURE_WORDS_NUM_CPUID,
> +    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> +    FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>      FEATURE_WORDS,
> };
> 
> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
>                                 FEATURE_WORDS_FIRST_MSR)
> 
> Then the existing loops that use FeatureWordInfo can go up to
> FEATURE_WORDS_NUM_CPUID.

I assume we want to make some (or most) of the loops go up to
FEATURE_WORDS (e.g. make x86_cpu_get_supported_feature_word()
support MSRs too), otherwise it would be pointless to reuse the
same array.

I would be OK with both approaches, though.  If the first version
doesn't use the `features[]` array and implements this with a
separate `msr_features[]` or `msr_arch_capabilities` field, it
would work for me (especially if this means we can get this
implemented in time for QEMU 3.0).

-- 
Eduardo