Re: [PATCH v4 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC

[Leif Lindholm <leif@nuviainc.com>]

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
> > > > > > 
> >