Re: [PATCH 08/10] spapr: introduce SpaprMachineClass::numa_assoc_domains

[David Gibson]

On Fri, Aug 14, 2020 at 05:54:22PM -0300, Daniel Henrique Barboza wrote:
> We can't use the input from machine->numa_state->nodes directly
> in the pSeries machine because PAPR does not work with raw distance
> values, like ACPI SLIT does. We need to determine common
> associativity domains, based on similar performance/distance of the
> resources, and set these domains in the associativy array that goes
> to the FDT of each resource.
> 
> To ease the translation between regular ACPI NUMA distance info
> to our PAPR dialect, let's create a matrix called numa_assoc_domains
> in the SpaprMachineClass. This matrix will be initiated during
> machine init, where  we will read NUMA information from user input,
> apply a heuristic to determine the associativity domains for each node,
> then populate numa_assoc_domains accordingly.
> 
> The changes are mostly centered in the spapr_set_associativity()
> helper that will use the values of numa_assoc_domains instead of
> using 0x0, with spapr_dt_dynamic_reconfiguration_memory() and
> h_home_node_associativity() being the exceptions.
> 
> To keep the changes under control, we'll plug in the matrix usage
> in the existing code first. The actual heuristic to determine
> the associativity domains for each NUMA node will come in a follow-up
> patch.
> 
> Note that the matrix is initiated with zeros, meaning that there is
> no guest changes implemented in this patch. We'll keep these
> changes from legacy NUMA guests by not initiating the matrix
> in these cases.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

IIUC, what this is basically doing is that instead of doing the
translation from qemu's internal NUMA representation to PAPRs at the
point(s) we emit the PAPR information, we're moving it to persistent
data we calculate for each (qemu) numa node then just copy out when we
emit the information?

That could be a reasonable idea, and indeed the rest of the series
might be easier to understand if this change were earlier in the
series.  In particular it means we might be able localise all the
hacks for calculating the right vectors depending on machine
version/options into one place.

A couple of nits, though:

