Re: [PATCH v4 2/8] hw/intc: GICv3 ITS register definitions added

[Peter Maydell]

On Wed, 2 Jun 2021 at 19:00, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Defined descriptors for ITS device table,collection table and ITS
> command queue entities.Implemented register read/write functions,
> extract ITS table parameters and command queue parameters,extended
> gicv3 common to capture qemu address space(which host the ITS table
> platform memories required for subsequent ITS processing) and
> initialize the same in ITS device.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>

> @@ -41,7 +192,73 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr 
> offset,
>                                uint64_t value, MemTxAttrs attrs)
>  {
>      MemTxResult result = MEMTX_OK;
> +    int index;
>
> +    switch (offset) {
> +    case GITS_CTLR:
> +        s->ctlr |= (value & ~(s->ctlr));
> +
> +        if (s->ctlr & ITS_CTLR_ENABLED) {
> +            extract_table_params(s);
> +            extract_cmdq_params(s);
> +            s->creadr = 0;
> +        }
> +        break;
> +    case GITS_CBASER:
> +        /*
> +         * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
> +         *                 already enabled
> +         */
> +        if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> +            s->cbaser = deposit64(s->cbaser, 0, 32, value);
> +            s->creadr = 0;
> +        }
> +        break;
> +    case GITS_CBASER + 4:
> +        /*
> +         * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
> +         *                 already enabled
> +         */
> +        if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> +            s->cbaser = deposit64(s->cbaser, 32, 32, value);
> +        }
> +        break;
> +    case GITS_CWRITER:
> +        s->cwriter = deposit64(s->cwriter, 0, 32,
> +                               (value & ~R_GITS_CWRITER_RETRY_MASK));
> +        break;
> +    case GITS_CWRITER + 4:
> +        s->cwriter = deposit64(s->cwriter, 32, 32,
> +                               (value & ~R_GITS_CWRITER_RETRY_MASK));

The RETRY bit is at the bottom of the 64-bit register, so you
don't want to mask with it when we're writing the top 32 bits
(otherwise you incorrectly clear bit 33 of the full 64-bit register).

> +        break;
> +    case GITS_BASER ... GITS_BASER + 0x3f:
> +        /*
> +         * IMPDEF choice:- GITS_BASERn register becomes RO if ITS is
> +         *                 already enabled
> +         */
> +        if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> +            index = (offset - GITS_BASER) / 8;
> +
> +            if (offset & 7) {
> +                s->baser[index] = deposit64(s->baser[index], 32, 32,
> +                                            (value & ~GITS_BASER_VAL_MASK));
> +            } else {
> +                s->baser[index] = deposit64(s->baser[index], 0, 32,
> +                                            (value & ~GITS_BASER_VAL_MASK));
> +            }

This has two problems:
(1) same as above, you're masking a 32-bit half-value with a MASK
constant that's for the full 64-bit value
(2) here (unlike with CWRITER) we don't want to clear the non-writeable
bits but leave them alone.

Something like this should work:

               if (offset & 7) {
                   value <<= 32;
                   value &= ~GITS_BASER_VAL_MASK;
                   s->baser[index] &= GITS_BASER_VAL_MASK |
MAKE_64BIT_MASK(0, 32);
                   s->baser[index] |= value;
                } else {
                   value &= ~GITS_BASER_VAL_MASK;
                   s->baser[index] &= GITS_BASER_VAL_MASK |
MAKE_64BIT_MASK(32, 32);
                   s->baser[index] |= value;
                }

> +        }
> +        break;
> +    case GITS_IIDR:
> +    case GITS_IDREGS ... GITS_IDREGS + 0x2f:
> +        /* RO registers, ignore the write */
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid guest write to RO register at offset "
> +                      TARGET_FMT_plx "\n", __func__, offset);
> +        break;
> +    default:
> +        result = MEMTX_ERROR;
> +        break;
> +    }
>      return result;
>  }

> @@ -57,7 +322,42 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr 
> offset,
>                                 uint64_t value, MemTxAttrs attrs)
>  {
>      MemTxResult result = MEMTX_OK;
> +    int index;
>
> +    switch (offset) {
> +    case GITS_BASER ... GITS_BASER + 0x3f:
> +        /*
> +         * IMPDEF choice:- GITS_BASERn register becomes RO if ITS is
> +         *                 already enabled
> +         */
> +        if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> +            index = (offset - GITS_BASER) / 8;
> +            s->baser[index] |= (value & ~GITS_BASER_VAL_MASK);

This will allow the guest to write a 1 to a writeable bit,
but will not allow it to write a 0 again...
     s->baser[index] &= GITS_BASER_VAL_MASK;
     s->baser[index] |= (value & ~GITS_BASER_VAL_MASK);

Why VAL_MASK, by the way? The mask is defining the set of read-only bits,
so RO_MASK seems like a clearer name.

> +        }
> +        break;
> +    case GITS_CBASER:
> +        /*
> +         * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
> +         *                 already enabled
> +         */
> +        if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> +            s->cbaser = value;
> +        }
> +        break;
> +    case GITS_CWRITER:
> +        s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK;
> +        break;
> +    case GITS_CREADR:
> +    case GITS_TYPER:
> +        /* RO registers, ignore the write */
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid guest write to RO register at offset "
> +                      TARGET_FMT_plx "\n", __func__, offset);
> +        break;
> +    default:
> +        result = MEMTX_ERROR;
> +        break;
> +    }
>      return result;
>  }

Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM