[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] spapr: Add associativity reference point count to mac
From: |
David Gibson |
Subject: |
Re: [PATCH v2 1/2] spapr: Add associativity reference point count to machine info |
Date: |
Thu, 21 May 2020 15:12:49 +1000 |
On Thu, May 21, 2020 at 01:34:37AM +0200, Greg Kurz wrote:
> On Mon, 18 May 2020 16:44:17 -0500
> Reza Arbab <address@hidden> 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 <address@hidden>
> > ---
> > 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
signature.asc
Description: PGP signature