Re: [Qemu-devel] [PATCH v7 2/9] i386: Add cache information in X86CPUDefinition

[Eduardo Habkost]

On Thu, Apr 26, 2018 at 11:26:42AM -0500, Babu Moger wrote:
> Add cache information in X86CPUDefinition and CPUX86State.
> 
> Signed-off-by: Babu Moger <address@hidden>
> Tested-by: Geoffrey McRae <address@hidden>
> ---
>  target/i386/cpu.c | 4 ++++
>  target/i386/cpu.h | 8 ++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b6c1592..a518a0f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1105,6 +1105,7 @@ struct X86CPUDefinition {
>      int stepping;
>      FeatureWordArray features;
>      const char *model_id;
> +    CPUCaches cache_info;
>  };
>  
>  static X86CPUDefinition builtin_x86_defs[] = {
> @@ -3242,6 +3243,9 @@ static void x86_cpu_load_def(X86CPU *cpu, 
> X86CPUDefinition *def, Error **errp)
>          env->features[w] = def->features[w];
>      }
>  
> +    /* Load Cache information from the X86CPUDefinition */
> +    memcpy(&env->cache_info, &def->cache_info, sizeof(CPUCaches));
> +
>      /* Special cases not set in the X86CPUDefinition structs: */
>      /* TODO: in-kernel irqchip for hvf */
>      if (kvm_enabled()) {
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index fa03e2c..1213bb7 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1096,6 +1096,13 @@ typedef struct CPUCacheInfo {
>  } CPUCacheInfo;
>  
>  
> +typedef struct CPUCaches {
> +        bool valid;
> +        CPUCacheInfo l1d_cache;
> +        CPUCacheInfo l1i_cache;
> +        CPUCacheInfo l2_cache;
> +        CPUCacheInfo l3_cache;
> +} CPUCaches;
>  
>  typedef struct CPUX86State {
>      /* standard registers */
> @@ -1282,6 +1289,7 @@ typedef struct CPUX86State {
>      /* Features that were explicitly enabled/disabled */
>      FeatureWordArray user_features;
>      uint32_t cpuid_model[12];
> +    CPUCaches cache_info;

Suggestion for a follow-up patch, or in case there's need to
respin this series: what about making both
X86CPUDefinition::cache_info and CPUX86State::cache_info pointers
instead of embedded structs?  This way you won't need the 'valid'
field (you can just check if the pointer is NULL), and won't need
the memcpy() above.

This shouldn't block the series, though:

Reviewed-by: Eduardo Habkost <address@hidden>

-- 
Eduardo