[PATCH v2 0/1] fixes in spapr_numa_FORM2_write_rtas_tables()

[PATCH v2 0/1] fixes in spapr_numa_FORM2_write_rtas_tables()

[Daniel Henrique Barboza]

Hi,

This new version contains suggestions from Greg, Phillipe and Zoltan
that were made in the v1.

Changes from v1:
- keep the heap allocation of both arrays;
- use stl_be_p();
- use sizeof(uint32_t) instead of hardcoding '4' when skipping the
length;
- use the existing NUMA_DISTANCE_MIN macro.
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05464.html


Daniel Henrique Barboza (1):
  spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()

 hw/ppc/spapr_numa.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
2.31.1

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

[Daniel Henrique Barboza]

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);
+    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));
 }
-- 
2.31.1

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

[Greg Kurz]

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));
>  }

Re: [PATCH v2 0/1] fixes in spapr_numa_FORM2_write_rtas_tables()

[David Gibson]

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

On Wed, Sep 22, 2021 at 09:28:51AM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> This new version contains suggestions from Greg, Phillipe and Zoltan
> that were made in the v1.

Applied to ppc-for-6.2, thanks.

> 
> Changes from v1:
> - keep the heap allocation of both arrays;
> - use stl_be_p();
> - use sizeof(uint32_t) instead of hardcoding '4' when skipping the
> length;
> - use the existing NUMA_DISTANCE_MIN macro.
> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05464.html
> 
> 
> Daniel Henrique Barboza (1):
>   spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
> 
>  hw/ppc/spapr_numa.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 

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