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

Eduardo Habkost
2018-06-05T01:59:28Z

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 <babu.moger@amd.com>
> ---
> 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 <ehabkost@redhat.com>

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?

--
Eduardo