RE: [PATCH v3 2/7] target/i386: Add new EPYC CPU versions with updated cache_info

[Moger, Babu]

[AMD Official Use Only - General]

Hi Maksim,

> -----Original Message-----
> From: Maksim Davydov <davydov-max@yandex-team.ru>
> Sent: Wednesday, April 26, 2023 3:35 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: weijiang.yang@intel.com; philmd@linaro.org; dwmw@amazon.co.uk;
> paul@xen.org; joao.m.martins@oracle.com; qemu-devel@nongnu.org;
> mtosatti@redhat.com; kvm@vger.kernel.org; mst@redhat.com;
> marcel.apfelbaum@gmail.com; yang.zhong@intel.com; jing2.liu@intel.com;
> vkuznets@redhat.com; Roth, Michael <Michael.Roth@amd.com>; Huang2, Wei
> <Wei.Huang2@amd.com>; berrange@redhat.com; pbonzini@redhat.com;
> richard.henderson@linaro.org
> Subject: Re: [PATCH v3 2/7] target/i386: Add new EPYC CPU versions with
> updated cache_info
> 
> 
> On 4/25/23 18:35, Moger, Babu wrote:
> > Hi Maksim,
> >
> > On 4/25/23 07:51, Maksim Davydov wrote:
> >> On 4/24/23 19:33, Babu Moger wrote:
> >>> From: Michael Roth <michael.roth@amd.com>
> >>>
> >>> Introduce new EPYC cpu versions: EPYC-v4 and EPYC-Rome-v3.
> >>> The only difference vs. older models is an updated cache_info with
> >>> the 'complex_indexing' bit unset, since this bit is not currently
> >>> defined for AMD and may cause problems should it be used for
> >>> something else in the future. Setting this bit will also cause CPUID
> >>> validation failures when running SEV-SNP guests.
> >>>
> >>> Signed-off-by: Michael Roth <michael.roth@amd.com>
> >>> Signed-off-by: Babu Moger <babu.moger@amd.com>
> >>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>    target/i386/cpu.c | 118
> >>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 118 insertions(+)
> >>>
> >>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c index
> >>> e3d9eaa307..c1bc47661d 100644
> >>> --- a/target/i386/cpu.c
> >>> +++ b/target/i386/cpu.c
> >>> @@ -1707,6 +1707,56 @@ static const CPUCaches epyc_cache_info = {
> >>>        },
> >>>    };
> >>>    +static CPUCaches epyc_v4_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,
> >>> +        .no_invd_sharing = true,
> >>> +    },
> >>> +    .l1i_cache = &(CPUCacheInfo) {
> >>> +        .type = INSTRUCTION_CACHE,
> >>> +        .level = 1,
> >>> +        .size = 64 * KiB,
> >>> +        .line_size = 64,
> >>> +        .associativity = 4,
> >>> +        .partitions = 1,
> >>> +        .sets = 256,
> >>> +        .lines_per_tag = 1,
> >>> +        .self_init = 1,
> >>> +        .no_invd_sharing = true,
> >>> +    },
> >>> +    .l2_cache = &(CPUCacheInfo) {
> >>> +        .type = UNIFIED_CACHE,
> >>> +        .level = 2,
> >>> +        .size = 512 * KiB,
> >>> +        .line_size = 64,
> >>> +        .associativity = 8,
> >>> +        .partitions = 1,
> >>> +        .sets = 1024,
> >>> +        .lines_per_tag = 1,
> >>> +    },
> >>> +    .l3_cache = &(CPUCacheInfo) {
> >>> +        .type = UNIFIED_CACHE,
> >>> +        .level = 3,
> >>> +        .size = 8 * MiB,
> >>> +        .line_size = 64,
> >>> +        .associativity = 16,
> >>> +        .partitions = 1,
> >>> +        .sets = 8192,
> >>> +        .lines_per_tag = 1,
> >>> +        .self_init = true,
> >>> +        .inclusive = true,
> >>> +        .complex_indexing = false,
> >>> +    },
> >>> +};
> >>> +
> >>>    static const CPUCaches epyc_rome_cache_info = {
> >>>        .l1d_cache = &(CPUCacheInfo) {
> >>>            .type = DATA_CACHE,
> >>> @@ -1757,6 +1807,56 @@ static const CPUCaches epyc_rome_cache_info
> =
> >>> {
> >>>        },
> >>>    };
> >>>    +static const CPUCaches epyc_rome_v3_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,
> >>> +        .no_invd_sharing = true,
> >>> +    },
> >>> +    .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,
> >>> +        .no_invd_sharing = true,
> >>> +    },
> >>> +    .l2_cache = &(CPUCacheInfo) {
> >>> +        .type = UNIFIED_CACHE,
> >>> +        .level = 2,
> >>> +        .size = 512 * KiB,
> >>> +        .line_size = 64,
> >>> +        .associativity = 8,
> >>> +        .partitions = 1,
> >>> +        .sets = 1024,
> >>> +        .lines_per_tag = 1,
> >>> +    },
> >>> +    .l3_cache = &(CPUCacheInfo) {
> >>> +        .type = UNIFIED_CACHE,
> >>> +        .level = 3,
> >>> +        .size = 16 * MiB,
> >>> +        .line_size = 64,
> >>> +        .associativity = 16,
> >>> +        .partitions = 1,
> >>> +        .sets = 16384,
> >>> +        .lines_per_tag = 1,
> >>> +        .self_init = true,
> >>> +        .inclusive = true,
> >>> +        .complex_indexing = false,
> >>> +    },
> >>> +};
> >>> +
> >>>    static const CPUCaches epyc_milan_cache_info = {
> >>>        .l1d_cache = &(CPUCacheInfo) {
> >>>            .type = DATA_CACHE,
> >>> @@ -4091,6 +4191,15 @@ static const X86CPUDefinition
> >>> builtin_x86_defs[] = {
> >>>                        { /* end of list */ }
> >>>                    }
> >>>                },
> >>> +            {
> >>> +                .version = 4,
> >>> +                .props = (PropValue[]) {
> >>> +                    { "model-id",
> >>> +                      "AMD EPYC-v4 Processor" },
> >>> +                    { /* end of list */ }
> >>> +                },
> >>> +                .cache_info = &epyc_v4_cache_info
> >>> +            },
> >>>                { /* end of list */ }
> >>>            }
> >>>        },
> >>> @@ -4210,6 +4319,15 @@ static const X86CPUDefinition
> >>> builtin_x86_defs[] = {
> >>>                        { /* end of list */ }
> >>>                    }
> >>>                },
> >>> +            {
> >>> +                .version = 3,
> >>> +                .props = (PropValue[]) {
> >>> +                    { "model-id",
> >>> +                      "AMD EPYC-Rome-v3 Processor" },
> >> What do you think about adding more information to the model name to
> >> reveal its key feature? For instance, model-id can be "EPYC-Rome-v3
> >> (NO INDEXING)", because only cache info was affected. Or alias can be
> >> used to achieve the same effect. It works well in
> > Actually, we already thought about it. But decided against it. Reason
> > is, when we add "(NO INDEXING)" to v3, we need to keep text in all the
> > future revisions v4 etc and other cpu models. Otherwise it will give
> > the impression that newer versions does not support "NO indexing". Hope it
> helps.
> >
> Maybe, this information can be revealed in the name of cache info structure
> that describes the new cache. Thus it can be reused in newer versions (v4 and
> etc) and show info about changes. This, of course, will not work well for new
> processor models, but as I see, the new model there is created with unset
> complex_indexing

Adding special aliases is a problem. It causes more confusion and future 
maintenance issues. We feel it is better not to add any special aliases in this 
case.
Will plan to send v4 with addressing other comments.
Thanks
Babu