| Message ID | 20200518214418.18248-1-arbab@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] spapr: Add associativity reference point count to machine info | expand |
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[]. > _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,
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, >
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: >> @@ -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[]. I'll rework nr_assoc_refpoints into a bool as David suggested. Struggling for a name at the moment, but I'll think on it. >> _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]) Will do. >> @@ -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. No problem. I'll squash the patches together and work in that PHB compat property as well. Thanks for your feedback!
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)); _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points", - refpoints, sizeof(refpoints))); + refpoints, nr_refpoints * sizeof(uint32_t))); _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; 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,
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(-)