Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features

[Eduardo Habkost <ehabkost@redhat.com>]

Hi,

Thanks for the patch and sorry for taking so long to review.
Comments below:

On Fri, Aug 10, 2018 at 10:06:27PM +0800, Robert Hoo wrote:
> Define FeatureWordType.
> Expand FeatureWordInfo to support both CPUID type feature word as well as
> MSR type's.
> Change feature_word_info[] accordingly.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
>  target/i386/cpu.c | 133 ++++++++++++++++++++++++++++++++++++++----------------
>  target/i386/cpu.h |   5 ++
>  2 files changed, 99 insertions(+), 39 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ba7abe5..f7c70d9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>            /* missing:
>            CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */
>  
> +typedef enum FeatureWordType {
> +   CPUID_FEATURE_WORD,
> +   MSR_FEATURE_WORD,
> +} FeatureWordType;
> +
>  typedef struct FeatureWordInfo {
> -    /* feature flags names are taken from "Intel Processor Identification and
> +    FeatureWordType type;
> +   /* feature flags names are taken from "Intel Processor Identification and
>       * the CPUID Instruction" and AMD's "CPUID Specification".
>       * In cases of disagreement between feature naming conventions,
>       * aliases may be added.
>       */
>      const char *feat_names[32];
> -    uint32_t cpuid_eax;   /* Input EAX for CPUID */
> -    bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */
> -    uint32_t cpuid_ecx;   /* Input ECX value for CPUID */
> -    int cpuid_reg;        /* output register (R_* constant) */
> +    union {
> +        /* If type==CPUID_FEATURE_WORD */
> +        struct {
> +            uint32_t eax;   /* Input EAX for CPUID */
> +            bool needs_ecx; /* CPUID instruction uses ECX as input */
> +            uint32_t ecx;   /* Input ECX value for CPUID */
> +            int reg;        /* output register (R_* constant) */
> +        } cpuid;
> +        /* If type==MSR_FEATURE_WORD */
> +        struct {
> +            uint32_t index;
> +            struct {   /*CPUID that enumerate this MSR*/
> +                FeatureWord cpuid_class;
> +                uint32_t    cpuid_flag;
> +            } cpuid_dep;
> +        } msr;
> +    };
>      uint32_t tcg_features; /* Feature flags supported by TCG */
>      uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */
>      uint32_t migratable_flags; /* Feature flags known to be migratable */

The data structure change looks good, but you are breaking the
build if you touch them without updating the existing code.  If
you break the build in your series, you break git-bisect.


[...]
> +    /*Below are MSR exposed features*/
> +    [FEATURE_WORDS_ARCH_CAPABILITIES] = {
> +        .type = MSR_FEATURE_WORD,
> +        .feat_names = {
> +            "rdctl-no", "ibrs-all", "rsba", NULL,
> +            "ssb-no", NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +        },
> +        .msr = { .index = MSR_IA32_ARCH_CAPABILITIES,
> +                .cpuid_dep = { FEAT_7_0_EDX,
> +                    CPUID_7_0_EDX_ARCH_CAPABILITIES }
> +                },
> +    },

I suggest moving this hunk to a separate patch: first we refactor
the existing code without changing behavior or adding new
features, then we add the new features.

>  };
>  
>  typedef struct X86RegisterInfo32 {
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index cddf9d9..9e8879e 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -502,9 +502,14 @@ typedef enum FeatureWord {
>      FEAT_6_EAX,         /* CPUID[6].EAX */
>      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,
> +    FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>      FEATURE_WORDS,
>  } FeatureWord;
>  
> +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR)
> +
>  typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  
>  /* cpuid_features bits */
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo