From: Peter Maydell <peter.maydell@linaro.org>
To: Shashi Mallela <shashi.mallela@linaro.org>
Cc: Leif Lindholm <leif@nuviainc.com>,
QEMU Developers <qemu-devel@nongnu.org>,
qemu-arm <qemu-arm@nongnu.org>,
Radoslaw Biernacki <rad@semihalf.com>
Subject: Re: [PATCH v4 2/8] hw/intc: GICv3 ITS register definitions added
Date: Tue, 8 Jun 2021 11:31:56 +0100 [thread overview]
Message-ID: <CAFEAcA-+x4Tt9XZVPqk8ojT6C2jYXsoCkAkR00vbL-X3ykXdNg@mail.gmail.com> (raw)
In-Reply-To: <20210602180042.111347-3-shashi.mallela@linaro.org>
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
next prev parent reply other threads:[~2021-06-08 10:33 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-02 18:00 [PATCH v4 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
2021-06-02 18:00 ` [PATCH v4 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
2021-06-08 10:02 ` Peter Maydell
2021-06-11 16:21 ` Eric Auger
2021-06-11 17:23 ` Shashi Mallela
2021-07-06 7:38 ` Eric Auger
2021-07-06 13:24 ` shashi.mallela
2021-07-06 14:04 ` Eric Auger
2021-07-06 14:18 ` shashi.mallela
2021-06-12 6:52 ` Eric Auger
2021-07-06 7:29 ` Eric Auger
2021-06-02 18:00 ` [PATCH v4 2/8] hw/intc: GICv3 ITS register definitions added Shashi Mallela
2021-06-08 10:31 ` Peter Maydell [this message]
2021-06-12 6:08 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-21 9:51 ` Eric Auger
2021-06-28 21:51 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework Shashi Mallela
2021-06-08 10:38 ` Peter Maydell
2021-06-13 14:13 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-21 10:03 ` Eric Auger
2021-06-28 21:58 ` shashi.mallela
2021-06-13 14:39 ` Eric Auger
2021-06-28 15:55 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 4/8] hw/intc: GICv3 ITS Command processing Shashi Mallela
2021-06-08 10:45 ` Peter Maydell
2021-06-13 15:55 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-21 10:13 ` Eric Auger
2021-06-28 22:04 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 5/8] hw/intc: GICv3 ITS Feature enablement Shashi Mallela
2021-06-08 10:57 ` Peter Maydell
2021-06-02 18:00 ` [PATCH v4 6/8] hw/intc: GICv3 redistributor ITS processing Shashi Mallela
2021-06-08 13:57 ` Peter Maydell
2021-06-10 23:39 ` Shashi Mallela
2021-06-11 8:30 ` Peter Maydell
2021-06-15 2:23 ` Shashi Mallela
2021-06-13 16:26 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC Shashi Mallela
2021-06-03 11:42 ` Leif Lindholm
2021-06-03 15:31 ` shashi.mallela
2021-06-04 10:42 ` Leif Lindholm
2021-06-04 15:36 ` shashi.mallela
2021-07-08 19:40 ` Leif Lindholm
2021-07-08 20:05 ` Peter Maydell
2021-07-08 22:05 ` Leif Lindholm
2021-08-05 20:10 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 8/8] hw/arm/virt: add ITS support in virt GIC Shashi Mallela
2021-06-08 11:00 ` Peter Maydell
2021-06-08 10:00 ` [PATCH v4 0/8] GICv3 LPI and ITS feature implementation Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFEAcA-+x4Tt9XZVPqk8ojT6C2jYXsoCkAkR00vbL-X3ykXdNg@mail.gmail.com \
--to=peter.maydell@linaro.org \
--cc=leif@nuviainc.com \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=rad@semihalf.com \
--cc=shashi.mallela@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).