On Thu, May 21, 2020 at 01:34:37AM +0200, Greg Kurz wrote:
> On Mon, 18 May 2020 16:44:17 -0500
> Reza Arbab <arbab@linux.ibm.com> wrote:
> 
> > Make the number of NUMA associativity reference points a
> > machine-specific value, using the currently assumed default (two
> > reference points). This preps the next patch to conditionally change it.
> > 
> > Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> > ---
> >  hw/ppc/spapr.c         | 6 +++++-
> >  include/hw/ppc/spapr.h | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c18eab0a2305..88b4a1f17716 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -889,10 +889,12 @@ static int spapr_dt_rng(void *fdt)
> >  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> >  {
> >      MachineState *ms = MACHINE(spapr);
> > +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> >      int rtas;
> >      GString *hypertas = g_string_sized_new(256);
> >      GString *qemu_hypertas = g_string_sized_new(256);
> >      uint32_t refpoints[] = { cpu_to_be32(0x4), cpu_to_be32(0x4) };
> > +    uint32_t nr_refpoints;
> >      uint64_t max_device_addr = MACHINE(spapr)->device_memory->base +
> >          memory_region_size(&MACHINE(spapr)->device_memory->mr);
> >      uint32_t lrdr_capacity[] = {
> > @@ -944,8 +946,9 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> >                       qemu_hypertas->str, qemu_hypertas->len));
> >      g_string_free(qemu_hypertas, TRUE);
> >  
> > +    nr_refpoints = MIN(smc->nr_assoc_refpoints, ARRAY_SIZE(refpoints));
> 
> Having the machine requesting more reference points than available
> would clearly be a bug. I'd rather add an assert() than silently
> clipping to the size of refpoints[].

Actually, I think this "num reference points" thing is a false
abstraction.  It's selecting a number of entries from a list of
reference points that's fixed.  The number of things we could do
simply by changing the machine property and not the array is pretty
small.

I think it would be simpler to just have a boolean in the machine
class.

> >      _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
> > -                     refpoints, sizeof(refpoints)));
> > +                     refpoints, nr_refpoints * sizeof(uint32_t)));
> >  
> 
> Size can be expressed without yet another explicit reference to the
> uint32_t type:
> 
> nr_refpoints * sizeof(refpoints[0])
> 
> >      _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> >                       maxdomains, sizeof(maxdomains)));
> > @@ -4541,6 +4544,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >      smc->linux_pci_probe = true;
> >      smc->smp_threads_vsmt = true;
> >      smc->nr_xirqs = SPAPR_NR_XIRQS;
> > +    smc->nr_assoc_refpoints = 2;
> 
> When adding a new setting for the default machine type, we usually
> take care of older machine types at the same time, ie. folding this
> patch into the next one. Both patches are simple enough that it should
> be okay and this would avoid this line to be touched again.
> 
> >      xfc->match_nvt = spapr_match_nvt;
> >  }
> >  
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index e579eaf28c05..abaf9a92adc0 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -129,6 +129,7 @@ struct SpaprMachineClass {
> >      bool linux_pci_probe;
> >      bool smp_threads_vsmt; /* set VSMT to smp_threads by default */
> >      hwaddr rma_limit;          /* clamp the RMA to this size */
> > +    uint32_t nr_assoc_refpoints;
> >  
> >      void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> >                            uint64_t *buid, hwaddr *pio, 
> 

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