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

[Eduardo Habkost <ehabkost@redhat.com>]

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