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

[Maksim Davydov]

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

> > "EPYC-v2 <-> AMD EPYC Processor (with IBPB) <-> EPYC-IBPB"
> > > +                    { /* end of list */ }
> > > +                },
> > > +                .cache_info = &epyc_rome_v3_cache_info
> > > +            },
> > >                { /* end of list */ }
> > >            }
> > >        },

--
Best regards,
Maksim Davydov