> ---
>  hw/ppc/spapr.c                | 46 +++++++++++++++++++++++------------
>  hw/ppc/spapr_hcall.c          | 13 ++++++++--
>  hw/ppc/spapr_nvdimm.c         | 13 +++++-----
>  hw/ppc/spapr_pci.c            |  3 ++-
>  include/hw/ppc/spapr.h        |  7 +++++-
>  include/hw/ppc/spapr_nvdimm.h |  5 ++--
>  6 files changed, 59 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b80a6f6936..4f50ab21ee 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -201,8 +201,13 @@ 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)
> +void spapr_set_associativity(uint32_t *assoc, int node_id, int cpu_index,
> +                             MachineState *machine)
>  {
> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> +    uint8_t assoc_domain1 = smc->numa_assoc_domains[node_id][0];
> +    uint8_t assoc_domain2 = smc->numa_assoc_domains[node_id][1];
> +    uint8_t assoc_domain3 = smc->numa_assoc_domains[node_id][2];
>      uint8_t assoc_size = 0x4;
>  
>      if (cpu_index >= 0) {
> @@ -211,17 +216,18 @@ void spapr_set_associativity(uint32_t *assoc, int 
> node_id, int 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[1] = cpu_to_be32(assoc_domain1);
> +    assoc[2] = cpu_to_be32(assoc_domain2);
> +    assoc[3] = cpu_to_be32(assoc_domain3);
>      assoc[4] = cpu_to_be32(node_id);

So spapr_set_associativity() is already a slightly dangerous function,
because the required buffer space for 'assoc' varies in a non-obvious
way depending on if cpu_index is >= 0.  I didn't comment on that when
it was introduced, because it's not really any worse than what it
replaced.

But with this change, I think we can do better.  I'd suggest storing
the full PAPR associativity vector for each qemu numa node verbatim,
so it can just be copied straight into the device tree without
interpretation.  Then the helper can actually do the property set, and
we don't need magically sized locals any more.

Obviously there will need to be some more handling for the extra layer
we add on cpu assoc vectors.  We could store a vector for each vcpu as
well, or just have a hack to adjust these (fdt_appendprop() might be
useful).

>  }
>  
> -static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
> +static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu,
> +                                   MachineState *machine)
>  {
>      int index = spapr_get_vcpu_id(cpu);
>      uint32_t associativity[6];
> -    spapr_set_associativity(associativity, cpu->node_id, index);
> +    spapr_set_associativity(associativity, cpu->node_id, index, machine);
>  
>      /* Advertise NUMA via ibm,associativity */
>      return fdt_setprop(fdt, offset, "ibm,associativity", associativity,
> @@ -335,14 +341,14 @@ static void add_str(GString *s, const gchar *s1)
>  }
>  
>  static int spapr_dt_memory_node(void *fdt, int nodeid, hwaddr start,
> -                                hwaddr size)
> +                                hwaddr size, MachineState *machine)
>  {
>      uint32_t associativity[5];
>      char mem_name[32];
>      uint64_t mem_reg_property[2];
>      int off;
>  
> -    spapr_set_associativity(associativity, nodeid, -1);
> +    spapr_set_associativity(associativity, nodeid, -1, machine);
>  
>      mem_reg_property[0] = cpu_to_be64(start);
>      mem_reg_property[1] = cpu_to_be64(size);
> @@ -574,6 +580,7 @@ static int 
> spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
>                                                     void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>      int nb_numa_nodes = machine->numa_state->num_nodes;
>      int ret, i, offset;
>      uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
> @@ -628,12 +635,17 @@ static int 
> spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
>      int_buf[1] = cpu_to_be32(4); /* Number of entries per associativity list 
> */
>      cur_index += 2;
>      for (i = 0; i < nr_nodes; i++) {
> +        uint8_t assoc_domain1 = smc->numa_assoc_domains[i][0];
> +        uint8_t assoc_domain2 = smc->numa_assoc_domains[i][1];
> +        uint8_t assoc_domain3 = smc->numa_assoc_domains[i][2];
> +
>          uint32_t associativity[] = {
> -            cpu_to_be32(0x0),
> -            cpu_to_be32(0x0),
> -            cpu_to_be32(0x0),
> +            cpu_to_be32(assoc_domain1),
> +            cpu_to_be32(assoc_domain2),
> +            cpu_to_be32(assoc_domain3),
>              cpu_to_be32(i)

So again, I suggest this be copiedas a blob from a stored vector for
the node, rather than a mix of stored info and locally calculated info
(the final 'i').

>          };
> +
>          memcpy(cur_index, associativity, sizeof(associativity));
>          cur_index += 4;
>      }
> @@ -667,7 +679,7 @@ static int spapr_dt_memory(SpaprMachineState *spapr, void 
> *fdt)
>          if (!mem_start) {
>              /* spapr_machine_init() checks for rma_size <= node0_size
>               * already */
> -            spapr_dt_memory_node(fdt, i, 0, spapr->rma_size);
> +            spapr_dt_memory_node(fdt, i, 0, spapr->rma_size, machine);
>              mem_start += spapr->rma_size;
>              node_size -= spapr->rma_size;
>          }
> @@ -679,7 +691,7 @@ static int spapr_dt_memory(SpaprMachineState *spapr, void 
> *fdt)
>                  sizetmp = 1ULL << ctzl(mem_start);
>              }
>  
> -            spapr_dt_memory_node(fdt, i, mem_start, sizetmp);
> +            spapr_dt_memory_node(fdt, i, mem_start, sizetmp, machine);
>              node_size -= sizetmp;
>              mem_start += sizetmp;
>          }
> @@ -809,7 +821,7 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int 
> offset,
>                        pft_size_prop, sizeof(pft_size_prop))));
>  
>      if (ms->numa_state->num_nodes > 1) {
> -        _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu));
> +        _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu, ms));
>      }
>  
>      _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt));
> @@ -1335,7 +1347,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool 
> reset, size_t space)
>  
>      /* NVDIMM devices */
>      if (mc->nvdimm_supported) {
> -        spapr_dt_persistent_memory(fdt);
> +        spapr_dt_persistent_memory(fdt, machine);
>      }
>  
>      return fdt;
> @@ -3453,6 +3465,7 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error 
> **errp)
>  int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>                            void *fdt, int *fdt_start_offset, Error **errp)
>  {
> +    MachineState *machine = MACHINE(spapr);
>      uint64_t addr;
>      uint32_t node;
>  
> @@ -3460,7 +3473,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, 
> SpaprMachineState *spapr,
>      node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
>                                      &error_abort);
>      *fdt_start_offset = spapr_dt_memory_node(fdt, node, addr,
> -                                             SPAPR_MEMORY_BLOCK_SIZE);
> +                                             SPAPR_MEMORY_BLOCK_SIZE,
> +                                             machine);
>      return 0;
>  }
>  
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index c1d01228c6..6e94e513cf 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1878,10 +1878,15 @@ static target_ulong 
> h_home_node_associativity(PowerPCCPU *cpu,
>                                                target_ulong opcode,
>                                                target_ulong *args)
>  {
> +    MachineState *machine = MACHINE(spapr);
> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>      target_ulong flags = args[0];
>      target_ulong procno = args[1];
>      PowerPCCPU *tcpu;
>      int idx;
> +    uint8_t assoc_domain1;
> +    uint8_t assoc_domain2;
> +    uint8_t assoc_domain3;
>  
>      /* only support procno from H_REGISTER_VPA */
>      if (flags != 0x1) {
> @@ -1893,13 +1898,17 @@ static target_ulong 
> h_home_node_associativity(PowerPCCPU *cpu,
>          return H_P2;
>      }
>  
> +    assoc_domain1 = smc->numa_assoc_domains[tcpu->node_id][0];
> +    assoc_domain2 = smc->numa_assoc_domains[tcpu->node_id][1];
> +    assoc_domain3 = smc->numa_assoc_domains[tcpu->node_id][2];
> +
>      /* sequence is the same as in the "ibm,associativity" property */
>  
>      idx = 0;
>  #define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
>                               ((uint64_t)(b) & 0xffffffff))
> -    args[idx++] = ASSOCIATIVITY(0, 0);
> -    args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
> +    args[idx++] = ASSOCIATIVITY(assoc_domain1, assoc_domain2);
> +    args[idx++] = ASSOCIATIVITY(assoc_domain3, tcpu->node_id);
>      args[idx++] = ASSOCIATIVITY(procno, -1);
>      for ( ; idx < 6; idx++) {
>          args[idx] = -1;
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index bd109bfc00..7f1f088c39 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -87,8 +87,9 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState 
> *spapr,
>                             void *fdt, int *fdt_start_offset, Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(drc->dev);
> +    MachineState *machine = MACHINE(spapr);
>  
> -    *fdt_start_offset = spapr_dt_nvdimm(fdt, 0, nvdimm);
> +    *fdt_start_offset = spapr_dt_nvdimm(fdt, 0, nvdimm, machine);
>  
>      return 0;
>  }
> @@ -104,8 +105,8 @@ void spapr_create_nvdimm_dr_connectors(SpaprMachineState 
> *spapr)
>  }
>  
>  
> -int spapr_dt_nvdimm(void *fdt, int parent_offset,
> -                           NVDIMMDevice *nvdimm)
> +int spapr_dt_nvdimm(void *fdt, int parent_offset, NVDIMMDevice *nvdimm,
> +                    MachineState *machine)
>  {
>      int child_offset;
>      char *buf;
> @@ -120,7 +121,7 @@ int spapr_dt_nvdimm(void *fdt, int parent_offset,
>      uint64_t size = object_property_get_int(OBJECT(nvdimm), 
> PC_DIMM_SIZE_PROP,
>                                              NULL);
>  
> -    spapr_set_associativity(associativity, node, -1);
> +    spapr_set_associativity(associativity, node, -1, machine);
>  
>      drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
>      g_assert(drc);
> @@ -159,7 +160,7 @@ int spapr_dt_nvdimm(void *fdt, int parent_offset,
>      return child_offset;
>  }
>  
> -void spapr_dt_persistent_memory(void *fdt)
> +void spapr_dt_persistent_memory(void *fdt, MachineState *machine)
>  {
>      int offset = fdt_subnode_offset(fdt, 0, "persistent-memory");
>      GSList *iter, *nvdimms = nvdimm_get_device_list();
> @@ -177,7 +178,7 @@ void spapr_dt_persistent_memory(void *fdt)
>      for (iter = nvdimms; iter; iter = iter->next) {
>          NVDIMMDevice *nvdimm = iter->data;
>  
> -        spapr_dt_nvdimm(fdt, offset, nvdimm);
> +        spapr_dt_nvdimm(fdt, offset, nvdimm, machine);
>      }
>      g_slist_free(nvdimms);
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c02ace226c..4d9ef63f3e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2355,7 +2355,8 @@ 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);
> +        MachineState *machine = MACHINE(spapr);
> +        spapr_set_associativity(associativity, phb->numa_node, -1, machine);
>          _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 cd158bf95a..1f9700ac19 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -104,6 +104,9 @@ typedef enum {
>  
>  #define FDT_MAX_SIZE                    0x100000
>  
> +/* Taken from Linux kernel arch/powerpc/mm/numa.h */
> +#define MAX_DISTANCE_REF_POINTS         4
> +
>  typedef struct SpaprCapabilities SpaprCapabilities;
>  struct SpaprCapabilities {
>      uint8_t caps[SPAPR_CAP_NUM];
> @@ -131,6 +134,7 @@ struct SpaprMachineClass {
>      hwaddr rma_limit;          /* clamp the RMA to this size */
>      bool pre_5_1_assoc_refpoints;
>      bool pre_5_2_numa_associativity;
> +    uint8_t numa_assoc_domains[MAX_NODES][MAX_DISTANCE_REF_POINTS-1];
>  
>      void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
>                            uint64_t *buid, hwaddr *pio, 
> @@ -863,7 +867,8 @@ 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);
> +void spapr_set_associativity(uint32_t *assoc, int node_id, int cpu_index,
> +                             MachineState *machine);
>  
>  #define TYPE_SPAPR_RNG "spapr-rng"
>  
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index b3330cc485..be30773c7d 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -27,8 +27,9 @@ QEMU_BUILD_BUG_ON(SPAPR_MINIMUM_SCM_BLOCK_SIZE % 
> SPAPR_MEMORY_BLOCK_SIZE);
>  
>  int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>                             void *fdt, int *fdt_start_offset, Error **errp);
> -int spapr_dt_nvdimm(void *fdt, int parent_offset, NVDIMMDevice *nvdimm);
> -void spapr_dt_persistent_memory(void *fdt);
> +int spapr_dt_nvdimm(void *fdt, int parent_offset, NVDIMMDevice *nvdimm,
> +                    MachineState *machine);
> +void spapr_dt_persistent_memory(void *fdt, MachineState *machine);
>  void spapr_nvdimm_validate_opts(NVDIMMDevice *nvdimm, uint64_t size,
>                                  Error **errp);
>  void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);

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