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.mall...@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.mall...@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.mall...@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 <l...@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 > > > > > > > >