Re: [PATCH v2 4/6] spapr_numa: change reference-points and maxdomain settings

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

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

On Thu, Sep 24, 2020 at 04:50:56PM -0300, Daniel Henrique Barboza wrote:
> This is the first guest visible change introduced in
> spapr_numa.c. The previous settings of both reference-points
> and maxdomains were too restrictive, but enough for the
> existing associativity we're setting in the resources.
> 
> We'll change that in the following patches, populating the
> associativity arrays based on user input. For those changes
> to be effective, reference-points and maxdomains must be
> more flexible. After this patch, we'll have 4 distinct
> levels of NUMA (0x4, 0x3, 0x2, 0x1) and maxdomains will
> allow for any type of configuration the user intends to
> do - under the scope and limitations of PAPR itself, of
> course.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Although..

> ---
>  hw/ppc/spapr_numa.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 990a5fce08..ea33439a15 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -222,24 +222,48 @@ int spapr_numa_write_assoc_lookup_arrays(SpaprMachineState *spapr, void *fdt,
>   */
>  void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
>  {
> +    MachineState *ms = MACHINE(spapr);
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      uint32_t refpoints[] = {
>          cpu_to_be32(0x4),
> -        cpu_to_be32(0x4),
> +        cpu_to_be32(0x3),
>          cpu_to_be32(0x2),
> +        cpu_to_be32(0x1),
>      };
>      uint32_t nr_refpoints = ARRAY_SIZE(refpoints);
> -    uint32_t maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
> +    uint32_t maxdomain = ms->numa_state->num_nodes + spapr->gpu_numa_id;
>      uint32_t maxdomains[] = {
>          cpu_to_be32(4),
> -        maxdomain,
> -        maxdomain,
> -        maxdomain,
> -        cpu_to_be32(spapr->gpu_numa_id),
> +        cpu_to_be32(maxdomain),
> +        cpu_to_be32(maxdomain),
> +        cpu_to_be32(maxdomain),
> +        cpu_to_be32(maxdomain)
>      };
>  
> -    if (smc->pre_5_1_assoc_refpoints) {
> -        nr_refpoints = 2;
> +    if (spapr_machine_using_legacy_numa(spapr)) {
> +        uint32_t legacy_refpoints[] = {
> +            cpu_to_be32(0x4),
> +            cpu_to_be32(0x4),
> +            cpu_to_be32(0x2),
> +        };
> +        uint32_t legacy_maxdomain = spapr->gpu_numa_id > 1 ? 1 : 0;
> +        uint32_t legacy_maxdomains[] = {
> +            cpu_to_be32(4),
> +            cpu_to_be32(legacy_maxdomain),
> +            cpu_to_be32(legacy_maxdomain),
> +            cpu_to_be32(legacy_maxdomain),
> +            cpu_to_be32(spapr->gpu_numa_id),
> +        };
> +
> +        nr_refpoints = 3;
> +
> +        memcpy(refpoints, legacy_refpoints, sizeof(legacy_refpoints));
> +        memcpy(maxdomains, legacy_maxdomains, sizeof(legacy_maxdomains));

It would be nice to have a G_STATIC_ASSERT() or QEMU_BUILD_BUG_MSG()
ro ensure that the two structures are the same size, if they became
different the memcpy is wildly unsafe.

> +        /* pseries-5.0 and older reference-points array is {0x4, 0x4} */
> +        if (smc->pre_5_1_assoc_refpoints) {
> +            nr_refpoints = 2;
> +        }
>      }
>  
>      _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",

-- 
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 --]