Re: [PATCH 07/10] spapr: create helper to set ibm,associativity

[David Gibson]

On Fri, Aug 14, 2020 at 05:54:21PM -0300, Daniel Henrique Barboza wrote:
> We have several places around hw/ppc files where we use the
> same code to set the ibm,associativity array. This patch
> creates a helper called spapr_set_associativity() to do
> that in a single place. It'll also make it saner to change
> the value of ibm,associativity in the next patches.
> 
> After this patch, only 2 places are left with open code
> ibm,associativity assignment:
> 
> - spapr_dt_dynamic_reconfiguration_memory()
> - h_home_node_associativity() in spapr_hcall.c
> 
> The update of associativity values will be made in these places
> manually later on.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

I like this - any chance you could move this to the front of the
series so that we can make this code easier to follow while we're
still discussing the more meaningful changes?

> ---
>  hw/ppc/spapr.c         | 32 +++++++++++++++++++++-----------
>  hw/ppc/spapr_nvdimm.c  |  8 +++-----
>  hw/ppc/spapr_pci.c     |  8 +++-----
>  include/hw/ppc/spapr.h |  1 +
>  4 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bc51d2db90..b80a6f6936 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -201,15 +201,27 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int 
> offset, PowerPCCPU *cpu,
>      return ret;
>  }
>  
> +void spapr_set_associativity(uint32_t *assoc, int node_id, int cpu_index)
> +{
> +    uint8_t assoc_size = 0x4;
> +
> +    if (cpu_index >= 0) {
> +        assoc_size = 0x5;
> +        assoc[5] = cpu_to_be32(cpu_index);
> +    }
> +
> +    assoc[0] = cpu_to_be32(assoc_size);
> +    assoc[1] = cpu_to_be32(0x0);
> +    assoc[2] = cpu_to_be32(0x0);
> +    assoc[3] = cpu_to_be32(0x0);
> +    assoc[4] = cpu_to_be32(node_id);
> +}
> +
>  static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>  {
>      int index = spapr_get_vcpu_id(cpu);
> -    uint32_t associativity[] = {cpu_to_be32(0x5),
> -                                cpu_to_be32(0x0),
> -                                cpu_to_be32(0x0),
> -                                cpu_to_be32(0x0),
> -                                cpu_to_be32(cpu->node_id),
> -                                cpu_to_be32(index)};
> +    uint32_t associativity[6];
> +    spapr_set_associativity(associativity, cpu->node_id, index);
>  
>      /* Advertise NUMA via ibm,associativity */
>      return fdt_setprop(fdt, offset, "ibm,associativity", associativity,
> @@ -325,15 +337,13 @@ static void add_str(GString *s, const gchar *s1)
>  static int spapr_dt_memory_node(void *fdt, int nodeid, hwaddr start,
>                                  hwaddr size)
>  {
> -    uint32_t associativity[] = {
> -        cpu_to_be32(0x4), /* length */
> -        cpu_to_be32(0x0), cpu_to_be32(0x0),
> -        cpu_to_be32(0x0), cpu_to_be32(nodeid)
> -    };
> +    uint32_t associativity[5];
>      char mem_name[32];
>      uint64_t mem_reg_property[2];
>      int off;
>  
> +    spapr_set_associativity(associativity, nodeid, -1);
> +
>      mem_reg_property[0] = cpu_to_be64(start);
>      mem_reg_property[1] = cpu_to_be64(size);
>  
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 81410aa63f..bd109bfc00 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -115,15 +115,13 @@ int spapr_dt_nvdimm(void *fdt, int parent_offset,
>                                               &error_abort);
>      uint64_t slot = object_property_get_uint(OBJECT(nvdimm), 
> PC_DIMM_SLOT_PROP,
>                                               &error_abort);
> -    uint32_t associativity[] = {
> -        cpu_to_be32(0x4), /* length */
> -        cpu_to_be32(0x0), cpu_to_be32(0x0),
> -        cpu_to_be32(0x0), cpu_to_be32(node)
> -    };
> +    uint32_t associativity[5];
>      uint64_t lsize = nvdimm->label_size;
>      uint64_t size = object_property_get_int(OBJECT(nvdimm), 
> PC_DIMM_SIZE_PROP,
>                                              NULL);
>  
> +    spapr_set_associativity(associativity, node, -1);
> +
>      drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
>      g_assert(drc);
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 09ac58fd7f..c02ace226c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2321,11 +2321,8 @@ int spapr_dt_phb(SpaprMachineState *spapr, 
> SpaprPhbState *phb,
>          cpu_to_be32(1),
>          cpu_to_be32(RTAS_IBM_RESET_PE_DMA_WINDOW)
>      };
> -    uint32_t associativity[] = {cpu_to_be32(0x4),
> -                                cpu_to_be32(0x0),
> -                                cpu_to_be32(0x0),
> -                                cpu_to_be32(0x0),
> -                                cpu_to_be32(phb->numa_node)};
> +    uint32_t associativity[5];
> +
>      SpaprTceTable *tcet;
>      SpaprDrc *drc;
>      Error *err = NULL;
> @@ -2358,6 +2355,7 @@ int spapr_dt_phb(SpaprMachineState *spapr, 
> SpaprPhbState *phb,
>  
>      /* Advertise NUMA via ibm,associativity */
>      if (phb->numa_node != -1) {
> +        spapr_set_associativity(associativity, phb->numa_node, -1);
>          _FDT(fdt_setprop(fdt, bus_off, "ibm,associativity", associativity,
>                           sizeof(associativity)));
>      }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d9f1afa8b2..cd158bf95a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -863,6 +863,7 @@ int spapr_phb_dt_populate(SpaprDrc *drc, 
> SpaprMachineState *spapr,
>  
>  void spapr_rtc_read(SpaprRtcState *rtc, struct tm *tm, uint32_t *ns);
>  int spapr_rtc_import_offset(SpaprRtcState *rtc, int64_t legacy_offset);
> +void spapr_set_associativity(uint32_t *assoc, int node_id, int cpu_index);
>  
>  #define TYPE_SPAPR_RNG "spapr-rng"
>  

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

signature.asc
Description: PGP signature