Re: [PATCH v2 5/8] hw/intc: GICv3 ITS Feature enablement

[Peter Maydell]

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.

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

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