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

[Greg Kurz <groug@kaod.org>]

On Wed, 23 Sep 2020 16:34:56 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> 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>
> ---
>  hw/ppc/spapr_numa.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 180800b2f3..688391278e 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -222,21 +222,30 @@ 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 maxdomains[] = {
> -        cpu_to_be32(4),
> -        maxdomain,
> -        maxdomain,
> -        maxdomain,
> -        cpu_to_be32(spapr->gpu_numa_id),
> -    };
> +    uint32_t maxdomain = cpu_to_be32(ms->numa_state->num_nodes +
> +                                     spapr->gpu_numa_id);
> +    uint32_t maxdomains[] = {0x4, maxdomain, maxdomain, maxdomain, maxdomain};
> +

It seems maxdomains[0] should be cpu_to_be32(0x4) and spaces are missing.

Maybe keep the previous multi-line declaration style ? This seems to produce
a nicer diff for the reviewer:

     uint32_t maxdomains[] = {
         cpu_to_be32(4),
         maxdomain,
         maxdomain,
         maxdomain,
-        cpu_to_be32(spapr->gpu_numa_id),
+        maxdomain,
     };

> +    if (spapr_machine_using_legacy_numa(spapr)) {
> +        refpoints[1] =  cpu_to_be32(0x4);
> +        refpoints[2] =  cpu_to_be32(0x2);

I'd rather have an explicit view of the legacy layouts for clarity...

> +        nr_refpoints = 3;
> +
> +        maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
> +        maxdomains[1] = maxdomain;
> +        maxdomains[2] = maxdomain;
> +        maxdomains[3] = maxdomain;
> +        maxdomains[4] = cpu_to_be32(spapr->gpu_numa_id);

... and here.

eg.

    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 = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
        uint32_t legacy_maxdomains[] = {
            cpu_to_be32(4),
            legacy_maxdomain,
            legacy_maxdomain,
            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));
    }

This allows to instantly see how things are expected to appear
in the FDT, without having to mentally patch the refpoints[] and
maxdomains[] arrays. This also makes the diff easier to review.

> +    }
>  
>      if (smc->pre_5_1_assoc_refpoints) {
>          nr_refpoints = 2;