Re: [PATCH v2 3/7] spapr: introduce SpaprMachineClass::numa_assoc_array

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

On Tue, Sep 01, 2020 at 09:56:41AM -0300, Daniel Henrique Barboza wrote:
> The next step to centralize all NUMA/associativity handling in
> the spapr machine is to create a 'one stop place' for all
> things ibm,associativity.
> 
> This patch introduces numa_assoc_array, a 2 dimensional array
> that will store all ibm,associativity arrays of all NUMA nodes.
> This array is initialized in a new spapr_numa_associativity_init()
> function, called in spapr_machine_init(). It is being initialized
> with the same values used in other ibm,associativity properties
> around spapr files (i.e. all zeros, last value is node_id).
> The idea is to remove all hardcoded definitions and FDT writes
> of ibm,associativity arrays, doing instead a call to the new
> helper spapr_numa_write_associativity_dt() helper, that will
> be able to write the DT with the correct values.
> 
> We'll start small, handling the trivial cases first. The
> remaining instances of ibm,associativity will be handled
> next.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

The idea is great, but there's one small but significant problem here:

> +void spapr_numa_associativity_init(MachineState *machine)
> +{
> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> +    int nb_numa_nodes = machine->numa_state->num_nodes;
> +    int i;
> +
> +    /*
> +     * For all associativity arrays: first position is the size,
> +     * position MAX_DISTANCE_REF_POINTS is always the numa_id,
> +     * represented by the index 'i'.
> +     *
> +     * This will break on sparse NUMA setups, when/if QEMU starts
> +     * to support it, because there will be no more guarantee that
> +     * 'i' will be a valid node_id set by the user.
> +     */
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        smc->numa_assoc_array[i][0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS);
> +        smc->numa_assoc_array[i][MAX_DISTANCE_REF_POINTS] = cpu_to_be32(i);

This initialization is called on a machine *instance*, which means it
should treat the machine class as read-only.  i.e. the
numa_assoc_array should be in the SpaprMachineState, rather than the
class.

I mean, we'd get away with it in practice, since there's only ever
likely to be a single machine instance, but still we should correct
this.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]