On Mon, 2021-04-19 at 11:51 +0100, Peter Maydell wrote: > On Thu, 1 Apr 2021 at 03:41, Shashi Mallela < > shashi.mall...@linaro.org> wrote: > > Added properties to enable ITS feature and define qemu system > > address space memory in gicv3 common,setup distributor and > > redistributor registers to indicate LPI support. > > > > Signed-off-by: Shashi Mallela <shashi.mall...@linaro.org> > > --- > > hw/intc/arm_gicv3_common.c | 16 ++++++++++++++++ > > hw/intc/arm_gicv3_dist.c | 22 ++++++++++++++++++++-- > > hw/intc/arm_gicv3_redist.c | 28 +++++++++++++++++++++++++- > > -- > > hw/intc/gicv3_internal.h | 17 +++++++++++++++++ > > include/hw/intc/arm_gicv3_common.h | 8 ++++++++ > > 5 files changed, 86 insertions(+), 5 deletions(-) > > > > diff --git a/hw/intc/arm_gicv3_common.c > > b/hw/intc/arm_gicv3_common.c > > index 58ef65f589..3bfc52f7fa 100644 > > --- a/hw/intc/arm_gicv3_common.c > > +++ b/hw/intc/arm_gicv3_common.c > > @@ -156,6 +156,7 @@ static const VMStateDescription > > vmstate_gicv3_cpu = { > > VMSTATE_UINT32(gicr_waker, GICv3CPUState), > > VMSTATE_UINT64(gicr_propbaser, GICv3CPUState), > > VMSTATE_UINT64(gicr_pendbaser, GICv3CPUState), > > + VMSTATE_BOOL(lpi_outofrange, GICv3CPUState), > > VMSTATE_UINT32(gicr_igroupr0, GICv3CPUState), > > VMSTATE_UINT32(gicr_ienabler0, GICv3CPUState), > > VMSTATE_UINT32(gicr_ipendr0, GICv3CPUState), > > @@ -227,6 +228,7 @@ static const VMStateDescription vmstate_gicv3 = > > { > > .priority = MIG_PRI_GICV3, > > .fields = (VMStateField[]) { > > VMSTATE_UINT32(gicd_ctlr, GICv3State), > > + VMSTATE_UINT32(gicd_typer, GICv3State), > > VMSTATE_UINT32_ARRAY(gicd_statusr, GICv3State, 2), > > VMSTATE_UINT32_ARRAY(group, GICv3State, GICV3_BMP_SIZE), > > VMSTATE_UINT32_ARRAY(grpmod, GICv3State, GICV3_BMP_SIZE), > > You can't add fields to an existing VMStateDescription like this > without extra effort to handle migration compatibility. > What are we trying to achieve with the extra fields? > GICD_TYPER is read-only, I think. > I don't think we need an lpi_outofrange flag: we should just > naturally > handle this as part of working with the GITR_PROPBASER.IDbits field. > > Removed all fields to VMStateDescription,handled as part of working > with GITR_PROPBASER.IDbits field > > > @@ -381,6 +383,16 @@ static void > > arm_gicv3_common_realize(DeviceState *dev, Error **errp) > > (1 << 24) | > > (i << 8) | > > (last << 4); > > + > > + if (s->lpi_enable) { > > + s->cpu[i].gicr_typer |= GICR_TYPER_PLPIS; > > + > > + if (!s->sysmem) { > > + error_setg(errp, > > + "Redist-ITS: Guest 'sysmem' reference link not > > set"); > > + return; > > + } > > + } > > } > > } > > > > @@ -406,6 +418,7 @@ static void arm_gicv3_common_reset(DeviceState > > *dev) > > cs->gicr_waker = GICR_WAKER_ProcessorSleep | > > GICR_WAKER_ChildrenAsleep; > > cs->gicr_propbaser = 0; > > cs->gicr_pendbaser = 0; > > + cs->lpi_outofrange = false; > > /* If we're resetting a TZ-aware GIC as if secure firmware > > * had set it up ready to start a kernel in non-secure, we > > * need to set interrupts to group 1 so the kernel can use > > them. > > @@ -494,9 +507,12 @@ static Property arm_gicv3_common_properties[] > > = { > > DEFINE_PROP_UINT32("num-cpu", GICv3State, num_cpu, 1), > > DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32), > > DEFINE_PROP_UINT32("revision", GICv3State, revision, 3), > > + DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0), > > DEFINE_PROP_BOOL("has-security-extensions", GICv3State, > > security_extn, 0), > > DEFINE_PROP_ARRAY("redist-region-count", GICv3State, > > nb_redist_regions, > > redist_region_count, qdev_prop_uint32, > > uint32_t), > > + DEFINE_PROP_LINK("sysmem", GICv3State, sysmem, > > TYPE_MEMORY_REGION, > > + MemoryRegion *), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c > > index b65f56f903..96a317a8ef 100644 > > --- a/hw/intc/arm_gicv3_dist.c > > +++ b/hw/intc/arm_gicv3_dist.c > > @@ -366,12 +366,15 @@ static MemTxResult gicd_readl(GICv3State *s, > > hwaddr offset, > > return MEMTX_OK; > > case GICD_TYPER: > > { > > + bool lpi_supported = false; > > /* For this implementation: > > * No1N == 1 (1-of-N SPI interrupts not supported) > > * A3V == 1 (non-zero values of Affinity level 3 > > supported) > > * IDbits == 0xf (we support 16-bit interrupt identifiers) > > * DVIS == 0 (Direct virtual LPI injection not supported) > > - * LPIS == 0 (LPIs not supported) > > + * LPIS == 1 (LPIs are supported if affinity routing is > > enabled) > > + * num_LPIs == 0b00000 (bits [15:11],Number of LPIs as > > indicated > > + * by GICD_TYPER.IDbits) > > We support LPIs, but we have none ? That doesn't sound right, and > it's > not what the code below reports. > The interpretation as per spec here is that number of LPIs indication > comes from GICD_TYPER.IDbits with bits being set to 0b00000 > > > * MBIS == 0 (message-based SPIs not supported) > > * SecurityExtn == 1 if security extns supported > > * CPUNumber == 0 since for us ARE is always 1 > > @@ -385,8 +388,23 @@ static MemTxResult gicd_readl(GICv3State *s, > > hwaddr offset, > > */ > > bool sec_extn = !(s->gicd_ctlr & GICD_CTLR_DS); > > > > + /* > > + * With securityextn on,LPIs are supported when affinity > > routing > > (missing space after comma) > > > + * is enabled for non-secure state and if off LPIs are > > supported > > + * when affinity routing is enabled. > > + */ > > + if (s->lpi_enable) { > > + if (sec_extn) { > > + lpi_supported = (s->gicd_ctlr & GICD_CTLR_ARE_NS); > > + } else { > > + lpi_supported = (s->gicd_ctlr & GICD_CTLR_ARE); > > + } > > + } > > + > > *data = (1 << 25) | (1 << 24) | (sec_extn << 10) | > > - (0xf << 19) | itlinesnumber; > > + (lpi_supported << GICD_TYPER_LPIS_OFFSET) | > > (GICD_TYPER_IDBITS << > > + GICD_TYPER_IDBITS_OFFSET) | itlinesnumber; > > + s->gicd_typer = *data; > > return MEMTX_OK; > > } > > case GICD_IIDR: > > diff --git a/hw/intc/arm_gicv3_redist.c > > b/hw/intc/arm_gicv3_redist.c > > index 8645220d61..325b974e70 100644 > > --- a/hw/intc/arm_gicv3_redist.c > > +++ b/hw/intc/arm_gicv3_redist.c > > @@ -248,10 +248,16 @@ static MemTxResult gicr_writel(GICv3CPUState > > *cs, hwaddr offset, > > case GICR_CTLR: > > /* For our implementation, GICR_TYPER.DPGS is 0 and so all > > * the DPG bits are RAZ/WI. We don't do anything > > asynchronously, > > - * so UWP and RWP are RAZ/WI. And GICR_TYPER.LPIS is 0 (we > > don't > > - * implement LPIs) so Enable_LPIs is RES0. So there are no > > writable > > - * bits for us. > > + * so UWP and RWP are RAZ/WI. GICR_TYPER.LPIS is 1 (we > > + * implement LPIs) so Enable_LPIs is programmable. > > */ > > + if (cs->gicr_typer & GICR_TYPER_PLPIS) { > > + if (value & GICR_CTLR_ENABLE_LPIS) { > > + cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS; > > + } else { > > + cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; > > + } > > + } > > return MEMTX_OK; > > case GICR_STATUSR: > > /* RAZ/WI for our implementation */ > > @@ -275,6 +281,14 @@ static MemTxResult gicr_writel(GICv3CPUState > > *cs, hwaddr offset, > > cs->gicr_waker = value; > > return MEMTX_OK; > > case GICR_PROPBASER: > > + if (FIELD_EX64(value, GICR_PROPBASER, IDBITS) < > > + GICR_PROPBASER_IDBITS_THRESHOLD) { > > + cs->lpi_outofrange = true; > > I don't think you should need to special case this. It just means > "turns out > that the LPI table is effectively zero length". When code doing LPI > table > lookups calculates the LPI table size (which it needs to do anyway) > it can > just catch the "turns out to be negative" case then. > > > + } > > + if (FIELD_EX64(value, GICR_PROPBASER, IDBITS) > > > GICD_TYPER_IDBITS) { > > + value &= ~R_GICR_PROPBASER_IDBITS_MASK; > > + value |= GICD_TYPER_IDBITS; > > + } > > This isn't what the spec says happens. It says that if the value the > guest writes > in this field is larger than GICD_TYPER.IDbits, then the > GICD_TYPER.IDBits value > applies. That doesn't mean the value reads back as GICD_TYPER.IDBits. > > How you want to handle this depends on what's going on, but it > probably mostly > looks like "code that cares about GICR_PROPBASER.IDBits will do > MIN(field_value, GICD_TYPER_IDBITS) to find the effective value of > the field". > > > cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 0, 32, > > value); > > return MEMTX_OK; > > case GICR_PROPBASER + 4: > > @@ -397,6 +411,14 @@ static MemTxResult gicr_writell(GICv3CPUState > > *cs, hwaddr offset, > > { > > switch (offset) { > > case GICR_PROPBASER: > > + if (FIELD_EX64(value, GICR_PROPBASER, IDBITS) < > > + GICR_PROPBASER_IDBITS_THRESHOLD) { > > + cs->lpi_outofrange = true; > > + } > > + if (FIELD_EX64(value, GICR_PROPBASER, IDBITS) > > > GICD_TYPER_IDBITS) { > > + value &= ~R_GICR_PROPBASER_IDBITS_MASK; > > + value |= GICD_TYPER_IDBITS; > > + } > > Same comments as above. > > > cs->gicr_propbaser = value; > > return MEMTX_OK; > > case GICR_PENDBASER: > > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > > index e9f9aa6722..a2718704d4 100644 > > --- a/hw/intc/gicv3_internal.h > > +++ b/hw/intc/gicv3_internal.h > > @@ -68,6 +68,9 @@ > > #define GICD_CTLR_E1NWF (1U << 7) > > #define GICD_CTLR_RWP (1U << 31) > > > > +#define GICD_TYPER_LPIS_OFFSET 17 > > +#define GICD_TYPER_IDBITS_OFFSET 19 > > +#define GICD_TYPER_IDBITS_MASK 0x1f > > /* 16 bits EventId */ > > #define GICD_TYPER_IDBITS 0xf > > > > @@ -126,6 +129,20 @@ > > #define GICR_WAKER_ProcessorSleep (1U << 1) > > #define GICR_WAKER_ChildrenAsleep (1U << 2) > > > > +FIELD(GICR_PROPBASER, IDBITS, 0, 5) > > +FIELD(GICR_PROPBASER, INNERCACHE, 7, 3) > > +FIELD(GICR_PROPBASER, SHAREABILITY, 10, 2) > > +FIELD(GICR_PROPBASER, PHYADDR, 12, 40) > > +FIELD(GICR_PROPBASER, OUTERCACHE, 56, 3) > > + > > +#define GICR_PROPBASER_IDBITS_THRESHOLD 0xd > > + > > +FIELD(GICR_PENDBASER, INNERCACHE, 7, 3) > > +FIELD(GICR_PENDBASER, SHAREABILITY, 10, 2) > > +FIELD(GICR_PENDBASER, PHYADDR, 16, 36) > > +FIELD(GICR_PENDBASER, OUTERCACHE, 56, 3) > > +FIELD(GICR_PENDBASER, PTZ, 62, 1) > > + > > #define ICC_CTLR_EL1_CBPR (1U << 0) > > #define ICC_CTLR_EL1_EOIMODE (1U << 1) > > #define ICC_CTLR_EL1_PMHE (1U << 6) > > diff --git a/include/hw/intc/arm_gicv3_common.h > > b/include/hw/intc/arm_gicv3_common.h > > index 3a710592a9..db3989484d 100644 > > --- a/include/hw/intc/arm_gicv3_common.h > > +++ b/include/hw/intc/arm_gicv3_common.h > > @@ -175,6 +175,13 @@ struct GICv3CPUState { > > uint32_t gicr_nsacr; > > uint8_t gicr_ipriorityr[GIC_INTERNAL]; > > > > + /* > > + * flag to indicate LPIs are out of range > > + * since IDbits from GICR_PROPBASER is less > > + * than 0b1101 > > + */ > > + bool lpi_outofrange; > > + > > /* CPU interface */ > > uint64_t icc_sre_el1; > > uint64_t icc_ctlr_el1[2]; > > @@ -221,6 +228,7 @@ struct GICv3State { > > uint32_t num_cpu; > > uint32_t num_irq; > > uint32_t revision; > > + bool lpi_enable; > > bool security_extn; > > bool irq_reset_nonsecure; > > bool gicd_no_migration_shift_bug; > > thanks > -- PMM