| Message ID | 20210602180042.111347-8-shashi.mallela@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GICv3 LPI and ITS feature implementation | expand |
On Wed, Jun 02, 2021 at 14:00:41 -0400, Shashi Mallela wrote: > Included creation of ITS as part of SBSA platform GIC > initialization. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > index 43c19b4923..3d9c073636 100644 > --- a/hw/arm/sbsa-ref.c > +++ b/hw/arm/sbsa-ref.c > @@ -34,7 +34,7 @@ > #include "hw/boards.h" > #include "hw/ide/internal.h" > #include "hw/ide/ahci_internal.h" > -#include "hw/intc/arm_gicv3_common.h" > +#include "hw/intc/arm_gicv3_its_common.h" > #include "hw/loader.h" > #include "hw/pci-host/gpex.h" > #include "hw/qdev-properties.h" > @@ -64,6 +64,7 @@ enum { > SBSA_CPUPERIPHS, > SBSA_GIC_DIST, > SBSA_GIC_REDIST, > + SBSA_GIC_ITS, > SBSA_SECURE_EC, > SBSA_GWDT, > SBSA_GWDT_REFRESH, > @@ -107,6 +108,7 @@ static const MemMapEntry sbsa_ref_memmap[] = { > [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, > [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, > [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, It seems customary in QEMU to flag gaps in memory space (although admittedly, we'd already failed to do so here). This patch leaves a gap of 0x00010000. Is there a particular reason? > + [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 }, And then again a gap (the one we already had). / Leif > [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, > [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, > [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, > @@ -377,7 +379,20 @@ static void create_secure_ram(SBSAMachineState *sms, > memory_region_add_subregion(secure_sysmem, base, secram); > } > > -static void create_gic(SBSAMachineState *sms) > +static void create_its(SBSAMachineState *sms) > +{ > + DeviceState *dev; > + > + dev = qdev_new(TYPE_ARM_GICV3_ITS); > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > + > + object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(sms->gic), > + &error_abort); > + sysbus_realize_and_unref(s, &error_fatal); > + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); > +} > + > +static void create_gic(SBSAMachineState *sms, MemoryRegion *mem) > { > unsigned int smp_cpus = MACHINE(sms)->smp.cpus; > SysBusDevice *gicbusdev; > @@ -404,6 +419,10 @@ static void create_gic(SBSAMachineState *sms) > qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1); > qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", redist0_count); > > + object_property_set_link(OBJECT(sms->gic), "sysmem", OBJECT(mem), > + &error_fatal); > + qdev_prop_set_bit(sms->gic, "has-lpi", true); > + > gicbusdev = SYS_BUS_DEVICE(sms->gic); > sysbus_realize_and_unref(gicbusdev, &error_fatal); > sysbus_mmio_map(gicbusdev, 0, sbsa_ref_memmap[SBSA_GIC_DIST].base); > @@ -450,6 +469,7 @@ static void create_gic(SBSAMachineState *sms) > sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, > qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ)); > } > + create_its(sms); > } > > static void create_uart(const SBSAMachineState *sms, int uart, > @@ -762,7 +782,7 @@ static void sbsa_ref_init(MachineState *machine) > > create_secure_ram(sms, secure_sysmem); > > - create_gic(sms); > + create_gic(sms, sysmem); > > create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); > create_uart(sms, SBSA_SECURE_UART, secure_sysmem, serial_hd(1)); > -- > 2.27.0 >
On Thu, 2021-06-03 at 12:42 +0100, Leif Lindholm wrote: > On Wed, Jun 02, 2021 at 14:00:41 -0400, Shashi Mallela wrote: > > Included creation of ITS as part of SBSA platform GIC > > initialization. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++--- > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > > index 43c19b4923..3d9c073636 100644 > > --- a/hw/arm/sbsa-ref.c > > +++ b/hw/arm/sbsa-ref.c > > @@ -34,7 +34,7 @@ > > #include "hw/boards.h" > > #include "hw/ide/internal.h" > > #include "hw/ide/ahci_internal.h" > > -#include "hw/intc/arm_gicv3_common.h" > > +#include "hw/intc/arm_gicv3_its_common.h" > > #include "hw/loader.h" > > #include "hw/pci-host/gpex.h" > > #include "hw/qdev-properties.h" > > @@ -64,6 +64,7 @@ enum { > > SBSA_CPUPERIPHS, > > SBSA_GIC_DIST, > > SBSA_GIC_REDIST, > > + SBSA_GIC_ITS, > > SBSA_SECURE_EC, > > SBSA_GWDT, > > SBSA_GWDT_REFRESH, > > @@ -107,6 +108,7 @@ static const MemMapEntry sbsa_ref_memmap[] = { > > [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, > > [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, > > [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, > > It seems customary in QEMU to flag gaps in memory space (although > admittedly, we'd already failed to do so here). This patch leaves a > gap of 0x00010000. Is there a particular reason? > > > + [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 }, > > And then again a gap (the one we already had). > > / > Leif > > No specific reason,but from ITS point of view tried to stay within > the GIC's 0x40060000 to 0x50000000 zone.The gap of 0x00010000 would > also account for future GIC additions like virtual LPI support. > > > > [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, > > [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, > > [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, > > @@ -377,7 +379,20 @@ static void create_secure_ram(SBSAMachineState > > *sms, > > memory_region_add_subregion(secure_sysmem, base, secram); > > } > > > > -static void create_gic(SBSAMachineState *sms) > > +static void create_its(SBSAMachineState *sms) > > +{ > > + DeviceState *dev; > > + > > + dev = qdev_new(TYPE_ARM_GICV3_ITS); > > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > > + > > + object_property_set_link(OBJECT(dev), "parent-gicv3", > > OBJECT(sms->gic), > > + &error_abort); > > + sysbus_realize_and_unref(s, &error_fatal); > > + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); > > +} > > + > > +static void create_gic(SBSAMachineState *sms, MemoryRegion *mem) > > { > > unsigned int smp_cpus = MACHINE(sms)->smp.cpus; > > SysBusDevice *gicbusdev; > > @@ -404,6 +419,10 @@ static void create_gic(SBSAMachineState *sms) > > qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1); > > qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", > > redist0_count); > > > > + object_property_set_link(OBJECT(sms->gic), "sysmem", > > OBJECT(mem), > > + &error_fatal); > > + qdev_prop_set_bit(sms->gic, "has-lpi", true); > > + > > gicbusdev = SYS_BUS_DEVICE(sms->gic); > > sysbus_realize_and_unref(gicbusdev, &error_fatal); > > sysbus_mmio_map(gicbusdev, 0, > > sbsa_ref_memmap[SBSA_GIC_DIST].base); > > @@ -450,6 +469,7 @@ static void create_gic(SBSAMachineState *sms) > > sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, > > qdev_get_gpio_in(cpudev, > > ARM_CPU_VFIQ)); > > } > > + create_its(sms); > > } > > > > static void create_uart(const SBSAMachineState *sms, int uart, > > @@ -762,7 +782,7 @@ static void sbsa_ref_init(MachineState > > *machine) > > > > create_secure_ram(sms, secure_sysmem); > > > > - create_gic(sms); > > + create_gic(sms, sysmem); > > > > create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); > > create_uart(sms, SBSA_SECURE_UART, secure_sysmem, > > serial_hd(1)); > > -- > > 2.27.0 > >
On Thu, Jun 03, 2021 at 11:31:21 -0400, shashi.mallela@linaro.org wrote: > On Thu, 2021-06-03 at 12:42 +0100, Leif Lindholm wrote: > > On Wed, Jun 02, 2021 at 14:00:41 -0400, Shashi Mallela wrote: > > > Included creation of ITS as part of SBSA platform GIC > > > initialization. > > > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > > --- > > > hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++--- > > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > > > index 43c19b4923..3d9c073636 100644 > > > --- a/hw/arm/sbsa-ref.c > > > +++ b/hw/arm/sbsa-ref.c > > > @@ -34,7 +34,7 @@ > > > #include "hw/boards.h" > > > #include "hw/ide/internal.h" > > > #include "hw/ide/ahci_internal.h" > > > -#include "hw/intc/arm_gicv3_common.h" > > > +#include "hw/intc/arm_gicv3_its_common.h" > > > #include "hw/loader.h" > > > #include "hw/pci-host/gpex.h" > > > #include "hw/qdev-properties.h" > > > @@ -64,6 +64,7 @@ enum { > > > SBSA_CPUPERIPHS, > > > SBSA_GIC_DIST, > > > SBSA_GIC_REDIST, > > > + SBSA_GIC_ITS, > > > SBSA_SECURE_EC, > > > SBSA_GWDT, > > > SBSA_GWDT_REFRESH, > > > @@ -107,6 +108,7 @@ static const MemMapEntry sbsa_ref_memmap[] = { > > > [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, > > > [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, > > > [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, > > > > It seems customary in QEMU to flag gaps in memory space (although > > admittedly, we'd already failed to do so here). This patch leaves a > > gap of 0x00010000. Is there a particular reason? > > > > > + [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 }, > > > > And then again a gap (the one we already had). > > > > No specific reason,but from ITS point of view tried to stay within > > the GIC's 0x40060000 to 0x50000000 zone.The gap of 0x00010000 would > > also account for future GIC additions like virtual LPI support. Right. I was more thinking 64kB isn't much space to extend into. Would it be worth pushing the ITS either all the way up to just below 0x50000000, 0x48000000, or 0x45000000? Either way, the gap(s) would be good to point out with comments, and potential future use. I only noticed this one on like the third pass of reading. / Leif > > > [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, > > > [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, > > > [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, > > > @@ -377,7 +379,20 @@ static void create_secure_ram(SBSAMachineState > > > *sms, > > > memory_region_add_subregion(secure_sysmem, base, secram); > > > } > > > > > > -static void create_gic(SBSAMachineState *sms) > > > +static void create_its(SBSAMachineState *sms) > > > +{ > > > + DeviceState *dev; > > > + > > > + dev = qdev_new(TYPE_ARM_GICV3_ITS); > > > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > > > + > > > + object_property_set_link(OBJECT(dev), "parent-gicv3", > > > OBJECT(sms->gic), > > > + &error_abort); > > > + sysbus_realize_and_unref(s, &error_fatal); > > > + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); > > > +} > > > + > > > +static void create_gic(SBSAMachineState *sms, MemoryRegion *mem) > > > { > > > unsigned int smp_cpus = MACHINE(sms)->smp.cpus; > > > SysBusDevice *gicbusdev; > > > @@ -404,6 +419,10 @@ static void create_gic(SBSAMachineState *sms) > > > qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1); > > > qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", > > > redist0_count); > > > > > > + object_property_set_link(OBJECT(sms->gic), "sysmem", > > > OBJECT(mem), > > > + &error_fatal); > > > + qdev_prop_set_bit(sms->gic, "has-lpi", true); > > > + > > > gicbusdev = SYS_BUS_DEVICE(sms->gic); > > > sysbus_realize_and_unref(gicbusdev, &error_fatal); > > > sysbus_mmio_map(gicbusdev, 0, > > > sbsa_ref_memmap[SBSA_GIC_DIST].base); > > > @@ -450,6 +469,7 @@ static void create_gic(SBSAMachineState *sms) > > > sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, > > > qdev_get_gpio_in(cpudev, > > > ARM_CPU_VFIQ)); > > > } > > > + create_its(sms); > > > } > > > > > > static void create_uart(const SBSAMachineState *sms, int uart, > > > @@ -762,7 +782,7 @@ static void sbsa_ref_init(MachineState > > > *machine) > > > > > > create_secure_ram(sms, secure_sysmem); > > > > > > - create_gic(sms); > > > + create_gic(sms, sysmem); > > > > > > create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); > > > create_uart(sms, SBSA_SECURE_UART, secure_sysmem, > > > serial_hd(1)); > > > -- > > > 2.27.0 > > > >
On Fri, 2021-06-04 at 11:42 +0100, Leif Lindholm wrote: > On Thu, Jun 03, 2021 at 11:31:21 -0400, shashi.mallela@linaro.org > wrote: > > On Thu, 2021-06-03 at 12:42 +0100, Leif Lindholm wrote: > > > On Wed, Jun 02, 2021 at 14:00:41 -0400, Shashi Mallela wrote: > > > > Included creation of ITS as part of SBSA platform GIC > > > > initialization. > > > > > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > > > --- > > > > hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++--- > > > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > > > > index 43c19b4923..3d9c073636 100644 > > > > --- a/hw/arm/sbsa-ref.c > > > > +++ b/hw/arm/sbsa-ref.c > > > > @@ -34,7 +34,7 @@ > > > > #include "hw/boards.h" > > > > #include "hw/ide/internal.h" > > > > #include "hw/ide/ahci_internal.h" > > > > -#include "hw/intc/arm_gicv3_common.h" > > > > +#include "hw/intc/arm_gicv3_its_common.h" > > > > #include "hw/loader.h" > > > > #include "hw/pci-host/gpex.h" > > > > #include "hw/qdev-properties.h" > > > > @@ -64,6 +64,7 @@ enum { > > > > SBSA_CPUPERIPHS, > > > > SBSA_GIC_DIST, > > > > SBSA_GIC_REDIST, > > > > + SBSA_GIC_ITS, > > > > SBSA_SECURE_EC, > > > > SBSA_GWDT, > > > > SBSA_GWDT_REFRESH, > > > > @@ -107,6 +108,7 @@ static const MemMapEntry sbsa_ref_memmap[] > > > > = { > > > > [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, > > > > [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, > > > > [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, > > > > > > It seems customary in QEMU to flag gaps in memory space (although > > > admittedly, we'd already failed to do so here). This patch leaves > > > a > > > gap of 0x00010000. Is there a particular reason? > > > > > > > + [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 }, > > > > > > And then again a gap (the one we already had). > > > > > > No specific reason,but from ITS point of view tried to stay > > > within > > > the GIC's 0x40060000 to 0x50000000 zone.The gap of 0x00010000 > > > would > > > also account for future GIC additions like virtual LPI support. > > Right. I was more thinking 64kB isn't much space to extend into. > Would it be worth pushing the ITS either all the way up to just below > 0x50000000, 0x48000000, or 0x45000000? > > The current memory allocation size (of 67MB) for redistributor(SBSA_GIC_REDIST) is already very large relative to its overall required register address space.Hence ITS started at 0x44090000 (considering that redistributor space is sufficiently spaced) until 0x440B0000.Future virtual LPI addition can still stay within the 0x45000000 mark,leaving the whole area between 0x45000000 to 0x50000000 free for other devices. are comments still recommended here? > > Either way, the gap(s) would be good to point out with comments, and > potential future use. I only noticed this one on like the third pass > of reading. > > / > Leif > > > > > [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, > > > > [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, > > > > [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, > > > > @@ -377,7 +379,20 @@ static void > > > > create_secure_ram(SBSAMachineState > > > > *sms, > > > > memory_region_add_subregion(secure_sysmem, base, secram); > > > > } > > > > > > > > -static void create_gic(SBSAMachineState *sms) > > > > +static void create_its(SBSAMachineState *sms) > > > > +{ > > > > + DeviceState *dev; > > > > + > > > > + dev = qdev_new(TYPE_ARM_GICV3_ITS); > > > > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > > > > + > > > > + object_property_set_link(OBJECT(dev), "parent-gicv3", > > > > OBJECT(sms->gic), > > > > + &error_abort); > > > > + sysbus_realize_and_unref(s, &error_fatal); > > > > + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); > > > > +} > > > > + > > > > +static void create_gic(SBSAMachineState *sms, MemoryRegion > > > > *mem) > > > > { > > > > unsigned int smp_cpus = MACHINE(sms)->smp.cpus; > > > > SysBusDevice *gicbusdev; > > > > @@ -404,6 +419,10 @@ static void create_gic(SBSAMachineState > > > > *sms) > > > > qdev_prop_set_uint32(sms->gic, "len-redist-region-count", > > > > 1); > > > > qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", > > > > redist0_count); > > > > > > > > + object_property_set_link(OBJECT(sms->gic), "sysmem", > > > > OBJECT(mem), > > > > + &error_fatal); > > > > + qdev_prop_set_bit(sms->gic, "has-lpi", true); > > > > + > > > > gicbusdev = SYS_BUS_DEVICE(sms->gic); > > > > sysbus_realize_and_unref(gicbusdev, &error_fatal); > > > > sysbus_mmio_map(gicbusdev, 0, > > > > sbsa_ref_memmap[SBSA_GIC_DIST].base); > > > > @@ -450,6 +469,7 @@ static void create_gic(SBSAMachineState > > > > *sms) > > > > sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, > > > > qdev_get_gpio_in(cpudev, > > > > ARM_CPU_VFIQ)); > > > > } > > > > + create_its(sms); > > > > } > > > > > > > > static void create_uart(const SBSAMachineState *sms, int uart, > > > > @@ -762,7 +782,7 @@ static void sbsa_ref_init(MachineState > > > > *machine) > > > > > > > > create_secure_ram(sms, secure_sysmem); > > > > > > > > - create_gic(sms); > > > > + create_gic(sms, sysmem); > > > > > > > > create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); > > > > create_uart(sms, SBSA_SECURE_UART, secure_sysmem, > > > > serial_hd(1)); > > > > -- > > > > 2.27.0 > > > >
Hi Shashi, Apologies for dragging my heels over this. And the couple of GICv4-ish patches that have been I finally sat down today and did the cross-referencing between GIC architecture reference manual and GIC-6/700 TRMs required to verbalise my concerns usefully. On Thu, Jul 08, 2021 at 18:54:00 +0100, Leif Lindholm wrote: > On Fri, Jun 04, 2021 at 11:36:02 -0400, shashi.mallela@linaro.org wrote: > > On Fri, 2021-06-04 at 11:42 +0100, Leif Lindholm wrote: > > > On Thu, Jun 03, 2021 at 11:31:21 -0400, shashi.mallela@linaro.org > > > wrote: > > > > On Thu, 2021-06-03 at 12:42 +0100, Leif Lindholm wrote: > > > > > On Wed, Jun 02, 2021 at 14:00:41 -0400, Shashi Mallela wrote: > > > > > > Included creation of ITS as part of SBSA platform GIC > > > > > > initialization. > > > > > > > > > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > > > > > --- > > > > > > hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++--- > > > > > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > > > > > > index 43c19b4923..3d9c073636 100644 > > > > > > --- a/hw/arm/sbsa-ref.c > > > > > > +++ b/hw/arm/sbsa-ref.c > > > > > > @@ -107,6 +108,7 @@ static const MemMapEntry sbsa_ref_memmap[] > > > > > > = { > > > > > > [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, > > > > > > [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, > > > > > > [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, > > > > > > > > > > It seems customary in QEMU to flag gaps in memory space (although > > > > > admittedly, we'd already failed to do so here). This patch leaves > > > > > a > > > > > gap of 0x00010000. Is there a particular reason? > > > > > > > > > > > + [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 }, > > > > > > > > > > And then again a gap (the one we already had). > > > > > > > > > > No specific reason,but from ITS point of view tried to stay > > > > > within > > > > > the GIC's 0x40060000 to 0x50000000 zone.The gap of 0x00010000 > > > > > would > > > > > also account for future GIC additions like virtual LPI support. > > > > > > Right. I was more thinking 64kB isn't much space to extend into. > > > Would it be worth pushing the ITS either all the way up to just below > > > 0x50000000, 0x48000000, or 0x45000000? > > > > The current memory allocation size (of 67MB) for > > redistributor(SBSA_GIC_REDIST) is already very large relative to its > > overall required register address space. > > > Hence ITS started at 0x44090000 > > (considering that redistributor space is sufficiently spaced) until > > 0x440B0000.Future virtual LPI addition can still stay within the > > 0x45000000 mark, So, it turns out the practical problem here isn't the size and placement of "buffer" regions around the ITS region - but the actual placement of the ITS region itself, and the buffers around it. While this is an idealised model of a GIC implementation, there are benefits to it looking (quite) a bit like an actual ARM implementation. And GIC-6/700 place the ITS between the Distributor and Redistributors. Both GICs support up to 16 ITS *per chip*: https://developer.arm.com/documentation/100336/0106/operation/interrupt-translation-service--its and for GIC-600 this takes up (ITScount x 2) + 1 64k pages. For GIC-700, with GICv4.1 support, it is (ITScount x 4) + 1. Now, I don't know if we fully want to support layout compatibility with a "real GIC". But ideally, I would want to. At least for sbsa-ref. I think my summary-summary would be: - I think we will need to introduce a compatiblity-breaking change to sbsa-ref. - I think we will need to have support for more than one ITS if we're going to be able to use QEMU to prototype real systems. - I think we should then start versioning sbsa-ref (like many other platforms already are). And there are other reasons why I would want to do this. - But I think it would be unfair to hold this set back for it. Which can be seen as a long-winded way of saying: Reviewed-by: Leif Lindholm <leif@nuviainc.com> / Leif > > Leaving the whole area between 0x45000000 to 0x50000000 > > free for other devices. > > are comments still recommended here? > > I would prefer comments on the sizes of gaps. > > > > > > > Either way, the gap(s) would be good to point out with comments, and > > > potential future use. I only noticed this one on like the third pass > > > of reading. > > > > > > / > > > Leif > > > > > > > > > [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, > > > > > > [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, > > > > > > [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, > > > > > > @@ -377,7 +379,20 @@ static void > > > > > > create_secure_ram(SBSAMachineState > > > > > > *sms, > > > > > > memory_region_add_subregion(secure_sysmem, base, secram); > > > > > > } > > > > > > > > > > > > -static void create_gic(SBSAMachineState *sms) > > > > > > +static void create_its(SBSAMachineState *sms) > > > > > > +{ > > > > > > + DeviceState *dev; > > > > > > + > > > > > > + dev = qdev_new(TYPE_ARM_GICV3_ITS); > > > > > > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > > > > > > + > > > > > > + object_property_set_link(OBJECT(dev), "parent-gicv3", > > > > > > OBJECT(sms->gic), > > > > > > + &error_abort); > > > > > > + sysbus_realize_and_unref(s, &error_fatal); > > > > > > + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); > > > > > > +} > > > > > > + > > > > > > +static void create_gic(SBSAMachineState *sms, MemoryRegion > > > > > > *mem) > > > > > > { > > > > > > unsigned int smp_cpus = MACHINE(sms)->smp.cpus; > > > > > > SysBusDevice *gicbusdev; > > > > > > @@ -404,6 +419,10 @@ static void create_gic(SBSAMachineState > > > > > > *sms) > > > > > > qdev_prop_set_uint32(sms->gic, "len-redist-region-count", > > > > > > 1); > > > > > > qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", > > > > > > redist0_count); > > > > > > > > > > > > + object_property_set_link(OBJECT(sms->gic), "sysmem", > > > > > > OBJECT(mem), > > > > > > + &error_fatal); > > > > > > + qdev_prop_set_bit(sms->gic, "has-lpi", true); > > > > > > + > > > > > > gicbusdev = SYS_BUS_DEVICE(sms->gic); > > > > > > sysbus_realize_and_unref(gicbusdev, &error_fatal); > > > > > > sysbus_mmio_map(gicbusdev, 0, > > > > > > sbsa_ref_memmap[SBSA_GIC_DIST].base); > > > > > > @@ -450,6 +469,7 @@ static void create_gic(SBSAMachineState > > > > > > *sms) > > > > > > sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, > > > > > > qdev_get_gpio_in(cpudev, > > > > > > ARM_CPU_VFIQ)); > > > > > > } > > > > > > + create_its(sms); > > > > > > } > > > > > > > > > > > > static void create_uart(const SBSAMachineState *sms, int uart, > > > > > > @@ -762,7 +782,7 @@ static void sbsa_ref_init(MachineState > > > > > > *machine) > > > > > > > > > > > > create_secure_ram(sms, secure_sysmem); > > > > > > > > > > > > - create_gic(sms); > > > > > > + create_gic(sms, sysmem); > > > > > > > > > > > > create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); > > > > > > create_uart(sms, SBSA_SECURE_UART, secure_sysmem, > > > > > > serial_hd(1)); > > > > > > -- > > > > > > 2.27.0 > > > > > > > >
On Thu, 8 Jul 2021 at 20:40, Leif Lindholm <leif@nuviainc.com> wrote: > I think my summary-summary would be: > - I think we will need to introduce a compatiblity-breaking change to > sbsa-ref. > - I think we will need to have support for more than one ITS if we're > going to be able to use QEMU to prototype real systems. > - I think we should then start versioning sbsa-ref (like many other > platforms already are). And there are other reasons why I would want > to do this. > - But I think it would be unfair to hold this set back for it. FWIW, I do not currently expect this series to make 6.1, so we have some time to get things right. thanks -- PMM
On Thu, Jul 08, 2021 at 21:05:02 +0100, Peter Maydell wrote: > On Thu, 8 Jul 2021 at 20:40, Leif Lindholm <leif@nuviainc.com> wrote: > > I think my summary-summary would be: > > - I think we will need to introduce a compatiblity-breaking change to > > sbsa-ref. > > - I think we will need to have support for more than one ITS if we're > > going to be able to use QEMU to prototype real systems. > > - I think we should then start versioning sbsa-ref (like many other > > platforms already are). And there are other reasons why I would want > > to do this. > > - But I think it would be unfair to hold this set back for it. > > FWIW, I do not currently expect this series to make 6.1, so we > have some time to get things right. Ah, ok. Then I would ideally like to see this patch add the ITS block between Distributor and Redistributors regions. I think it makes the most sense for this version to match the GIC-600 layout. I am also going to rework those two remaining gicv4-ish patches based on my acquired understanding, to be sent out post v6.1. And, I would like to switch the default CPU of sbsa-ref to "max" as part of that platform versioning, now that is supported in TF-A (from v2.5). / Leif
On Thu, 2021-07-08 at 23:05 +0100, Leif Lindholm wrote: > On Thu, Jul 08, 2021 at 21:05:02 +0100, Peter Maydell wrote: > > On Thu, 8 Jul 2021 at 20:40, Leif Lindholm <leif@nuviainc.com> > > wrote: > > > I think my summary-summary would be: > > > - I think we will need to introduce a compatiblity-breaking > > > change to > > > sbsa-ref. > > > - I think we will need to have support for more than one ITS if > > > we're > > > going to be able to use QEMU to prototype real systems. > > > - I think we should then start versioning sbsa-ref (like many > > > other > > > platforms already are). And there are other reasons why I would > > > want > > > to do this. > > > - But I think it would be unfair to hold this set back for it. > > > > FWIW, I do not currently expect this series to make 6.1, so we > > have some time to get things right. > > Ah, ok. > > Then I would ideally like to see this patch add the ITS block between > Distributor and Redistributors regions. I think it makes the most > sense > for this version to match the GIC-600 layout. Have added ITS block between Distributor and Redistributors regions,with comments. Also,added sbsa versioning support (as discussed in last call) > > I am also going to rework those two remaining gicv4-ish patches based > on my acquired understanding, to be sent out post v6.1. > > And, I would like to switch the default CPU of sbsa-ref to "max" as > part of that platform versioning, now that is supported in TF-A > (from v2.5). > > / > Leif
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index 43c19b4923..3d9c073636 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -34,7 +34,7 @@ #include "hw/boards.h" #include "hw/ide/internal.h" #include "hw/ide/ahci_internal.h" -#include "hw/intc/arm_gicv3_common.h" +#include "hw/intc/arm_gicv3_its_common.h" #include "hw/loader.h" #include "hw/pci-host/gpex.h" #include "hw/qdev-properties.h" @@ -64,6 +64,7 @@ enum { SBSA_CPUPERIPHS, SBSA_GIC_DIST, SBSA_GIC_REDIST, + SBSA_GIC_ITS, SBSA_SECURE_EC, SBSA_GWDT, SBSA_GWDT_REFRESH, @@ -107,6 +108,7 @@ static const MemMapEntry sbsa_ref_memmap[] = { [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, + [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 }, [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, @@ -377,7 +379,20 @@ static void create_secure_ram(SBSAMachineState *sms, memory_region_add_subregion(secure_sysmem, base, secram); } -static void create_gic(SBSAMachineState *sms) +static void create_its(SBSAMachineState *sms) +{ + DeviceState *dev; + + dev = qdev_new(TYPE_ARM_GICV3_ITS); + SysBusDevice *s = SYS_BUS_DEVICE(dev); + + object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(sms->gic), + &error_abort); + sysbus_realize_and_unref(s, &error_fatal); + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); +} + +static void create_gic(SBSAMachineState *sms, MemoryRegion *mem) { unsigned int smp_cpus = MACHINE(sms)->smp.cpus; SysBusDevice *gicbusdev; @@ -404,6 +419,10 @@ static void create_gic(SBSAMachineState *sms) qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1); qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", redist0_count); + object_property_set_link(OBJECT(sms->gic), "sysmem", OBJECT(mem), + &error_fatal); + qdev_prop_set_bit(sms->gic, "has-lpi", true); + gicbusdev = SYS_BUS_DEVICE(sms->gic); sysbus_realize_and_unref(gicbusdev, &error_fatal); sysbus_mmio_map(gicbusdev, 0, sbsa_ref_memmap[SBSA_GIC_DIST].base); @@ -450,6 +469,7 @@ static void create_gic(SBSAMachineState *sms) sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ)); } + create_its(sms); } static void create_uart(const SBSAMachineState *sms, int uart, @@ -762,7 +782,7 @@ static void sbsa_ref_init(MachineState *machine) create_secure_ram(sms, secure_sysmem); - create_gic(sms); + create_gic(sms, sysmem); create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); create_uart(sms, SBSA_SECURE_UART, secure_sysmem, serial_hd(1));
Included creation of ITS as part of SBSA platform GIC initialization. Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> --- hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) -- 2.27.0