Re: [PATCH v5 2/6] target/i386: Update EPYC-Rome CPU model for Cache property, RAS, SVM feature bits

[Zhao Liu <zhao1.liu@intel.com>]

On Thu, Feb 06, 2025 at 01:28:35PM -0600, Babu Moger wrote:
> Date: Thu, 6 Feb 2025 13:28:35 -0600
> From: Babu Moger <babu.moger@amd.com>
> Subject: [PATCH v5 2/6] target/i386: Update EPYC-Rome CPU model for Cache
>  property, RAS, SVM feature bits
> X-Mailer: git-send-email 2.34.1
> 
> Found that some of the cache properties are not set correctly for EPYC models.
> 
> l1d_cache.no_invd_sharing should not be true.
> l1i_cache.no_invd_sharing should not be true.
> 
> L2.self_init should be true.
> L2.inclusive should be true.
> 
> L3.inclusive should not be true.
> L3.no_invd_sharing should be true.
> 
> Fix these cache properties.
> 
> Also add the missing RAS and SVM features bits on AMD EPYC-Rome. The SVM
> feature bits are used in nested guests.
> 
> succor		: Software uncorrectable error containment and recovery capability.
> overflow-recov	: MCA overflow recovery support.
> lbrv		: LBR virtualization
> tsc-scale	: MSR based TSC rate control
> vmcb-clean	: VMCB clean bits
> flushbyasid	: Flush by ASID
> pause-filter	: Pause intercept filter
> pfthreshold	: PAUSE filter threshold
> v-vmsave-vmload	: Virtualized VMLOAD and VMSAVE
> vgif		: Virtualized GIF
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Maksim Davydov <davydov-max@yandex-team.ru>
> ---
>  target/i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 94292bfaa2..e2c3c797ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2342,6 +2342,60 @@ static const CPUCaches epyc_rome_v3_cache_info = {
>      },
>  };
>  
> +static const CPUCaches epyc_rome_v5_cache_info = {
> +    .l1d_cache = &(CPUCacheInfo) {
> +        .type = DATA_CACHE,
> +        .level = 1,
> +        .size = 32 * KiB,
> +        .line_size = 64,
> +        .associativity = 8,
> +        .partitions = 1,
> +        .sets = 64,
> +        .lines_per_tag = 1,
> +        .self_init = 1,

This field could be true,

> +        .share_level = CPU_TOPOLOGY_LEVEL_CORE,
> +    },
> +    .l1i_cache = &(CPUCacheInfo) {
> +        .type = INSTRUCTION_CACHE,
> +        .level = 1,
> +        .size = 32 * KiB,
> +        .line_size = 64,
> +        .associativity = 8,
> +        .partitions = 1,
> +        .sets = 64,
> +        .lines_per_tag = 1,
> +        .self_init = 1,

ditto,

Compared to the previous cache model version, the differences can be
checked. I feel that in the future, when we introduce a new cache model,
it's better to avoid omitting items that default to false. This way, the
cache model can correspond to the output of the cpuid tool, making it
easier to compare and check.

Overall, LGTM,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>