Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Monday, June 4, 2018 8:59 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor
> Cache Information for cpuid 0x8000001D
> 
> On Thu, May 24, 2018 at 11:43:31AM -0400, Babu Moger wrote:
> > Add information for cpuid 0x8000001D leaf. Populate cache topology
> information
> > for different cache types (Data Cache, Instruction Cache, L2 and L3)
> supported
> > by 0x8000001D leaf. Please refer to the Processor Programming Reference
> (PPR)
> > for AMD Family 17h Model for more details.
> >
> > Signed-off-by: Babu Moger <address@hidden>
> > ---
> >  target/i386/cpu.c | 117
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  target/i386/kvm.c |  29 ++++++++++++--
> >  2 files changed, 143 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 5c9bdc9..0d423e5 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -337,6 +337,99 @@ static void
> encode_cache_cpuid80000006(CPUCacheInfo *l2,
> >  }
> >
> >  /*
> > + * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> > + * Please refer to the AMD64 Architecture Programmer’s Manual Volume
> 3.
> > + * Define the constants to build the cpu topology. Right now, TOPOEXT
> > + * feature is enabled only on EPYC. So, these constants are based on
> > + * EPYC supported configurations. We may need to handle the cases if
> > + * these values change in future.
> > + */
> > +/* Maximum core complexes in a node */
> > +#define MAX_CCX 2
> > +/* Maximum cores in a core complex */
> > +#define MAX_CORES_IN_CCX 4
> > +/* Maximum cores in a node */
> > +#define MAX_CORES_IN_NODE 8
> > +/* Maximum nodes in a socket */
> > +#define MAX_NODES_PER_SOCKET 4
> > +
> > +/*
> > + * Figure out the number of nodes required to build this config.
> > + * Max cores in a node is 4
> > + */
> > +static int nodes_in_socket(int nr_cores)
> > +{
> > +    int nodes;
> > +
> > +    nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> > +
> > +   /* Hardware does not support config with 3 nodes, return 4 in that case
> */
> > +    return (nodes == 3) ? 4 : nodes;
> > +}
> > +
> > +/*
> > + * Decide the number of cores in a core complex with the given nr_cores
> using
> > + * following set constants MAX_CCX, MAX_CORES_IN_CCX,
> MAX_CORES_IN_NODE and
> > + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> > + * L3 cache is shared across all cores in a core complex. So, this will 
> > also
> > + * tell us how many cores are sharing the L3 cache.
> > + */
> > +static int cores_in_core_complex(int nr_cores)
> > +{
> > +    int nodes;
> > +
> > +    /* Check if we can fit all the cores in one core complex */
> > +    if (nr_cores <= MAX_CORES_IN_CCX) {
> > +        return nr_cores;
> > +    }
> > +    /* Get the number of nodes required to build this config */
> > +    nodes = nodes_in_socket(nr_cores);
> > +
> > +    /*
> > +     * Divide the cores accros all the core complexes
> > +     * Return rounded up value
> > +     */
> > +    return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> > +}
> 
> It's nice that we try to figure out a good default even on weird
> topologies:
> 
> Reviewed-by: Eduardo Habkost <address@hidden>
Thanks
> 
> But I still worry about the complexity of compat code in the
> future if we decide to change the results of this function a
> little bit.  Maybe we should allow TOPOEXT to be enabled only on
> cases where we really know how to fit the cores in a round number
> of nodes/core-complexes?
> 
> Let's do this in a follow-up patch?

I thought about this earlier.  But dropped the idea because it will be too 
restrictive. 
Most of the odd number of cores cannot be supported by real hardware.
If you strongly believe we need to add this check then I will add it.
 
> 
> --
> Eduardo