On Wed, 2 Jun 2021 at 19:00, 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 | 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
