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

[Peter Maydell]

On Wed, 2 Jun 2021 at 19:00, 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         | 12 ++++++++++++
>  hw/intc/arm_gicv3_dist.c           |  7 +++++--
>  hw/intc/arm_gicv3_its.c            |  9 ++++++++-
>  hw/intc/arm_gicv3_redist.c         | 14 +++++++++++---
>  hw/intc/gicv3_internal.h           | 17 +++++++++++++++++
>  include/hw/intc/arm_gicv3_common.h |  1 +
>  6 files changed, 54 insertions(+), 6 deletions(-)


> @@ -386,7 +388,8 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr 
> offset,
>          bool sec_extn = !(s->gicd_ctlr & GICD_CTLR_DS);
>
>          *data = (1 << 25) | (1 << 24) | (sec_extn << 10) |
> -            (0xf << 19) | itlinesnumber;
> +            (s->lpi_enable << GICD_TYPER_LPIS_OFFSET) |
> +            (GICD_TYPER_IDBITS << GICD_TYPER_IDBITS_OFFSET) | itlinesnumber;
>          return MEMTX_OK;
>      }
>      case GICD_IIDR:

This change is doing two things at once:
(1) setting the LPI enable bit
(2) changing from (0xf << 19) to something using symbolic constants.

If you want to do (2) as a cleanup I don't object, but please put
it in its own patch as it is unrelated to this one.

> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 82bb5b84ef..0a978cf55b 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -294,6 +294,7 @@ static MemTxResult process_mapti(GICv3ITSState *s, 
> uint64_t value,
>      uint64_t itel = 0;
>      uint32_t iteh = 0;
>      uint32_t int_spurious = INTID_SPURIOUS;
> +    uint64_t idbits;
>
>      devid = (value >> DEVID_SHIFT) & DEVID_MASK;
>      offset += NUM_BYTES_IN_DW;
> @@ -330,7 +331,13 @@ static MemTxResult process_mapti(GICv3ITSState *s, 
> uint64_t value,
>      max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));
>
>      if (!ignore_pInt) {
> -        max_Intid = (1UL << (FIELD_EX64(s->typer, GITS_TYPER, IDBITS) + 1));
> +        idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, 
> GICR_PROPBASER,
> +                                IDBITS), GICD_TYPER_IDBITS);
> +
> +        if (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) {
> +            return res;
> +        }
> +        max_Intid = (1ULL << (idbits + 1));
>      }
>

This change should be folded into the patch where you add
this process_mapti() code, so it is correct from the start.

>      if ((devid > s->dt.max_devids) || (icid > s->ct.max_collids) ||
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index 8645220d61..fb9a4ee3cc 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -244,14 +244,21 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr 
> offset,
>  static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
>                                 uint64_t value, MemTxAttrs attrs)
>  {
> +

Stray new blank line.

>      switch (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 */
> @@ -395,6 +402,7 @@ static MemTxResult gicr_readll(GICv3CPUState *cs, hwaddr 
> offset,
>  static MemTxResult gicr_writell(GICv3CPUState *cs, hwaddr offset,
>                                  uint64_t value, MemTxAttrs attrs)
>  {
> +
>      switch (offset) {
>      case GICR_PROPBASER:
>          cs->gicr_propbaser = value;

Another stray new blank line.

thanks
-- PMM