| Message ID | 20220408141550.1271295-23-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: Implement GICv4 | expand |
On 4/8/22 07:15, Peter Maydell wrote: > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index 7c75dd6f072..9f1fe09a78e 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -442,8 +442,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data, > * in the memory map); if so then the GIC has multiple MemoryRegions > * for the redistributors. > */ > - cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE; > - offset %= GICV3_REDIST_SIZE; > + cpuidx = region->cpuidx + offset / gicv3_redist_size(s); > + offset %= gicv3_redist_size(s); > > cs = &s->cpu[cpuidx]; > > @@ -501,8 +501,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > * in the memory map); if so then the GIC has multiple MemoryRegions > * for the redistributors. > */ > - cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE; > - offset %= GICV3_REDIST_SIZE; > + cpuidx = region->cpuidx + offset / gicv3_redist_size(s); > + offset %= gicv3_redist_size(s); In these two cases, it would be better to call the function only once. Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Sat, 9 Apr 2022 at 19:32, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/8/22 07:15, Peter Maydell wrote: > > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > > index 7c75dd6f072..9f1fe09a78e 100644 > > --- a/hw/intc/arm_gicv3_redist.c > > +++ b/hw/intc/arm_gicv3_redist.c > > @@ -442,8 +442,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data, > > * in the memory map); if so then the GIC has multiple MemoryRegions > > * for the redistributors. > > */ > > - cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE; > > - offset %= GICV3_REDIST_SIZE; > > + cpuidx = region->cpuidx + offset / gicv3_redist_size(s); > > + offset %= gicv3_redist_size(s); > > > > cs = &s->cpu[cpuidx]; > > > > @@ -501,8 +501,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > > * in the memory map); if so then the GIC has multiple MemoryRegions > > * for the redistributors. > > */ > > - cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE; > > - offset %= GICV3_REDIST_SIZE; > > + cpuidx = region->cpuidx + offset / gicv3_redist_size(s); > > + offset %= gicv3_redist_size(s); > > In these two cases, it would be better to call the function only once. The compiler is smart enough to only actually call the function once (and thus to do the / and % with a single div and imul), so I don't think that explicitly adding a local variable and doing the call-once by hand is really worthwhile. thanks -- PMM
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 8d58d38836f..9720ccf7507 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -489,6 +489,27 @@ FIELD(VTE, RDBASE, 42, RDBASE_PROCNUM_LENGTH) /* Functions internal to the emulated GICv3 */ +/** + * gicv3_redist_size: + * @s: GICv3State + * + * Return the size of the redistributor register frame in bytes + * (which depends on what GIC version this is) + */ +static inline int gicv3_redist_size(GICv3State *s) +{ + /* + * Redistributor size is controlled by the redistributor GICR_TYPER.VLPIS. + * It's the same for every redistributor in the GIC, so arbitrarily + * use the register field in the first one. + */ + if (s->cpu[0].gicr_typer & GICR_TYPER_VLPIS) { + return GICV4_REDIST_SIZE; + } else { + return GICV3_REDIST_SIZE; + } +} + /** * gicv3_intid_is_special: * @intid: interrupt ID diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index 08b27789385..40bc404a652 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -38,7 +38,12 @@ #define GICV3_LPI_INTID_START 8192 +/* + * The redistributor in GICv3 has two 64KB frames per CPU; in + * GICv4 it has four 64KB frames per CPU. + */ #define GICV3_REDIST_SIZE 0x20000 +#define GICV4_REDIST_SIZE 0x40000 /* Number of SGI target-list bits */ #define GICV3_TARGETLIST_BITS 16 diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index dcc5ce28c6a..18999e3c8bb 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -295,7 +295,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, memory_region_init_io(®ion->iomem, OBJECT(s), ops ? &ops[1] : NULL, region, name, - s->redist_region_count[i] * GICV3_REDIST_SIZE); + s->redist_region_count[i] * gicv3_redist_size(s)); sysbus_init_mmio(sbd, ®ion->iomem); g_free(name); } diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c index 7c75dd6f072..9f1fe09a78e 100644 --- a/hw/intc/arm_gicv3_redist.c +++ b/hw/intc/arm_gicv3_redist.c @@ -442,8 +442,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data, * in the memory map); if so then the GIC has multiple MemoryRegions * for the redistributors. */ - cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE; - offset %= GICV3_REDIST_SIZE; + cpuidx = region->cpuidx + offset / gicv3_redist_size(s); + offset %= gicv3_redist_size(s); cs = &s->cpu[cpuidx]; @@ -501,8 +501,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, * in the memory map); if so then the GIC has multiple MemoryRegions * for the redistributors. */ - cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE; - offset %= GICV3_REDIST_SIZE; + cpuidx = region->cpuidx + offset / gicv3_redist_size(s); + offset %= gicv3_redist_size(s); cs = &s->cpu[cpuidx];
The GICv4 extends the redistributor register map -- where GICv3 had two 64KB frames per CPU, GICv4 has four frames. Add support for the extra frame by using a new gicv3_redist_size() function in the places in the GIC implementation which currently use a fixed constant size for the redistributor register block. (Until we implement the extra registers they will RAZ/WI.) Any board that wants to use a GICv4 will need to also adjust to handle the different sized redistributor register block; that will be done separately. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/gicv3_internal.h | 21 +++++++++++++++++++++ include/hw/intc/arm_gicv3_common.h | 5 +++++ hw/intc/arm_gicv3_common.c | 2 +- hw/intc/arm_gicv3_redist.c | 8 ++++---- 4 files changed, 31 insertions(+), 5 deletions(-)