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

[Leif Lindholm]

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