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

[shashi . mallela]

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