[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/8] hw/intc: GICv3 ITS Feature enablement
|
From: |
shashi . mallela |
|
Subject: |
Re: [PATCH v2 5/8] hw/intc: GICv3 ITS Feature enablement |
|
Date: |
Thu, 29 Apr 2021 19:39:08 -0400 |
On Mon, 2021-04-19 at 11:51 +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 03:41, Shashi Mallela <
> shashi.mallela@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.mallela@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