Re: [PATCH v2 1/1] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()

[Greg Kurz <groug@kaod.org>]

On Wed, 22 Sep 2021 09:28:52 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> This patch has a handful of modifications for the recent added
> FORM2 support:
> 
> - to not allocate more than the necessary size in 'distance_table'.
> At this moment the array is oversized due to allocating uint32_t for
> all elements, when most of them fits in an uint8_t. Fix it by
> changing the array to uint8_t and allocating the exact size;
> 
> - use stl_be_p() to store the uint32_t at the start of 'distance_table';
> 
> - use sizeof(uint32_t) to skip the uint32_t length when populating the
> distances;
> 
> - use the NUMA_DISTANCE_MIN macro from sysemu/numa.h to avoid hardcoding
> the local distance value.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_numa.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 58d5dc7084..5822938448 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -502,9 +502,8 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>      int nb_numa_nodes = ms->numa_state->num_nodes;
>      int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>      g_autofree uint32_t *lookup_index_table = NULL;
> -    g_autofree uint32_t *distance_table = NULL;
> +    g_autofree uint8_t *distance_table = NULL;
>      int src, dst, i, distance_table_size;
> -    uint8_t *node_distances;
>  
>      /*
>       * ibm,numa-lookup-index-table: array with length and a
> @@ -531,11 +530,13 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>       * array because NUMA ids can be sparse (node 0 is the first,
>       * node 8 is the second ...).
>       */
> -    distance_table = g_new0(uint32_t, distance_table_entries + 1);
> -    distance_table[0] = cpu_to_be32(distance_table_entries);

I personally liked the comment from v1:

+    /*
+     * Distance table is an uint8_t array with a leading uint32_t
+     * containing its length.
+     */

but I guess the code is simple enough to understand that, so :

Reviewed-by: Greg Kurz <groug@kaod.org>

> +    distance_table_size = distance_table_entries * sizeof(uint8_t) +
> +                          sizeof(uint32_t);
> +    distance_table = g_new0(uint8_t, distance_table_size);
> +    stl_be_p(distance_table, distance_table_entries);
>  
> -    node_distances = (uint8_t *)&distance_table[1];
> -    i = 0;
> +    /* Skip the uint32_t array length at the start */
> +    i = sizeof(uint32_t);
>  
>      for (src = 0; src < nb_numa_nodes; src++) {
>          for (dst = 0; dst < nb_numa_nodes; dst++) {
> @@ -546,16 +547,14 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>               * adding the numa_info to retrieve distance info from.
>               */
>              if (src == dst) {
> -                node_distances[i++] = 10;
> +                distance_table[i++] = NUMA_DISTANCE_MIN;
>                  continue;
>              }
>  
> -            node_distances[i++] = numa_info[src].distance[dst];
> +            distance_table[i++] = numa_info[src].distance[dst];
>          }
>      }
>  
> -    distance_table_size = distance_table_entries * sizeof(uint8_t) +
> -                          sizeof(uint32_t);
>      _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
>                       distance_table, distance_table_size));
>  }