* [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes
@ 2022-02-01 19:31 Peter Maydell
2022-02-01 19:31 ` [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets Peter Maydell
` (13 more replies)
0 siblings, 14 replies; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:31 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
This is another set of patches to the ITS emulation; mostly
cleanups, but there are two bug fixes.
Cleanups:
(1) Switch away from reading command packets by multiple separate
calls to address_space_ldq_le(), and instead use
address_space_map()/unmap() to get all 4 doublewords in the command
packet at once in the top level queue-processing loop. This gets all
the handling of loading words out of the individual command-handling
functions and makes them a lot easier to read IMHO. Plus it reduces
the number of lines of code by about a hundred.
(2) Unify all of the get_foo()/update_foo() functions that we use
for reading and writing the various in-guest-memory tables so that
they use a single style of API rather than being confusingly
different about how they indicate failure and how they return
the interesting fields from the table entries.
(3) A handful of less exciting minor tweaks.
Bugfixes:
(1) We were mis-calculating the address to use for the
last 4 bytes in an interrupt table entry, so they overwrote
the middle 4 bytes... The fix for this one is slightly
awkward because we need to handle migration from guests which
have in-memory tables written using the buggy code.
(2) We shouldn't validity-check rdbase in MAPC with V=0
(3) MAPI/MAPTI with intid 1023 should be rejected
thanks
-- PMM
Peter Maydell (13):
hw/intc/arm_gicv3_its: Use address_space_map() to access command queue
packets
hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t
hw/intc/arm_gicv3_its: Pass DTEntry to update_dte()
hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t
hw/intc/arm_gicv3_its: Pass CTEntry to update_cte()
hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and
update_ite()
hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite()
hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a
struct
hw/intc/arm_gicv3_its: Make update_ite() use ITEntry
hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields
hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field
hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI
hw/intc/arm_gicv3_its: Split error checks
hw/intc/gicv3_internal.h | 23 +-
include/hw/intc/arm_gicv3_its_common.h | 2 -
hw/intc/arm_gicv3_its.c | 696 +++++++++++--------------
3 files changed, 328 insertions(+), 393 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
@ 2022-02-01 19:31 ` Peter Maydell
2022-02-03 2:15 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 02/13] hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t Peter Maydell
` (12 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:31 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
Currently the ITS accesses each 8-byte doubleword in a 4-doubleword
command packet with a separate address_space_ldq_le() call. This is
awkward because the individual command processing functions have
ended up with code to handle "load more doublewords out of the
packet", which is both unwieldy and also a potential source of bugs
because it's not obvious when looking at a line that pulls a field
out of the 'value' variable which of the 4 doublewords that variable
currently holds.
Switch to using address_space_map() to map the whole command packet
at once and fish the four doublewords out of it. Then each process_*
function can start with a few lines of code that extract the fields
it cares about.
This requires us to split out the guts of process_its_cmd() into a
new do_process_its_cmd(), because we were previously overloading the
value and offset arguments as a backdoor way to directly pass the
devid and eventid from a write to GITS_TRANSLATER. The new
do_process_its_cmd() takes those arguments directly, and
process_its_cmd() is just a wrapper that does the "read fields from
command packet" part.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/gicv3_internal.h | 4 +-
hw/intc/arm_gicv3_its.c | 208 +++++++++++----------------------------
2 files changed, 62 insertions(+), 150 deletions(-)
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index b1af26df9f4..60c8617e4e4 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -309,8 +309,8 @@ FIELD(GITS_TYPER, CIL, 36, 1)
#define LPI_CTE_ENABLED TABLE_ENTRY_VALID_MASK
#define LPI_PRIORITY_MASK 0xfc
-#define GITS_CMDQ_ENTRY_SIZE 32
-#define NUM_BYTES_IN_DW 8
+#define GITS_CMDQ_ENTRY_WORDS 4
+#define GITS_CMDQ_ENTRY_SIZE (GITS_CMDQ_ENTRY_WORDS * sizeof(uint64_t))
#define CMD_MASK 0xff
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 51d9be4ae6f..b74753fb8fe 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -224,11 +224,9 @@ static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res)
* 3. handling of ITS CLEAR command
* 4. handling of ITS DISCARD command
*/
-static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
- uint32_t offset, ItsCmdType cmd)
+static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
+ uint32_t eventid, ItsCmdType cmd)
{
- AddressSpace *as = &s->gicv3->dma_as;
- uint32_t devid, eventid;
MemTxResult res = MEMTX_OK;
bool dte_valid;
uint64_t dte = 0;
@@ -240,22 +238,6 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
bool cte_valid = false;
uint64_t rdbase;
- if (cmd == NONE) {
- devid = offset;
- } else {
- devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
-
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
- }
-
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- eventid = (value & EVENTID_MASK);
-
if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: devid %d>=%d",
@@ -342,11 +324,19 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
}
return CMD_CONTINUE;
}
-
-static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
- uint32_t offset, bool ignore_pInt)
+static ItsCmdResult process_its_cmd(GICv3ITSState *s, const uint64_t *cmdpkt,
+ ItsCmdType cmd)
+{
+ uint32_t devid, eventid;
+
+ devid = (cmdpkt[0] & DEVID_MASK) >> DEVID_SHIFT;
+ eventid = cmdpkt[1] & EVENTID_MASK;
+ return do_process_its_cmd(s, devid, eventid, cmd);
+}
+
+static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
+ bool ignore_pInt)
{
- AddressSpace *as = &s->gicv3->dma_as;
uint32_t devid, eventid;
uint32_t pIntid = 0;
uint64_t num_eventids;
@@ -357,32 +347,16 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
uint64_t dte = 0;
IteEntry ite = {};
- devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
-
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- eventid = (value & EVENTID_MASK);
+ devid = (cmdpkt[0] & DEVID_MASK) >> DEVID_SHIFT;
+ eventid = cmdpkt[1] & EVENTID_MASK;
if (ignore_pInt) {
pIntid = eventid;
} else {
- pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT);
+ pIntid = (cmdpkt[1] & pINTID_MASK) >> pINTID_SHIFT;
}
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
-
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- icid = value & ICID_MASK;
+ icid = cmdpkt[2] & ICID_MASK;
if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -459,31 +433,18 @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
return res == MEMTX_OK;
}
-static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
+static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
{
- AddressSpace *as = &s->gicv3->dma_as;
uint16_t icid;
uint64_t rdbase;
bool valid;
- MemTxResult res = MEMTX_OK;
- uint64_t value;
- offset += NUM_BYTES_IN_DW;
- offset += NUM_BYTES_IN_DW;
+ icid = cmdpkt[2] & ICID_MASK;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
-
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- icid = value & ICID_MASK;
-
- rdbase = (value & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT;
+ rdbase = (cmdpkt[2] & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT;
rdbase &= RDBASE_PROCNUM_MASK;
- valid = (value & CMD_FIELD_VALID_MASK);
+ valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
if ((icid >= s->ct.num_entries) || (rdbase >= s->gicv3->num_cpu)) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -532,39 +493,17 @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
return res == MEMTX_OK;
}
-static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
- uint32_t offset)
+static ItsCmdResult process_mapd(GICv3ITSState *s, const uint64_t *cmdpkt)
{
- AddressSpace *as = &s->gicv3->dma_as;
uint32_t devid;
uint8_t size;
uint64_t itt_addr;
bool valid;
- MemTxResult res = MEMTX_OK;
- devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
-
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
-
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- size = (value & SIZE_MASK);
-
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
-
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- itt_addr = (value & ITTADDR_MASK) >> ITTADDR_SHIFT;
-
- valid = (value & CMD_FIELD_VALID_MASK);
+ devid = (cmdpkt[0] & DEVID_MASK) >> DEVID_SHIFT;
+ size = cmdpkt[1] & SIZE_MASK;
+ itt_addr = (cmdpkt[2] & ITTADDR_MASK) >> ITTADDR_SHIFT;
+ valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
if ((devid >= s->dt.num_entries) ||
(size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
@@ -582,23 +521,13 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
return update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL;
}
-static ItsCmdResult process_movall(GICv3ITSState *s, uint64_t value,
- uint32_t offset)
+static ItsCmdResult process_movall(GICv3ITSState *s, const uint64_t *cmdpkt)
{
- AddressSpace *as = &s->gicv3->dma_as;
- MemTxResult res = MEMTX_OK;
uint64_t rd1, rd2;
- /* No fields in dwords 0 or 1 */
- offset += NUM_BYTES_IN_DW;
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
+ rd1 = FIELD_EX64(cmdpkt[2], MOVALL_2, RDBASE1);
+ rd2 = FIELD_EX64(cmdpkt[3], MOVALL_3, RDBASE2);
- rd1 = FIELD_EX64(value, MOVALL_2, RDBASE1);
if (rd1 >= s->gicv3->num_cpu) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: RDBASE1 %" PRId64
@@ -606,15 +535,6 @@ static ItsCmdResult process_movall(GICv3ITSState *s, uint64_t value,
__func__, rd1, s->gicv3->num_cpu);
return CMD_CONTINUE;
}
-
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
-
- rd2 = FIELD_EX64(value, MOVALL_3, RDBASE2);
if (rd2 >= s->gicv3->num_cpu) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: RDBASE2 %" PRId64
@@ -634,10 +554,8 @@ static ItsCmdResult process_movall(GICv3ITSState *s, uint64_t value,
return CMD_CONTINUE;
}
-static ItsCmdResult process_movi(GICv3ITSState *s, uint64_t value,
- uint32_t offset)
+static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
{
- AddressSpace *as = &s->gicv3->dma_as;
MemTxResult res = MEMTX_OK;
uint32_t devid, eventid, intid;
uint16_t old_icid, new_icid;
@@ -648,23 +566,9 @@ static ItsCmdResult process_movi(GICv3ITSState *s, uint64_t value,
uint64_t num_eventids;
IteEntry ite = {};
- devid = FIELD_EX64(value, MOVI_0, DEVICEID);
-
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
- eventid = FIELD_EX64(value, MOVI_1, EVENTID);
-
- offset += NUM_BYTES_IN_DW;
- value = address_space_ldq_le(as, s->cq.base_addr + offset,
- MEMTXATTRS_UNSPECIFIED, &res);
- if (res != MEMTX_OK) {
- return CMD_STALL;
- }
- new_icid = FIELD_EX64(value, MOVI_2, ICID);
+ devid = FIELD_EX64(cmdpkt[0], MOVI_0, DEVICEID);
+ eventid = FIELD_EX64(cmdpkt[1], MOVI_1, EVENTID);
+ new_icid = FIELD_EX64(cmdpkt[2], MOVI_2, ICID);
if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -786,9 +690,7 @@ static void process_cmdq(GICv3ITSState *s)
uint32_t wr_offset = 0;
uint32_t rd_offset = 0;
uint32_t cq_offset = 0;
- uint64_t data;
AddressSpace *as = &s->gicv3->dma_as;
- MemTxResult res = MEMTX_OK;
uint8_t cmd;
int i;
@@ -816,28 +718,40 @@ static void process_cmdq(GICv3ITSState *s)
while (wr_offset != rd_offset) {
ItsCmdResult result = CMD_CONTINUE;
+ void *hostmem;
+ hwaddr buflen;
+ uint64_t cmdpkt[GITS_CMDQ_ENTRY_WORDS];
cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE);
- data = address_space_ldq_le(as, s->cq.base_addr + cq_offset,
- MEMTXATTRS_UNSPECIFIED, &res);
- if (res != MEMTX_OK) {
+
+ buflen = GITS_CMDQ_ENTRY_SIZE;
+ hostmem = address_space_map(as, s->cq.base_addr + cq_offset,
+ &buflen, false, MEMTXATTRS_UNSPECIFIED);
+ if (!hostmem || buflen != GITS_CMDQ_ENTRY_SIZE) {
+ if (hostmem) {
+ address_space_unmap(as, hostmem, buflen, false, 0);
+ }
s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1);
qemu_log_mask(LOG_GUEST_ERROR,
"%s: could not read command at 0x%" PRIx64 "\n",
__func__, s->cq.base_addr + cq_offset);
break;
}
+ for (i = 0; i < ARRAY_SIZE(cmdpkt); i++) {
+ cmdpkt[i] = ldq_le_p(hostmem + i * sizeof(uint64_t));
+ }
+ address_space_unmap(as, hostmem, buflen, false, 0);
- cmd = (data & CMD_MASK);
+ cmd = cmdpkt[0] & CMD_MASK;
trace_gicv3_its_process_command(rd_offset, cmd);
switch (cmd) {
case GITS_CMD_INT:
- result = process_its_cmd(s, data, cq_offset, INTERRUPT);
+ result = process_its_cmd(s, cmdpkt, INTERRUPT);
break;
case GITS_CMD_CLEAR:
- result = process_its_cmd(s, data, cq_offset, CLEAR);
+ result = process_its_cmd(s, cmdpkt, CLEAR);
break;
case GITS_CMD_SYNC:
/*
@@ -848,19 +762,19 @@ static void process_cmdq(GICv3ITSState *s)
*/
break;
case GITS_CMD_MAPD:
- result = process_mapd(s, data, cq_offset);
+ result = process_mapd(s, cmdpkt);
break;
case GITS_CMD_MAPC:
- result = process_mapc(s, cq_offset);
+ result = process_mapc(s, cmdpkt);
break;
case GITS_CMD_MAPTI:
- result = process_mapti(s, data, cq_offset, false);
+ result = process_mapti(s, cmdpkt, false);
break;
case GITS_CMD_MAPI:
- result = process_mapti(s, data, cq_offset, true);
+ result = process_mapti(s, cmdpkt, true);
break;
case GITS_CMD_DISCARD:
- result = process_its_cmd(s, data, cq_offset, DISCARD);
+ result = process_its_cmd(s, cmdpkt, DISCARD);
break;
case GITS_CMD_INV:
case GITS_CMD_INVALL:
@@ -875,10 +789,10 @@ static void process_cmdq(GICv3ITSState *s)
}
break;
case GITS_CMD_MOVI:
- result = process_movi(s, data, cq_offset);
+ result = process_movi(s, cmdpkt);
break;
case GITS_CMD_MOVALL:
- result = process_movall(s, data, cq_offset);
+ result = process_movall(s, cmdpkt);
break;
default:
break;
@@ -1032,15 +946,13 @@ static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
{
GICv3ITSState *s = (GICv3ITSState *)opaque;
bool result = true;
- uint32_t devid = 0;
trace_gicv3_its_translation_write(offset, data, size, attrs.requester_id);
switch (offset) {
case GITS_TRANSLATER:
if (s->ctlr & R_GITS_CTLR_ENABLED_MASK) {
- devid = attrs.requester_id;
- result = process_its_cmd(s, data, devid, NONE);
+ result = do_process_its_cmd(s, attrs.requester_id, data, NONE);
}
break;
default:
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 02/13] hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
2022-02-01 19:31 ` [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets Peter Maydell
@ 2022-02-01 19:31 ` Peter Maydell
2022-02-03 2:23 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 03/13] hw/intc/arm_gicv3_its: Pass DTEntry to update_dte() Peter Maydell
` (11 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:31 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
In the ITS, a DTE is an entry in the device table, which contains
multiple fields. Currently the function get_dte() which reads one
entry from the device table returns it as a raw 64-bit integer,
which we then pass around in that form, only extracting fields
from it as we need them.
Create a real C struct with the same fields as the DTE, and
populate it in get_dte(), so that that function and update_dte()
are the only ones that need to care about the in-guest-memory
format of the DTE.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This isn't a massive gain for the DTE, though I do think it
looks a bit nicer. The real benefit is in aligning all the
get_{cte,dte,ite} functions to have the same shaped API:
currently get_ite() is different from all the rest.
---
hw/intc/arm_gicv3_its.c | 111 ++++++++++++++++++++--------------------
1 file changed, 56 insertions(+), 55 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index b74753fb8fe..6d70d7d59e2 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -46,6 +46,12 @@ typedef struct {
uint64_t itel;
} IteEntry;
+typedef struct DTEntry {
+ bool valid;
+ unsigned size;
+ uint64_t ittaddr;
+} DTEntry;
+
/*
* The ITS spec permits a range of CONSTRAINED UNPREDICTABLE options
* if a command parameter is not correct. These include both "stall
@@ -143,22 +149,18 @@ static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte,
return FIELD_EX64(*cte, CTE, VALID);
}
-static bool update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
+static bool update_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
IteEntry ite)
{
AddressSpace *as = &s->gicv3->dma_as;
- uint64_t itt_addr;
MemTxResult res = MEMTX_OK;
- itt_addr = FIELD_EX64(dte, DTE, ITTADDR);
- itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
-
- address_space_stq_le(as, itt_addr + (eventid * (sizeof(uint64_t) +
+ address_space_stq_le(as, dte->ittaddr + (eventid * (sizeof(uint64_t) +
sizeof(uint32_t))), ite.itel, MEMTXATTRS_UNSPECIFIED,
&res);
if (res == MEMTX_OK) {
- address_space_stl_le(as, itt_addr + (eventid * (sizeof(uint64_t) +
+ address_space_stl_le(as, dte->ittaddr + (eventid * (sizeof(uint64_t) +
sizeof(uint32_t))) + sizeof(uint32_t), ite.iteh,
MEMTXATTRS_UNSPECIFIED, &res);
}
@@ -169,24 +171,20 @@ static bool update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
}
}
-static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
+static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
uint16_t *icid, uint32_t *pIntid, MemTxResult *res)
{
AddressSpace *as = &s->gicv3->dma_as;
- uint64_t itt_addr;
bool status = false;
IteEntry ite = {};
- itt_addr = FIELD_EX64(dte, DTE, ITTADDR);
- itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
-
- ite.itel = address_space_ldq_le(as, itt_addr +
+ ite.itel = address_space_ldq_le(as, dte->ittaddr +
(eventid * (sizeof(uint64_t) +
sizeof(uint32_t))), MEMTXATTRS_UNSPECIFIED,
res);
if (*res == MEMTX_OK) {
- ite.iteh = address_space_ldl_le(as, itt_addr +
+ ite.iteh = address_space_ldl_le(as, dte->ittaddr +
(eventid * (sizeof(uint64_t) +
sizeof(uint32_t))) + sizeof(uint32_t),
MEMTXATTRS_UNSPECIFIED, res);
@@ -205,15 +203,33 @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
return status;
}
-static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res)
+/*
+ * Read the Device Table entry at index @devid. On success (including
+ * successfully determining that there is no valid DTE for this index),
+ * we return MEMTX_OK and populate the DTEntry struct accordingly.
+ * If there is an error reading memory then we return the error code.
+ */
+static MemTxResult get_dte(GICv3ITSState *s, uint32_t devid, DTEntry *dte)
{
+ MemTxResult res = MEMTX_OK;
AddressSpace *as = &s->gicv3->dma_as;
- uint64_t entry_addr = table_entry_addr(s, &s->dt, devid, res);
+ uint64_t entry_addr = table_entry_addr(s, &s->dt, devid, &res);
+ uint64_t dteval;
if (entry_addr == -1) {
- return 0; /* a DTE entry with the Valid bit clear */
+ /* No L2 table entry, i.e. no valid DTE, or a memory error */
+ dte->valid = false;
+ return res;
}
- return address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, res);
+ dteval = address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, &res);
+ if (res != MEMTX_OK) {
+ return res;
+ }
+ dte->valid = FIELD_EX64(dteval, DTE, VALID);
+ dte->size = FIELD_EX64(dteval, DTE, SIZE);
+ /* DTE word field stores bits [51:8] of the ITT address */
+ dte->ittaddr = FIELD_EX64(dteval, DTE, ITTADDR) << ITTADDR_SHIFT;
+ return MEMTX_OK;
}
/*
@@ -228,8 +244,6 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
uint32_t eventid, ItsCmdType cmd)
{
MemTxResult res = MEMTX_OK;
- bool dte_valid;
- uint64_t dte = 0;
uint64_t num_eventids;
uint16_t icid = 0;
uint32_t pIntid = 0;
@@ -237,6 +251,7 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
uint64_t cte = 0;
bool cte_valid = false;
uint64_t rdbase;
+ DTEntry dte;
if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -245,23 +260,17 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
return CMD_CONTINUE;
}
- dte = get_dte(s, devid, &res);
-
- if (res != MEMTX_OK) {
+ if (get_dte(s, devid, &dte) != MEMTX_OK) {
return CMD_STALL;
}
- dte_valid = FIELD_EX64(dte, DTE, VALID);
-
- if (!dte_valid) {
+ if (!dte.valid) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: "
- "invalid dte: %"PRIx64" for %d\n",
- __func__, dte, devid);
+ "invalid dte for %d\n", __func__, devid);
return CMD_CONTINUE;
}
- num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
-
+ num_eventids = 1ULL << (dte.size + 1);
if (eventid >= num_eventids) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: eventid %d >= %"
@@ -270,7 +279,7 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
return CMD_CONTINUE;
}
- ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
+ ite_valid = get_ite(s, eventid, &dte, &icid, &pIntid, &res);
if (res != MEMTX_OK) {
return CMD_STALL;
}
@@ -320,7 +329,7 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
if (cmd == DISCARD) {
IteEntry ite = {};
/* remove mapping from interrupt translation table */
- return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+ return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
}
return CMD_CONTINUE;
}
@@ -341,11 +350,9 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
uint32_t pIntid = 0;
uint64_t num_eventids;
uint32_t num_intids;
- bool dte_valid;
- MemTxResult res = MEMTX_OK;
uint16_t icid = 0;
- uint64_t dte = 0;
IteEntry ite = {};
+ DTEntry dte;
devid = (cmdpkt[0] & DEVID_MASK) >> DEVID_SHIFT;
eventid = cmdpkt[1] & EVENTID_MASK;
@@ -365,24 +372,21 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
return CMD_CONTINUE;
}
- dte = get_dte(s, devid, &res);
-
- if (res != MEMTX_OK) {
+ if (get_dte(s, devid, &dte) != MEMTX_OK) {
return CMD_STALL;
}
- dte_valid = FIELD_EX64(dte, DTE, VALID);
- num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
+ num_eventids = 1ULL << (dte.size + 1);
num_intids = 1ULL << (GICD_TYPER_IDBITS + 1);
if ((icid >= s->ct.num_entries)
- || !dte_valid || (eventid >= num_eventids) ||
+ || !dte.valid || (eventid >= num_eventids) ||
(((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) &&
(pIntid != INTID_SPURIOUS))) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes "
"icid %d or eventid %d or pIntid %d or"
"unmapped dte %d\n", __func__, icid, eventid,
- pIntid, dte_valid);
+ pIntid, dte.valid);
/*
* in this implementation, in case of error
* we ignore this command and move onto the next
@@ -392,13 +396,13 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
}
/* add ite entry to interrupt translation table */
- ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid);
+ ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, true);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid);
ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);
- return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+ return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
}
static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
@@ -561,10 +565,10 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
uint16_t old_icid, new_icid;
uint64_t old_cte, new_cte;
uint64_t old_rdbase, new_rdbase;
- uint64_t dte;
- bool dte_valid, ite_valid, cte_valid;
+ bool ite_valid, cte_valid;
uint64_t num_eventids;
IteEntry ite = {};
+ DTEntry dte;
devid = FIELD_EX64(cmdpkt[0], MOVI_0, DEVICEID);
eventid = FIELD_EX64(cmdpkt[1], MOVI_1, EVENTID);
@@ -576,21 +580,18 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
__func__, devid, s->dt.num_entries);
return CMD_CONTINUE;
}
- dte = get_dte(s, devid, &res);
- if (res != MEMTX_OK) {
+ if (get_dte(s, devid, &dte) != MEMTX_OK) {
return CMD_STALL;
}
- dte_valid = FIELD_EX64(dte, DTE, VALID);
- if (!dte_valid) {
+ if (!dte.valid) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: "
- "invalid dte: %"PRIx64" for %d\n",
- __func__, dte, devid);
+ "invalid dte for %d\n", __func__, devid);
return CMD_CONTINUE;
}
- num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
+ num_eventids = 1ULL << (dte.size + 1);
if (eventid >= num_eventids) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: eventid %d >= %"
@@ -599,7 +600,7 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
return CMD_CONTINUE;
}
- ite_valid = get_ite(s, eventid, dte, &old_icid, &intid, &res);
+ ite_valid = get_ite(s, eventid, &dte, &old_icid, &intid, &res);
if (res != MEMTX_OK) {
return CMD_STALL;
}
@@ -678,7 +679,7 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, intid);
ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, new_icid);
- return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+ return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 03/13] hw/intc/arm_gicv3_its: Pass DTEntry to update_dte()
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
2022-02-01 19:31 ` [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets Peter Maydell
2022-02-01 19:31 ` [PATCH 02/13] hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t Peter Maydell
@ 2022-02-01 19:31 ` Peter Maydell
2022-02-03 2:30 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 04/13] hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t Peter Maydell
` (10 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:31 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
Make update_dte() take a DTEntry struct rather than all the fields of
the new DTE as separate arguments.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 6d70d7d59e2..1856210e79a 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -465,20 +465,23 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
return update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL;
}
-static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
- uint8_t size, uint64_t itt_addr)
+/*
+ * Update the Device Table entry for @devid to @dte. Returns true
+ * on success, false if there was a memory access error.
+ */
+static bool update_dte(GICv3ITSState *s, uint32_t devid, const DTEntry *dte)
{
AddressSpace *as = &s->gicv3->dma_as;
uint64_t entry_addr;
- uint64_t dte = 0;
+ uint64_t dteval = 0;
MemTxResult res = MEMTX_OK;
if (s->dt.valid) {
- if (valid) {
+ if (dte->valid) {
/* add mapping entry to device table */
- dte = FIELD_DP64(dte, DTE, VALID, 1);
- dte = FIELD_DP64(dte, DTE, SIZE, size);
- dte = FIELD_DP64(dte, DTE, ITTADDR, itt_addr);
+ dteval = FIELD_DP64(dteval, DTE, VALID, 1);
+ dteval = FIELD_DP64(dteval, DTE, SIZE, dte->size);
+ dteval = FIELD_DP64(dteval, DTE, ITTADDR, dte->ittaddr);
}
} else {
return true;
@@ -493,27 +496,25 @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
/* No L2 table for this index: discard write and continue */
return true;
}
- address_space_stq_le(as, entry_addr, dte, MEMTXATTRS_UNSPECIFIED, &res);
+ address_space_stq_le(as, entry_addr, dteval, MEMTXATTRS_UNSPECIFIED, &res);
return res == MEMTX_OK;
}
static ItsCmdResult process_mapd(GICv3ITSState *s, const uint64_t *cmdpkt)
{
uint32_t devid;
- uint8_t size;
- uint64_t itt_addr;
- bool valid;
+ DTEntry dte;
devid = (cmdpkt[0] & DEVID_MASK) >> DEVID_SHIFT;
- size = cmdpkt[1] & SIZE_MASK;
- itt_addr = (cmdpkt[2] & ITTADDR_MASK) >> ITTADDR_SHIFT;
- valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
+ dte.size = cmdpkt[1] & SIZE_MASK;
+ dte.ittaddr = (cmdpkt[2] & ITTADDR_MASK) >> ITTADDR_SHIFT;
+ dte.valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
if ((devid >= s->dt.num_entries) ||
- (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
+ (dte.size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
qemu_log_mask(LOG_GUEST_ERROR,
"ITS MAPD: invalid device table attributes "
- "devid %d or size %d\n", devid, size);
+ "devid %d or size %d\n", devid, dte.size);
/*
* in this implementation, in case of error
* we ignore this command and move onto the next
@@ -522,7 +523,7 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, const uint64_t *cmdpkt)
return CMD_CONTINUE;
}
- return update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL;
+ return update_dte(s, devid, &dte) ? CMD_CONTINUE : CMD_STALL;
}
static ItsCmdResult process_movall(GICv3ITSState *s, const uint64_t *cmdpkt)
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 04/13] hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (2 preceding siblings ...)
2022-02-01 19:31 ` [PATCH 03/13] hw/intc/arm_gicv3_its: Pass DTEntry to update_dte() Peter Maydell
@ 2022-02-01 19:31 ` Peter Maydell
2022-02-03 2:58 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 05/13] hw/intc/arm_gicv3_its: Pass CTEntry to update_cte() Peter Maydell
` (9 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:31 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
In the ITS, a CTE is an entry in the collection table, which contains
multiple fields. Currently the function get_cte() which reads one
entry from the device table returns a success/failure boolean and
passes back the raw 64-bit integer CTE value via a pointer argument.
We then extract fields from the CTE as we need them.
Create a real C struct with the same fields as the CTE, and
populate it in get_cte(), so that that function and update_cte()
are the only ones which need to care about the in-guest-memory
format of the CTE.
This brings get_cte()'s API into line with get_dte().
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 96 ++++++++++++++++++++++-------------------
1 file changed, 52 insertions(+), 44 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 1856210e79a..482a71ba73c 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -52,6 +52,11 @@ typedef struct DTEntry {
uint64_t ittaddr;
} DTEntry;
+typedef struct CTEntry {
+ bool valid;
+ uint32_t rdbase;
+} CTEntry;
+
/*
* The ITS spec permits a range of CONSTRAINED UNPREDICTABLE options
* if a command parameter is not correct. These include both "stall
@@ -135,18 +140,32 @@ static uint64_t table_entry_addr(GICv3ITSState *s, TableDesc *td,
return (l2 & ((1ULL << 51) - 1)) + (idx % num_l2_entries) * td->entry_sz;
}
-static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte,
- MemTxResult *res)
+/*
+ * Read the Collection Table entry at index @icid. On success (including
+ * successfully determining that there is no valid CTE for this index),
+ * we return MEMTX_OK and populate the CTEntry struct @cte accordingly.
+ * If there is an error reading memory then we return the error code.
+ */
+static MemTxResult get_cte(GICv3ITSState *s, uint16_t icid, CTEntry *cte)
{
AddressSpace *as = &s->gicv3->dma_as;
- uint64_t entry_addr = table_entry_addr(s, &s->ct, icid, res);
+ MemTxResult res = MEMTX_OK;
+ uint64_t entry_addr = table_entry_addr(s, &s->ct, icid, &res);
+ uint64_t cteval;
if (entry_addr == -1) {
- return false; /* not valid */
+ /* No L2 table entry, i.e. no valid CTE, or a memory error */
+ cte->valid = false;
+ return res;
}
- *cte = address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, res);
- return FIELD_EX64(*cte, CTE, VALID);
+ cteval = address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, &res);
+ if (res != MEMTX_OK) {
+ return res;
+ }
+ cte->valid = FIELD_EX64(cteval, CTE, VALID);
+ cte->rdbase = FIELD_EX64(cteval, CTE, RDBASE);
+ return MEMTX_OK;
}
static bool update_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
@@ -248,10 +267,8 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
uint16_t icid = 0;
uint32_t pIntid = 0;
bool ite_valid = false;
- uint64_t cte = 0;
- bool cte_valid = false;
- uint64_t rdbase;
DTEntry dte;
+ CTEntry cte;
if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -298,15 +315,13 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
return CMD_CONTINUE;
}
- cte_valid = get_cte(s, icid, &cte, &res);
- if (res != MEMTX_OK) {
+ if (get_cte(s, icid, &cte) != MEMTX_OK) {
return CMD_STALL;
}
- if (!cte_valid) {
+ if (!cte.valid) {
qemu_log_mask(LOG_GUEST_ERROR,
- "%s: invalid command attributes: "
- "invalid cte: %"PRIx64"\n",
- __func__, cte);
+ "%s: invalid command attributes: invalid CTE\n",
+ __func__);
return CMD_CONTINUE;
}
@@ -314,16 +329,14 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
* Current implementation only supports rdbase == procnum
* Hence rdbase physical address is ignored
*/
- rdbase = FIELD_EX64(cte, CTE, RDBASE);
-
- if (rdbase >= s->gicv3->num_cpu) {
+ if (cte.rdbase >= s->gicv3->num_cpu) {
return CMD_CONTINUE;
}
if ((cmd == CLEAR) || (cmd == DISCARD)) {
- gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0);
+ gicv3_redist_process_lpi(&s->gicv3->cpu[cte.rdbase], pIntid, 0);
} else {
- gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1);
+ gicv3_redist_process_lpi(&s->gicv3->cpu[cte.rdbase], pIntid, 1);
}
if (cmd == DISCARD) {
@@ -564,12 +577,11 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
MemTxResult res = MEMTX_OK;
uint32_t devid, eventid, intid;
uint16_t old_icid, new_icid;
- uint64_t old_cte, new_cte;
- uint64_t old_rdbase, new_rdbase;
- bool ite_valid, cte_valid;
+ bool ite_valid;
uint64_t num_eventids;
IteEntry ite = {};
DTEntry dte;
+ CTEntry old_cte, new_cte;
devid = FIELD_EX64(cmdpkt[0], MOVI_0, DEVICEID);
eventid = FIELD_EX64(cmdpkt[1], MOVI_1, EVENTID);
@@ -627,50 +639,46 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
return CMD_CONTINUE;
}
- cte_valid = get_cte(s, old_icid, &old_cte, &res);
- if (res != MEMTX_OK) {
+ if (get_cte(s, old_icid, &old_cte) != MEMTX_OK) {
return CMD_STALL;
}
- if (!cte_valid) {
+ if (!old_cte.valid) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: "
- "invalid cte: %"PRIx64"\n",
- __func__, old_cte);
+ "invalid CTE for old ICID 0x%x\n",
+ __func__, old_icid);
return CMD_CONTINUE;
}
- cte_valid = get_cte(s, new_icid, &new_cte, &res);
- if (res != MEMTX_OK) {
+ if (get_cte(s, new_icid, &new_cte) != MEMTX_OK) {
return CMD_STALL;
}
- if (!cte_valid) {
+ if (!new_cte.valid) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: "
- "invalid cte: %"PRIx64"\n",
- __func__, new_cte);
+ "invalid CTE for new ICID 0x%x\n",
+ __func__, new_icid);
return CMD_CONTINUE;
}
- old_rdbase = FIELD_EX64(old_cte, CTE, RDBASE);
- if (old_rdbase >= s->gicv3->num_cpu) {
+ if (old_cte.rdbase >= s->gicv3->num_cpu) {
qemu_log_mask(LOG_GUEST_ERROR,
- "%s: CTE has invalid rdbase 0x%"PRIx64"\n",
- __func__, old_rdbase);
+ "%s: CTE has invalid rdbase 0x%x\n",
+ __func__, old_cte.rdbase);
return CMD_CONTINUE;
}
- new_rdbase = FIELD_EX64(new_cte, CTE, RDBASE);
- if (new_rdbase >= s->gicv3->num_cpu) {
+ if (new_cte.rdbase >= s->gicv3->num_cpu) {
qemu_log_mask(LOG_GUEST_ERROR,
- "%s: CTE has invalid rdbase 0x%"PRIx64"\n",
- __func__, new_rdbase);
+ "%s: CTE has invalid rdbase 0x%x\n",
+ __func__, new_cte.rdbase);
return CMD_CONTINUE;
}
- if (old_rdbase != new_rdbase) {
+ if (old_cte.rdbase != new_cte.rdbase) {
/* Move the LPI from the old redistributor to the new one */
- gicv3_redist_mov_lpi(&s->gicv3->cpu[old_rdbase],
- &s->gicv3->cpu[new_rdbase],
+ gicv3_redist_mov_lpi(&s->gicv3->cpu[old_cte.rdbase],
+ &s->gicv3->cpu[new_cte.rdbase],
intid);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 05/13] hw/intc/arm_gicv3_its: Pass CTEntry to update_cte()
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (3 preceding siblings ...)
2022-02-01 19:31 ` [PATCH 04/13] hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t Peter Maydell
@ 2022-02-01 19:31 ` Peter Maydell
2022-02-03 3:00 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite() Peter Maydell
` (8 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:31 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
Make update_cte() take a CTEntry struct rather than all the fields
of the new CTE as separate arguments.
This brings it into line with the update_dte() API.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 482a71ba73c..b94775fd379 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -418,22 +418,25 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
}
-static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
- uint64_t rdbase)
+/*
+ * Update the Collection Table entry for @icid to @cte. Returns true
+ * on success, false if there was a memory access error.
+ */
+static bool update_cte(GICv3ITSState *s, uint16_t icid, const CTEntry *cte)
{
AddressSpace *as = &s->gicv3->dma_as;
uint64_t entry_addr;
- uint64_t cte = 0;
+ uint64_t cteval = 0;
MemTxResult res = MEMTX_OK;
if (!s->ct.valid) {
return true;
}
- if (valid) {
+ if (cte->valid) {
/* add mapping entry to collection table */
- cte = FIELD_DP64(cte, CTE, VALID, 1);
- cte = FIELD_DP64(cte, CTE, RDBASE, rdbase);
+ cteval = FIELD_DP64(cteval, CTE, VALID, 1);
+ cteval = FIELD_DP64(cteval, CTE, RDBASE, cte->rdbase);
}
entry_addr = table_entry_addr(s, &s->ct, icid, &res);
@@ -446,27 +449,26 @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
return true;
}
- address_space_stq_le(as, entry_addr, cte, MEMTXATTRS_UNSPECIFIED, &res);
+ address_space_stq_le(as, entry_addr, cteval, MEMTXATTRS_UNSPECIFIED, &res);
return res == MEMTX_OK;
}
static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
{
uint16_t icid;
- uint64_t rdbase;
- bool valid;
+ CTEntry cte;
icid = cmdpkt[2] & ICID_MASK;
- rdbase = (cmdpkt[2] & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT;
- rdbase &= RDBASE_PROCNUM_MASK;
+ cte.rdbase = (cmdpkt[2] & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT;
+ cte.rdbase &= RDBASE_PROCNUM_MASK;
- valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
+ cte.valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
- if ((icid >= s->ct.num_entries) || (rdbase >= s->gicv3->num_cpu)) {
+ if ((icid >= s->ct.num_entries) || (cte.rdbase >= s->gicv3->num_cpu)) {
qemu_log_mask(LOG_GUEST_ERROR,
"ITS MAPC: invalid collection table attributes "
- "icid %d rdbase %" PRIu64 "\n", icid, rdbase);
+ "icid %d rdbase %u\n", icid, cte.rdbase);
/*
* in this implementation, in case of error
* we ignore this command and move onto the next
@@ -475,7 +477,7 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
return CMD_CONTINUE;
}
- return update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL;
+ return update_cte(s, icid, &cte) ? CMD_CONTINUE : CMD_STALL;
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite()
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (4 preceding siblings ...)
2022-02-01 19:31 ` [PATCH 05/13] hw/intc/arm_gicv3_its: Pass CTEntry to update_cte() Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 3:59 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 07/13] hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite() Peter Maydell
` (7 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
In get_ite() and update_ite() we work with a 12-byte in-guest-memory
table entry, which we intend to handle as an 8-byte value followed by
a 4-byte value. Unfortunately the calculation of the address of the
4-byte value is wrong, because we write it as:
table_base_address + (index * entrysize) + 4
(obfuscated by the way the expression has been written)
when it should be + 8. This bug meant that we overwrote the top
bytes of the 8-byte value with the 4-byte value. There are no
guest-visible effects because the top half of the 8-byte value
contains only the doorbell interrupt field, which is used only in
GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths
cancel each other out.
We can't simply change the calculation, because this would break
migration of a (TCG) guest from the old version of QEMU which had
in-guest-memory interrupt tables written using the buggy version of
update_ite(). We must also at the same time change the layout of the
fields within the ITE_L and ITE_H values so that the in-memory
locations of the fields we care about (VALID, INTTYPE, INTID and
ICID) stay the same.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/gicv3_internal.h | 19 ++++++++++---------
hw/intc/arm_gicv3_its.c | 28 +++++++++++-----------------
2 files changed, 21 insertions(+), 26 deletions(-)
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 60c8617e4e4..2bf1baef047 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -370,22 +370,23 @@ FIELD(MOVI_2, ICID, 0, 16)
* 12 bytes Interrupt translation Table Entry size
* as per Table 5.3 in GICv3 spec
* ITE Lower 8 Bytes
- * Bits: | 49 ... 26 | 25 ... 2 | 1 | 0 |
- * Values: | Doorbell | IntNum | IntType | Valid |
+ * Bits: | 63 ... 48 | 47 ... 32 | 31 ... 26 | 25 ... 2 | 1 | 0 |
+ * Values: | vPEID | ICID | unused | IntNum | IntType | Valid |
* ITE Higher 4 Bytes
- * Bits: | 31 ... 16 | 15 ...0 |
- * Values: | vPEID | ICID |
- * (When Doorbell is unused, as it always is in GICv3, it is 1023)
+ * Bits: | 31 ... 25 | 24 ... 0 |
+ * Values: | unused | Doorbell |
+ * (When Doorbell is unused, as it always is for INTYPE_PHYSICAL,
+ * the value of that field in memory cannot be relied upon -- older
+ * versions of QEMU did not correctly write to that memory.)
*/
#define ITS_ITT_ENTRY_SIZE 0xC
FIELD(ITE_L, VALID, 0, 1)
FIELD(ITE_L, INTTYPE, 1, 1)
FIELD(ITE_L, INTID, 2, 24)
-FIELD(ITE_L, DOORBELL, 26, 24)
-
-FIELD(ITE_H, ICID, 0, 16)
-FIELD(ITE_H, VPEID, 16, 16)
+FIELD(ITE_L, ICID, 32, 16)
+FIELD(ITE_L, VPEID, 48, 16)
+FIELD(ITE_H, DOORBELL, 0, 24)
/* Possible values for ITE_L INTTYPE */
#define ITE_INTTYPE_VIRTUAL 0
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index b94775fd379..48eaf20a6c9 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -173,14 +173,12 @@ static bool update_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
{
AddressSpace *as = &s->gicv3->dma_as;
MemTxResult res = MEMTX_OK;
+ hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
- address_space_stq_le(as, dte->ittaddr + (eventid * (sizeof(uint64_t) +
- sizeof(uint32_t))), ite.itel, MEMTXATTRS_UNSPECIFIED,
- &res);
+ address_space_stq_le(as, iteaddr, ite.itel, MEMTXATTRS_UNSPECIFIED, &res);
if (res == MEMTX_OK) {
- address_space_stl_le(as, dte->ittaddr + (eventid * (sizeof(uint64_t) +
- sizeof(uint32_t))) + sizeof(uint32_t), ite.iteh,
+ address_space_stl_le(as, iteaddr + 8, ite.iteh,
MEMTXATTRS_UNSPECIFIED, &res);
}
if (res != MEMTX_OK) {
@@ -196,16 +194,12 @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
AddressSpace *as = &s->gicv3->dma_as;
bool status = false;
IteEntry ite = {};
+ hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
- ite.itel = address_space_ldq_le(as, dte->ittaddr +
- (eventid * (sizeof(uint64_t) +
- sizeof(uint32_t))), MEMTXATTRS_UNSPECIFIED,
- res);
+ ite.itel = address_space_ldq_le(as, iteaddr, MEMTXATTRS_UNSPECIFIED, res);
if (*res == MEMTX_OK) {
- ite.iteh = address_space_ldl_le(as, dte->ittaddr +
- (eventid * (sizeof(uint64_t) +
- sizeof(uint32_t))) + sizeof(uint32_t),
+ ite.iteh = address_space_ldl_le(as, iteaddr + 8,
MEMTXATTRS_UNSPECIFIED, res);
if (*res == MEMTX_OK) {
@@ -213,7 +207,7 @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
int inttype = FIELD_EX64(ite.itel, ITE_L, INTTYPE);
if (inttype == ITE_INTTYPE_PHYSICAL) {
*pIntid = FIELD_EX64(ite.itel, ITE_L, INTID);
- *icid = FIELD_EX32(ite.iteh, ITE_H, ICID);
+ *icid = FIELD_EX64(ite.itel, ITE_L, ICID);
status = true;
}
}
@@ -412,8 +406,8 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, true);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
- ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);
+ ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, icid);
+ ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS);
return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
}
@@ -688,8 +682,8 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, 1);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, intid);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
- ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, new_icid);
+ ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, new_icid);
+ ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS);
return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 07/13] hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite()
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (5 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite() Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:01 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 08/13] hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a struct Peter Maydell
` (6 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
The get_ite() code has some awkward nested if statements; clean
them up by returning early if the memory accesses fail.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 48eaf20a6c9..6975a349f62 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -197,20 +197,22 @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
ite.itel = address_space_ldq_le(as, iteaddr, MEMTXATTRS_UNSPECIFIED, res);
+ if (*res != MEMTX_OK) {
+ return false;
+ }
- if (*res == MEMTX_OK) {
- ite.iteh = address_space_ldl_le(as, iteaddr + 8,
- MEMTXATTRS_UNSPECIFIED, res);
+ ite.iteh = address_space_ldl_le(as, iteaddr + 8,
+ MEMTXATTRS_UNSPECIFIED, res);
+ if (*res != MEMTX_OK) {
+ return false;
+ }
- if (*res == MEMTX_OK) {
- if (FIELD_EX64(ite.itel, ITE_L, VALID)) {
- int inttype = FIELD_EX64(ite.itel, ITE_L, INTTYPE);
- if (inttype == ITE_INTTYPE_PHYSICAL) {
- *pIntid = FIELD_EX64(ite.itel, ITE_L, INTID);
- *icid = FIELD_EX64(ite.itel, ITE_L, ICID);
- status = true;
- }
- }
+ if (FIELD_EX64(ite.itel, ITE_L, VALID)) {
+ int inttype = FIELD_EX64(ite.itel, ITE_L, INTTYPE);
+ if (inttype == ITE_INTTYPE_PHYSICAL) {
+ *pIntid = FIELD_EX64(ite.itel, ITE_L, INTID);
+ *icid = FIELD_EX64(ite.itel, ITE_L, ICID);
+ status = true;
}
}
return status;
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 08/13] hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a struct
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (6 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 07/13] hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite() Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:05 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 09/13] hw/intc/arm_gicv3_its: Make update_ite() use ITEntry Peter Maydell
` (5 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
In get_ite() we currently return the caller some of the fields of an
Interrupt Table Entry via a set of pointer arguments, and validate
some of them internally (interrupt type and valid bit) to return a
simple true/false 'valid' indication. Define a new ITEntry struct
which has all the fields that the in-memory ITE has, and bring the
get_ite() function in to line with get_dte() and get_cte().
This paves the way for handling virtual interrupts, which will want
a different subset of the fields in the ITE. Handling them under
the old "lots of pointer arguments" scheme would have meant a
confusingly large set of arguments for this function.
The new struct ITEntry is obviously confusably similar to the
existing IteEntry struct, whose fields are the raw 12 bytes
of the in-memory ITE. In the next commit we will make update_ite()
use ITEntry instead of IteEntry, which will allow us to delete
the IteEntry struct and remove the confusion.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 102 ++++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 47 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 6975a349f62..bd741085022 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -57,6 +57,16 @@ typedef struct CTEntry {
uint32_t rdbase;
} CTEntry;
+typedef struct ITEntry {
+ bool valid;
+ int inttype;
+ uint32_t intid;
+ uint32_t doorbell;
+ uint32_t icid;
+ uint32_t vpeid;
+} ITEntry;
+
+
/*
* The ITS spec permits a range of CONSTRAINED UNPREDICTABLE options
* if a command parameter is not correct. These include both "stall
@@ -188,34 +198,38 @@ static bool update_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
}
}
-static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
- uint16_t *icid, uint32_t *pIntid, MemTxResult *res)
+/*
+ * Read the Interrupt Table entry at index @eventid from the table specified
+ * by the DTE @dte. On success, we return MEMTX_OK and populate the ITEntry
+ * struct @ite accordingly. If there is an error reading memory then we return
+ * the error code.
+ */
+static MemTxResult get_ite(GICv3ITSState *s, uint32_t eventid,
+ const DTEntry *dte, ITEntry *ite)
{
AddressSpace *as = &s->gicv3->dma_as;
- bool status = false;
- IteEntry ite = {};
+ MemTxResult res = MEMTX_OK;
+ uint64_t itel;
+ uint32_t iteh;
hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
- ite.itel = address_space_ldq_le(as, iteaddr, MEMTXATTRS_UNSPECIFIED, res);
- if (*res != MEMTX_OK) {
- return false;
+ itel = address_space_ldq_le(as, iteaddr, MEMTXATTRS_UNSPECIFIED, &res);
+ if (res != MEMTX_OK) {
+ return res;
}
- ite.iteh = address_space_ldl_le(as, iteaddr + 8,
- MEMTXATTRS_UNSPECIFIED, res);
- if (*res != MEMTX_OK) {
- return false;
+ iteh = address_space_ldl_le(as, iteaddr + 8, MEMTXATTRS_UNSPECIFIED, &res);
+ if (res != MEMTX_OK) {
+ return res;
}
- if (FIELD_EX64(ite.itel, ITE_L, VALID)) {
- int inttype = FIELD_EX64(ite.itel, ITE_L, INTTYPE);
- if (inttype == ITE_INTTYPE_PHYSICAL) {
- *pIntid = FIELD_EX64(ite.itel, ITE_L, INTID);
- *icid = FIELD_EX64(ite.itel, ITE_L, ICID);
- status = true;
- }
- }
- return status;
+ ite->valid = FIELD_EX64(itel, ITE_L, VALID);
+ ite->inttype = FIELD_EX64(itel, ITE_L, INTTYPE);
+ ite->intid = FIELD_EX64(itel, ITE_L, INTID);
+ ite->icid = FIELD_EX64(itel, ITE_L, ICID);
+ ite->vpeid = FIELD_EX64(itel, ITE_L, VPEID);
+ ite->doorbell = FIELD_EX64(iteh, ITE_H, DOORBELL);
+ return MEMTX_OK;
}
/*
@@ -258,13 +272,10 @@ static MemTxResult get_dte(GICv3ITSState *s, uint32_t devid, DTEntry *dte)
static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
uint32_t eventid, ItsCmdType cmd)
{
- MemTxResult res = MEMTX_OK;
uint64_t num_eventids;
- uint16_t icid = 0;
- uint32_t pIntid = 0;
- bool ite_valid = false;
DTEntry dte;
CTEntry cte;
+ ITEntry ite;
if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -292,26 +303,25 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
return CMD_CONTINUE;
}
- ite_valid = get_ite(s, eventid, &dte, &icid, &pIntid, &res);
- if (res != MEMTX_OK) {
+ if (get_ite(s, eventid, &dte, &ite) != MEMTX_OK) {
return CMD_STALL;
}
- if (!ite_valid) {
+ if (!ite.valid || ite.inttype != ITE_INTTYPE_PHYSICAL) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: invalid ITE\n",
__func__);
return CMD_CONTINUE;
}
- if (icid >= s->ct.num_entries) {
+ if (ite.icid >= s->ct.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid ICID 0x%x in ITE (table corrupted?)\n",
- __func__, icid);
+ __func__, ite.icid);
return CMD_CONTINUE;
}
- if (get_cte(s, icid, &cte) != MEMTX_OK) {
+ if (get_cte(s, ite.icid, &cte) != MEMTX_OK) {
return CMD_STALL;
}
if (!cte.valid) {
@@ -330,15 +340,15 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
}
if ((cmd == CLEAR) || (cmd == DISCARD)) {
- gicv3_redist_process_lpi(&s->gicv3->cpu[cte.rdbase], pIntid, 0);
+ gicv3_redist_process_lpi(&s->gicv3->cpu[cte.rdbase], ite.intid, 0);
} else {
- gicv3_redist_process_lpi(&s->gicv3->cpu[cte.rdbase], pIntid, 1);
+ gicv3_redist_process_lpi(&s->gicv3->cpu[cte.rdbase], ite.intid, 1);
}
if (cmd == DISCARD) {
- IteEntry ite = {};
+ IteEntry itee = {};
/* remove mapping from interrupt translation table */
- return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
+ return update_ite(s, eventid, &dte, itee) ? CMD_CONTINUE : CMD_STALL;
}
return CMD_CONTINUE;
}
@@ -572,14 +582,13 @@ static ItsCmdResult process_movall(GICv3ITSState *s, const uint64_t *cmdpkt)
static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
{
- MemTxResult res = MEMTX_OK;
- uint32_t devid, eventid, intid;
- uint16_t old_icid, new_icid;
- bool ite_valid;
+ uint32_t devid, eventid;
+ uint16_t new_icid;
uint64_t num_eventids;
IteEntry ite = {};
DTEntry dte;
CTEntry old_cte, new_cte;
+ ITEntry old_ite;
devid = FIELD_EX64(cmdpkt[0], MOVI_0, DEVICEID);
eventid = FIELD_EX64(cmdpkt[1], MOVI_1, EVENTID);
@@ -611,22 +620,21 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
return CMD_CONTINUE;
}
- ite_valid = get_ite(s, eventid, &dte, &old_icid, &intid, &res);
- if (res != MEMTX_OK) {
+ if (get_ite(s, eventid, &dte, &old_ite) != MEMTX_OK) {
return CMD_STALL;
}
- if (!ite_valid) {
+ if (!old_ite.valid || old_ite.inttype != ITE_INTTYPE_PHYSICAL) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: invalid ITE\n",
__func__);
return CMD_CONTINUE;
}
- if (old_icid >= s->ct.num_entries) {
+ if (old_ite.icid >= s->ct.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid ICID 0x%x in ITE (table corrupted?)\n",
- __func__, old_icid);
+ __func__, old_ite.icid);
return CMD_CONTINUE;
}
@@ -637,14 +645,14 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
return CMD_CONTINUE;
}
- if (get_cte(s, old_icid, &old_cte) != MEMTX_OK) {
+ if (get_cte(s, old_ite.icid, &old_cte) != MEMTX_OK) {
return CMD_STALL;
}
if (!old_cte.valid) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes: "
"invalid CTE for old ICID 0x%x\n",
- __func__, old_icid);
+ __func__, old_ite.icid);
return CMD_CONTINUE;
}
@@ -677,13 +685,13 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
/* Move the LPI from the old redistributor to the new one */
gicv3_redist_mov_lpi(&s->gicv3->cpu[old_cte.rdbase],
&s->gicv3->cpu[new_cte.rdbase],
- intid);
+ old_ite.intid);
}
/* Update the ICID field in the interrupt translation table entry */
ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, 1);
ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, intid);
+ ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, old_ite.intid);
ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, new_icid);
ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS);
return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 09/13] hw/intc/arm_gicv3_its: Make update_ite() use ITEntry
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (7 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 08/13] hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a struct Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:09 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 10/13] hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields Peter Maydell
` (4 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
Make the update_ite() struct use the new ITEntry struct, so that
callers don't need to assemble the in-memory ITE data themselves, and
only get_ite() and update_ite() need to care about that in-memory
layout. We can then drop the no-longer-used IteEntry struct
definition.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 62 +++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index bd741085022..e3b63efddcc 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -41,11 +41,6 @@ typedef enum ItsCmdType {
INTERRUPT = 3,
} ItsCmdType;
-typedef struct {
- uint32_t iteh;
- uint64_t itel;
-} IteEntry;
-
typedef struct DTEntry {
bool valid;
unsigned size;
@@ -178,24 +173,35 @@ static MemTxResult get_cte(GICv3ITSState *s, uint16_t icid, CTEntry *cte)
return MEMTX_OK;
}
+/*
+ * Update the Interrupt Table entry at index @evinted in the table specified
+ * by the dte @dte. Returns true on success, false if there was a memory
+ * access error.
+ */
static bool update_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
- IteEntry ite)
+ const ITEntry *ite)
{
AddressSpace *as = &s->gicv3->dma_as;
MemTxResult res = MEMTX_OK;
hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
+ uint64_t itel = 0;
+ uint32_t iteh = 0;
- address_space_stq_le(as, iteaddr, ite.itel, MEMTXATTRS_UNSPECIFIED, &res);
-
- if (res == MEMTX_OK) {
- address_space_stl_le(as, iteaddr + 8, ite.iteh,
- MEMTXATTRS_UNSPECIFIED, &res);
+ if (ite->valid) {
+ itel = FIELD_DP64(itel, ITE_L, VALID, 1);
+ itel = FIELD_DP64(itel, ITE_L, INTTYPE, ite->inttype);
+ itel = FIELD_DP64(itel, ITE_L, INTID, ite->intid);
+ itel = FIELD_DP64(itel, ITE_L, ICID, ite->icid);
+ itel = FIELD_DP64(itel, ITE_L, VPEID, ite->vpeid);
+ iteh = FIELD_DP32(iteh, ITE_H, DOORBELL, ite->doorbell);
}
+
+ address_space_stq_le(as, iteaddr, itel, MEMTXATTRS_UNSPECIFIED, &res);
if (res != MEMTX_OK) {
return false;
- } else {
- return true;
}
+ address_space_stl_le(as, iteaddr + 8, iteh, MEMTXATTRS_UNSPECIFIED, &res);
+ return res == MEMTX_OK;
}
/*
@@ -346,9 +352,10 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s, uint32_t devid,
}
if (cmd == DISCARD) {
- IteEntry itee = {};
+ ITEntry ite = {};
/* remove mapping from interrupt translation table */
- return update_ite(s, eventid, &dte, itee) ? CMD_CONTINUE : CMD_STALL;
+ ite.valid = false;
+ return update_ite(s, eventid, &dte, &ite) ? CMD_CONTINUE : CMD_STALL;
}
return CMD_CONTINUE;
}
@@ -370,8 +377,8 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
uint64_t num_eventids;
uint32_t num_intids;
uint16_t icid = 0;
- IteEntry ite = {};
DTEntry dte;
+ ITEntry ite;
devid = (cmdpkt[0] & DEVID_MASK) >> DEVID_SHIFT;
eventid = cmdpkt[1] & EVENTID_MASK;
@@ -415,13 +422,13 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
}
/* add ite entry to interrupt translation table */
- ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, true);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, icid);
- ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS);
-
- return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
+ ite.valid = true;
+ ite.inttype = ITE_INTTYPE_PHYSICAL;
+ ite.intid = pIntid;
+ ite.icid = icid;
+ ite.doorbell = INTID_SPURIOUS;
+ ite.vpeid = 0;
+ return update_ite(s, eventid, &dte, &ite) ? CMD_CONTINUE : CMD_STALL;
}
/*
@@ -585,7 +592,6 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
uint32_t devid, eventid;
uint16_t new_icid;
uint64_t num_eventids;
- IteEntry ite = {};
DTEntry dte;
CTEntry old_cte, new_cte;
ITEntry old_ite;
@@ -689,12 +695,8 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt)
}
/* Update the ICID field in the interrupt translation table entry */
- ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, 1);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, old_ite.intid);
- ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, new_icid);
- ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS);
- return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL;
+ old_ite.icid = new_icid;
+ return update_ite(s, eventid, &dte, &old_ite) ? CMD_CONTINUE : CMD_STALL;
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 10/13] hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (8 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 09/13] hw/intc/arm_gicv3_its: Make update_ite() use ITEntry Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:18 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 11/13] hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field Peter Maydell
` (3 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
Currently we track in the TableDesc and CmdQDesc structs the state of
the GITS_BASER<n> and GITS_CBASER Valid bits. However we aren't very
consistent abut checking the valid field: we test it in update_cte()
and update_dte(), but not anywhere else we look things up in tables.
The GIC specification says that it is UNPREDICTABLE if a guest fails
to set any of these Valid bits before enabling the ITS via
GITS_CTLR.Enabled. So we can choose to handle Valid == 0 as
equivalent to a zero-length table. This is in fact how we're already
catching this case in most of the table-access paths: when Valid is 0
we leave the num_entries fields in TableDesc or CmdQDesc set to zero,
and then the out-of-bounds check "index >= num_entries" that we have
to do anyway before doing any of these table lookups will always be
true, catching the no-valid-table case without any extra code.
So we can remove the checks on the valid field from update_cte()
and update_dte(): since these happen after the bounds check there
was never any case when the test could fail. That means the valid
fields would be entirely unused, so just remove them.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
include/hw/intc/arm_gicv3_its_common.h | 2 --
hw/intc/arm_gicv3_its.c | 31 ++++++++++++--------------
2 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 3e2ad2dff60..0f130494dd3 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -42,7 +42,6 @@
#define GITS_TRANSLATER 0x0040
typedef struct {
- bool valid;
bool indirect;
uint16_t entry_sz;
uint32_t page_sz;
@@ -51,7 +50,6 @@ typedef struct {
} TableDesc;
typedef struct {
- bool valid;
uint32_t num_entries;
uint64_t base_addr;
} CmdQDesc;
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index e3b63efddcc..9735d609df2 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -442,10 +442,6 @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, const CTEntry *cte)
uint64_t cteval = 0;
MemTxResult res = MEMTX_OK;
- if (!s->ct.valid) {
- return true;
- }
-
if (cte->valid) {
/* add mapping entry to collection table */
cteval = FIELD_DP64(cteval, CTE, VALID, 1);
@@ -504,15 +500,11 @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, const DTEntry *dte)
uint64_t dteval = 0;
MemTxResult res = MEMTX_OK;
- if (s->dt.valid) {
- if (dte->valid) {
- /* add mapping entry to device table */
- dteval = FIELD_DP64(dteval, DTE, VALID, 1);
- dteval = FIELD_DP64(dteval, DTE, SIZE, dte->size);
- dteval = FIELD_DP64(dteval, DTE, ITTADDR, dte->ittaddr);
- }
- } else {
- return true;
+ if (dte->valid) {
+ /* add mapping entry to device table */
+ dteval = FIELD_DP64(dteval, DTE, VALID, 1);
+ dteval = FIELD_DP64(dteval, DTE, SIZE, dte->size);
+ dteval = FIELD_DP64(dteval, DTE, ITTADDR, dte->ittaddr);
}
entry_addr = table_entry_addr(s, &s->dt, devid, &res);
@@ -901,7 +893,6 @@ static void extract_table_params(GICv3ITSState *s)
}
memset(td, 0, sizeof(*td));
- td->valid = FIELD_EX64(value, GITS_BASER, VALID);
/*
* If GITS_BASER<n>.Valid is 0 for any <n> then we will not process
* interrupts. (GITS_TYPER.HCC is 0 for this implementation, so we
@@ -909,8 +900,15 @@ static void extract_table_params(GICv3ITSState *s)
* for the register corresponding to the Collection table but we
* still have to process interrupts using non-memory-backed
* Collection table entries.)
+ * The specification makes it UNPREDICTABLE to enable the ITS without
+ * marking each BASER<n> as valid. We choose to handle these as if
+ * the table was zero-sized, so commands using the table will fail
+ * and interrupts requested via GITS_TRANSLATER writes will be ignored.
+ * This happens automatically by leaving the num_entries field at
+ * zero, which will be caught by the bounds checks we have before
+ * every table lookup anyway.
*/
- if (!td->valid) {
+ if (!FIELD_EX64(value, GITS_BASER, VALID)) {
continue;
}
td->page_sz = page_sz;
@@ -936,9 +934,8 @@ static void extract_cmdq_params(GICv3ITSState *s)
num_pages = FIELD_EX64(value, GITS_CBASER, SIZE) + 1;
memset(&s->cq, 0 , sizeof(s->cq));
- s->cq.valid = FIELD_EX64(value, GITS_CBASER, VALID);
- if (s->cq.valid) {
+ if (FIELD_EX64(value, GITS_CBASER, VALID)) {
s->cq.num_entries = (num_pages * GITS_PAGE_SIZE_4K) /
GITS_CMDQ_ENTRY_SIZE;
s->cq.base_addr = FIELD_EX64(value, GITS_CBASER, PHYADDR);
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 11/13] hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (9 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 10/13] hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:24 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 12/13] hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI Peter Maydell
` (2 subsequent siblings)
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
In the MAPC command, if V=0 this is a request to delete a collection
table entry and the rdbase field of the command packet will not be
used. In particular, the specification says that the "UNPREDICTABLE
if rdbase is not valid" only applies for V=1.
We were doing a check-and-log-guest-error on rdbase regardless of
whether the V bit was set, and also (harmlessly but confusingly)
storing the contents of the rdbase field into the updated collection
table entry. Update the code so that if V=0 we don't check or use
the rdbase field value.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 9735d609df2..069991f7f36 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -468,21 +468,21 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
CTEntry cte;
icid = cmdpkt[2] & ICID_MASK;
-
- cte.rdbase = (cmdpkt[2] & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT;
- cte.rdbase &= RDBASE_PROCNUM_MASK;
-
cte.valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
+ if (cte.valid) {
+ cte.rdbase = (cmdpkt[2] & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT;
+ cte.rdbase &= RDBASE_PROCNUM_MASK;
+ } else {
+ cte.rdbase = 0;
+ }
- if ((icid >= s->ct.num_entries) || (cte.rdbase >= s->gicv3->num_cpu)) {
+ if (icid >= s->ct.num_entries) {
+ qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d", icid);
+ return CMD_CONTINUE;
+ }
+ if (cte.valid && cte.rdbase >= s->gicv3->num_cpu) {
qemu_log_mask(LOG_GUEST_ERROR,
- "ITS MAPC: invalid collection table attributes "
- "icid %d rdbase %u\n", icid, cte.rdbase);
- /*
- * in this implementation, in case of error
- * we ignore this command and move onto the next
- * command in the queue
- */
+ "ITS MAPC: invalid RDBASE %u ", cte.rdbase);
return CMD_CONTINUE;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 12/13] hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (10 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 11/13] hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:25 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 13/13] hw/intc/arm_gicv3_its: Split error checks Peter Maydell
2022-02-07 17:56 ` [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
When handling MAPI/MAPTI, we allow the supplied interrupt ID to be
either 1023 or something in the valid LPI range. This is a mistake:
only a real valid LPI is allowed. (The general behaviour of the ITS
is that most interrupt ID fields require a value in the LPI range;
the exception is that fields specifying a doorbell value, which are
all in GICv4 commands, allow also 1023 to mean "no doorbell".)
Remove the condition that incorrectly allows 1023 here.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This one's my fault -- Shashi's original code did the right thing,
IIRC. The spec text and pseudocode disagree here, and in code review
I backed the wrong horse. Sorry.
---
hw/intc/arm_gicv3_its.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 069991f7f36..8dade9440ac 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -406,8 +406,7 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
if ((icid >= s->ct.num_entries)
|| !dte.valid || (eventid >= num_eventids) ||
- (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) &&
- (pIntid != INTID_SPURIOUS))) {
+ (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)))) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid command attributes "
"icid %d or eventid %d or pIntid %d or"
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 13/13] hw/intc/arm_gicv3_its: Split error checks
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (11 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 12/13] hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI Peter Maydell
@ 2022-02-01 19:32 ` Peter Maydell
2022-02-03 4:26 ` Richard Henderson
2022-02-07 17:56 ` [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
13 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-01 19:32 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
In most of the ITS command processing, we check different error
possibilities one at a time and log them appropriately. In
process_mapti() and process_mapd() we have code which checks
multiple error cases at once, which means the logging is less
specific than it could be. Split those cases up.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_its.c | 52 ++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 8dade9440ac..4f598d3c14f 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -404,19 +404,29 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt,
num_eventids = 1ULL << (dte.size + 1);
num_intids = 1ULL << (GICD_TYPER_IDBITS + 1);
- if ((icid >= s->ct.num_entries)
- || !dte.valid || (eventid >= num_eventids) ||
- (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)))) {
+ if (icid >= s->ct.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
- "%s: invalid command attributes "
- "icid %d or eventid %d or pIntid %d or"
- "unmapped dte %d\n", __func__, icid, eventid,
- pIntid, dte.valid);
- /*
- * in this implementation, in case of error
- * we ignore this command and move onto the next
- * command in the queue
- */
+ "%s: invalid ICID 0x%x >= 0x%x\n",
+ __func__, icid, s->ct.num_entries);
+ return CMD_CONTINUE;
+ }
+
+ if (!dte.valid) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: no valid DTE for devid 0x%x\n", __func__, devid);
+ return CMD_CONTINUE;
+ }
+
+ if (eventid >= num_eventids) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid event ID 0x%x >= 0x%" PRIx64 "\n",
+ __func__, eventid, num_eventids);
+ return CMD_CONTINUE;
+ }
+
+ if (pIntid < GICV3_LPI_INTID_START || pIntid >= num_intids) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid interrupt ID 0x%x\n", __func__, pIntid);
return CMD_CONTINUE;
}
@@ -529,16 +539,16 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, const uint64_t *cmdpkt)
dte.ittaddr = (cmdpkt[2] & ITTADDR_MASK) >> ITTADDR_SHIFT;
dte.valid = cmdpkt[2] & CMD_FIELD_VALID_MASK;
- if ((devid >= s->dt.num_entries) ||
- (dte.size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
+ if (devid >= s->dt.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
- "ITS MAPD: invalid device table attributes "
- "devid %d or size %d\n", devid, dte.size);
- /*
- * in this implementation, in case of error
- * we ignore this command and move onto the next
- * command in the queue
- */
+ "ITS MAPD: invalid device ID field 0x%x >= 0x%x\n",
+ devid, s->dt.num_entries);
+ return CMD_CONTINUE;
+ }
+
+ if (dte.size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "ITS MAPD: invalid size %d\n", dte.size);
return CMD_CONTINUE;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets
2022-02-01 19:31 ` [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets Peter Maydell
@ 2022-02-03 2:15 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 2:15 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:31, Peter Maydell wrote:
> Currently the ITS accesses each 8-byte doubleword in a 4-doubleword
> command packet with a separate address_space_ldq_le() call. This is
> awkward because the individual command processing functions have
> ended up with code to handle "load more doublewords out of the
> packet", which is both unwieldy and also a potential source of bugs
> because it's not obvious when looking at a line that pulls a field
> out of the 'value' variable which of the 4 doublewords that variable
> currently holds.
>
> Switch to using address_space_map() to map the whole command packet
> at once and fish the four doublewords out of it. Then each process_*
> function can start with a few lines of code that extract the fields
> it cares about.
>
> This requires us to split out the guts of process_its_cmd() into a
> new do_process_its_cmd(), because we were previously overloading the
> value and offset arguments as a backdoor way to directly pass the
> devid and eventid from a write to GITS_TRANSLATER. The new
> do_process_its_cmd() takes those arguments directly, and
> process_its_cmd() is just a wrapper that does the "read fields from
> command packet" part.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/gicv3_internal.h | 4 +-
> hw/intc/arm_gicv3_its.c | 208 +++++++++++----------------------------
> 2 files changed, 62 insertions(+), 150 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 02/13] hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t
2022-02-01 19:31 ` [PATCH 02/13] hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t Peter Maydell
@ 2022-02-03 2:23 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 2:23 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:31, Peter Maydell wrote:
> In the ITS, a DTE is an entry in the device table, which contains
> multiple fields. Currently the function get_dte() which reads one
> entry from the device table returns it as a raw 64-bit integer,
> which we then pass around in that form, only extracting fields
> from it as we need them.
>
> Create a real C struct with the same fields as the DTE, and
> populate it in get_dte(), so that that function and update_dte()
> are the only ones that need to care about the in-guest-memory
> format of the DTE.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> This isn't a massive gain for the DTE, though I do think it
> looks a bit nicer. The real benefit is in aligning all the
> get_{cte,dte,ite} functions to have the same shaped API:
> currently get_ite() is different from all the rest.
> ---
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 03/13] hw/intc/arm_gicv3_its: Pass DTEntry to update_dte()
2022-02-01 19:31 ` [PATCH 03/13] hw/intc/arm_gicv3_its: Pass DTEntry to update_dte() Peter Maydell
@ 2022-02-03 2:30 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 2:30 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:31, Peter Maydell wrote:
> Make update_dte() take a DTEntry struct rather than all the fields of
> the new DTE as separate arguments.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 04/13] hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t
2022-02-01 19:31 ` [PATCH 04/13] hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t Peter Maydell
@ 2022-02-03 2:58 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 2:58 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:31, Peter Maydell wrote:
> In the ITS, a CTE is an entry in the collection table, which contains
> multiple fields. Currently the function get_cte() which reads one
> entry from the device table returns a success/failure boolean and
> passes back the raw 64-bit integer CTE value via a pointer argument.
> We then extract fields from the CTE as we need them.
>
> Create a real C struct with the same fields as the CTE, and
> populate it in get_cte(), so that that function and update_cte()
> are the only ones which need to care about the in-guest-memory
> format of the CTE.
>
> This brings get_cte()'s API into line with get_dte().
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 96 ++++++++++++++++++++++-------------------
> 1 file changed, 52 insertions(+), 44 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 05/13] hw/intc/arm_gicv3_its: Pass CTEntry to update_cte()
2022-02-01 19:31 ` [PATCH 05/13] hw/intc/arm_gicv3_its: Pass CTEntry to update_cte() Peter Maydell
@ 2022-02-03 3:00 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 3:00 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:31, Peter Maydell wrote:
> Make update_cte() take a CTEntry struct rather than all the fields
> of the new CTE as separate arguments.
>
> This brings it into line with the update_dte() API.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 32 +++++++++++++++++---------------
> 1 file changed, 17 insertions(+), 15 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite()
2022-02-01 19:32 ` [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite() Peter Maydell
@ 2022-02-03 3:59 ` Richard Henderson
2022-02-03 10:45 ` Peter Maydell
0 siblings, 1 reply; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 3:59 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> In get_ite() and update_ite() we work with a 12-byte in-guest-memory
> table entry, which we intend to handle as an 8-byte value followed by
> a 4-byte value. Unfortunately the calculation of the address of the
> 4-byte value is wrong, because we write it as:
>
> table_base_address + (index * entrysize) + 4
> (obfuscated by the way the expression has been written)
>
> when it should be + 8. This bug meant that we overwrote the top
> bytes of the 8-byte value with the 4-byte value. There are no
> guest-visible effects because the top half of the 8-byte value
> contains only the doorbell interrupt field, which is used only in
> GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths
> cancel each other out.
>
> We can't simply change the calculation, because this would break
> migration of a (TCG) guest from the old version of QEMU which had
> in-guest-memory interrupt tables written using the buggy version of
> update_ite(). We must also at the same time change the layout of the
> fields within the ITE_L and ITE_H values so that the in-memory
> locations of the fields we care about (VALID, INTTYPE, INTID and
> ICID) stay the same.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/gicv3_internal.h | 19 ++++++++++---------
> hw/intc/arm_gicv3_its.c | 28 +++++++++++-----------------
> 2 files changed, 21 insertions(+), 26 deletions(-)
This is confusing: 5-3 is titled "example of the number of bits that might be stored in an
ITE"? Surely there must be a true architected format for this table, the one real
hardware uses. Surely tcg will simply have to suck it up and break migration to fix this
properly.
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 07/13] hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite()
2022-02-01 19:32 ` [PATCH 07/13] hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite() Peter Maydell
@ 2022-02-03 4:01 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:01 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> The get_ite() code has some awkward nested if statements; clean
> them up by returning early if the memory accesses fail.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 08/13] hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a struct
2022-02-01 19:32 ` [PATCH 08/13] hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a struct Peter Maydell
@ 2022-02-03 4:05 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:05 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> In get_ite() we currently return the caller some of the fields of an
> Interrupt Table Entry via a set of pointer arguments, and validate
> some of them internally (interrupt type and valid bit) to return a
> simple true/false 'valid' indication. Define a new ITEntry struct
> which has all the fields that the in-memory ITE has, and bring the
> get_ite() function in to line with get_dte() and get_cte().
>
> This paves the way for handling virtual interrupts, which will want
> a different subset of the fields in the ITE. Handling them under
> the old "lots of pointer arguments" scheme would have meant a
> confusingly large set of arguments for this function.
>
> The new struct ITEntry is obviously confusably similar to the
> existing IteEntry struct, whose fields are the raw 12 bytes
> of the in-memory ITE. In the next commit we will make update_ite()
> use ITEntry instead of IteEntry, which will allow us to delete
> the IteEntry struct and remove the confusion.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 102 ++++++++++++++++++++++------------------
> 1 file changed, 55 insertions(+), 47 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 09/13] hw/intc/arm_gicv3_its: Make update_ite() use ITEntry
2022-02-01 19:32 ` [PATCH 09/13] hw/intc/arm_gicv3_its: Make update_ite() use ITEntry Peter Maydell
@ 2022-02-03 4:09 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:09 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> Make the update_ite() struct use the new ITEntry struct, so that
> callers don't need to assemble the in-memory ITE data themselves, and
> only get_ite() and update_ite() need to care about that in-memory
> layout. We can then drop the no-longer-used IteEntry struct
> definition.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 62 +++++++++++++++++++++--------------------
> 1 file changed, 32 insertions(+), 30 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 10/13] hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields
2022-02-01 19:32 ` [PATCH 10/13] hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields Peter Maydell
@ 2022-02-03 4:18 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:18 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> Currently we track in the TableDesc and CmdQDesc structs the state of
> the GITS_BASER<n> and GITS_CBASER Valid bits. However we aren't very
> consistent abut checking the valid field: we test it in update_cte()
> and update_dte(), but not anywhere else we look things up in tables.
>
> The GIC specification says that it is UNPREDICTABLE if a guest fails
> to set any of these Valid bits before enabling the ITS via
> GITS_CTLR.Enabled. So we can choose to handle Valid == 0 as
> equivalent to a zero-length table. This is in fact how we're already
> catching this case in most of the table-access paths: when Valid is 0
> we leave the num_entries fields in TableDesc or CmdQDesc set to zero,
> and then the out-of-bounds check "index >= num_entries" that we have
> to do anyway before doing any of these table lookups will always be
> true, catching the no-valid-table case without any extra code.
>
> So we can remove the checks on the valid field from update_cte()
> and update_dte(): since these happen after the bounds check there
> was never any case when the test could fail. That means the valid
> fields would be entirely unused, so just remove them.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> include/hw/intc/arm_gicv3_its_common.h | 2 --
> hw/intc/arm_gicv3_its.c | 31 ++++++++++++--------------
> 2 files changed, 14 insertions(+), 19 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 11/13] hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field
2022-02-01 19:32 ` [PATCH 11/13] hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field Peter Maydell
@ 2022-02-03 4:24 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:24 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> In the MAPC command, if V=0 this is a request to delete a collection
> table entry and the rdbase field of the command packet will not be
> used. In particular, the specification says that the "UNPREDICTABLE
> if rdbase is not valid" only applies for V=1.
>
> We were doing a check-and-log-guest-error on rdbase regardless of
> whether the V bit was set, and also (harmlessly but confusingly)
> storing the contents of the rdbase field into the updated collection
> table entry. Update the code so that if V=0 we don't check or use
> the rdbase field value.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 12/13] hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI
2022-02-01 19:32 ` [PATCH 12/13] hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI Peter Maydell
@ 2022-02-03 4:25 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:25 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> When handling MAPI/MAPTI, we allow the supplied interrupt ID to be
> either 1023 or something in the valid LPI range. This is a mistake:
> only a real valid LPI is allowed. (The general behaviour of the ITS
> is that most interrupt ID fields require a value in the LPI range;
> the exception is that fields specifying a doorbell value, which are
> all in GICv4 commands, allow also 1023 to mean "no doorbell".)
> Remove the condition that incorrectly allows 1023 here.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> This one's my fault -- Shashi's original code did the right thing,
> IIRC. The spec text and pseudocode disagree here, and in code review
> I backed the wrong horse. Sorry.
> ---
> hw/intc/arm_gicv3_its.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 13/13] hw/intc/arm_gicv3_its: Split error checks
2022-02-01 19:32 ` [PATCH 13/13] hw/intc/arm_gicv3_its: Split error checks Peter Maydell
@ 2022-02-03 4:26 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 4:26 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Shashi Mallela, Alex Bennée
On 2/2/22 06:32, Peter Maydell wrote:
> In most of the ITS command processing, we check different error
> possibilities one at a time and log them appropriately. In
> process_mapti() and process_mapd() we have code which checks
> multiple error cases at once, which means the logging is less
> specific than it could be. Split those cases up.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 52 ++++++++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 21 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite()
2022-02-03 3:59 ` Richard Henderson
@ 2022-02-03 10:45 ` Peter Maydell
2022-02-03 22:02 ` Richard Henderson
0 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2022-02-03 10:45 UTC (permalink / raw)
To: Richard Henderson; +Cc: Shashi Mallela, qemu-arm, Alex Bennée, qemu-devel
On Thu, 3 Feb 2022 at 03:59, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/2/22 06:32, Peter Maydell wrote:
> > In get_ite() and update_ite() we work with a 12-byte in-guest-memory
> > table entry, which we intend to handle as an 8-byte value followed by
> > a 4-byte value. Unfortunately the calculation of the address of the
> > 4-byte value is wrong, because we write it as:
> >
> > table_base_address + (index * entrysize) + 4
> > (obfuscated by the way the expression has been written)
> >
> > when it should be + 8. This bug meant that we overwrote the top
> > bytes of the 8-byte value with the 4-byte value. There are no
> > guest-visible effects because the top half of the 8-byte value
> > contains only the doorbell interrupt field, which is used only in
> > GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths
> > cancel each other out.
> >
> > We can't simply change the calculation, because this would break
> > migration of a (TCG) guest from the old version of QEMU which had
> > in-guest-memory interrupt tables written using the buggy version of
> > update_ite(). We must also at the same time change the layout of the
> > fields within the ITE_L and ITE_H values so that the in-memory
> > locations of the fields we care about (VALID, INTTYPE, INTID and
> > ICID) stay the same.
> This is confusing: 5-3 is titled "example of the number of bits that might be stored in an
> ITE"? Surely there must be a true architected format for this table, the one real
> hardware uses. Surely tcg will simply have to suck it up and break migration to fix this
> properly.
No, the ITE format is implementation-defined, like that of the other
in-guest-memory tables the ITS uses. It's UNPREDICTABLE for a guest
to try to directly access the tables in memory -- they are only ever
written or read by the ITS itself in response to incoming commands,
so it's not a problem for the format in memory to be impdef. This
flexibility in the spec allows implementations to minimize the size
of their data tables based on how large an ID size they support and
other potentially-configurable parameters. For instance if you look
at the values for the GITS_BASER* for the GIC-700 in its TRM you
can see that its Collection Table entry size is only 2 bytes, since
it uses the "rdbase is a CPU number" format; an ITS that used the
"rdbase is a physical address" implementation choice would need
more bytes there. (QEMU also uses "rdbase is a CPU number", but
we have rather profligately opted to use 8 bytes per collection
table entry.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite()
2022-02-03 10:45 ` Peter Maydell
@ 2022-02-03 22:02 ` Richard Henderson
0 siblings, 0 replies; 30+ messages in thread
From: Richard Henderson @ 2022-02-03 22:02 UTC (permalink / raw)
To: Peter Maydell; +Cc: Shashi Mallela, qemu-arm, Alex Bennée, qemu-devel
On 2/3/22 21:45, Peter Maydell wrote:
> On Thu, 3 Feb 2022 at 03:59, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 2/2/22 06:32, Peter Maydell wrote:
>>> In get_ite() and update_ite() we work with a 12-byte in-guest-memory
>>> table entry, which we intend to handle as an 8-byte value followed by
>>> a 4-byte value. Unfortunately the calculation of the address of the
>>> 4-byte value is wrong, because we write it as:
>>>
>>> table_base_address + (index * entrysize) + 4
>>> (obfuscated by the way the expression has been written)
>>>
>>> when it should be + 8. This bug meant that we overwrote the top
>>> bytes of the 8-byte value with the 4-byte value. There are no
>>> guest-visible effects because the top half of the 8-byte value
>>> contains only the doorbell interrupt field, which is used only in
>>> GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths
>>> cancel each other out.
>>>
>>> We can't simply change the calculation, because this would break
>>> migration of a (TCG) guest from the old version of QEMU which had
>>> in-guest-memory interrupt tables written using the buggy version of
>>> update_ite(). We must also at the same time change the layout of the
>>> fields within the ITE_L and ITE_H values so that the in-memory
>>> locations of the fields we care about (VALID, INTTYPE, INTID and
>>> ICID) stay the same.
>
>> This is confusing: 5-3 is titled "example of the number of bits that might be stored in an
>> ITE"? Surely there must be a true architected format for this table, the one real
>> hardware uses. Surely tcg will simply have to suck it up and break migration to fix this
>> properly.
>
> No, the ITE format is implementation-defined, like that of the other
> in-guest-memory tables the ITS uses. It's UNPREDICTABLE for a guest
> to try to directly access the tables in memory -- they are only ever
> written or read by the ITS itself in response to incoming commands,
> so it's not a problem for the format in memory to be impdef. This
> flexibility in the spec allows implementations to minimize the size
> of their data tables based on how large an ID size they support and
> other potentially-configurable parameters. For instance if you look
> at the values for the GITS_BASER* for the GIC-700 in its TRM you
> can see that its Collection Table entry size is only 2 bytes, since
> it uses the "rdbase is a CPU number" format; an ITS that used the
> "rdbase is a physical address" implementation choice would need
> more bytes there. (QEMU also uses "rdbase is a CPU number", but
> we have rather profligately opted to use 8 bytes per collection
> table entry.)
Ah, right. In which case,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
` (12 preceding siblings ...)
2022-02-01 19:32 ` [PATCH 13/13] hw/intc/arm_gicv3_its: Split error checks Peter Maydell
@ 2022-02-07 17:56 ` Peter Maydell
13 siblings, 0 replies; 30+ messages in thread
From: Peter Maydell @ 2022-02-07 17:56 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Shashi Mallela, Alex Bennée
On Tue, 1 Feb 2022 at 19:32, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This is another set of patches to the ITS emulation; mostly
> cleanups, but there are two bug fixes.
Applied to target-arm.next, thanks.
-- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-02-07 18:29 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 19:31 [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
2022-02-01 19:31 ` [PATCH 01/13] hw/intc/arm_gicv3_its: Use address_space_map() to access command queue packets Peter Maydell
2022-02-03 2:15 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 02/13] hw/intc/arm_gicv3_its: Keep DTEs as a struct, not a raw uint64_t Peter Maydell
2022-02-03 2:23 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 03/13] hw/intc/arm_gicv3_its: Pass DTEntry to update_dte() Peter Maydell
2022-02-03 2:30 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 04/13] hw/intc/arm_gicv3_its: Keep CTEs as a struct, not a raw uint64_t Peter Maydell
2022-02-03 2:58 ` Richard Henderson
2022-02-01 19:31 ` [PATCH 05/13] hw/intc/arm_gicv3_its: Pass CTEntry to update_cte() Peter Maydell
2022-02-03 3:00 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite() Peter Maydell
2022-02-03 3:59 ` Richard Henderson
2022-02-03 10:45 ` Peter Maydell
2022-02-03 22:02 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 07/13] hw/intc/arm_gicv3_its: Avoid nested ifs in get_ite() Peter Maydell
2022-02-03 4:01 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 08/13] hw/intc/arm_gicv3_its: Pass ITE values back from get_ite() via a struct Peter Maydell
2022-02-03 4:05 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 09/13] hw/intc/arm_gicv3_its: Make update_ite() use ITEntry Peter Maydell
2022-02-03 4:09 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 10/13] hw/intc/arm_gicv3_its: Drop TableDesc and CmdQDesc valid fields Peter Maydell
2022-02-03 4:18 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 11/13] hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field Peter Maydell
2022-02-03 4:24 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 12/13] hw/intc/arm_gicv3_its: Don't allow intid 1023 in MAPI/MAPTI Peter Maydell
2022-02-03 4:25 ` Richard Henderson
2022-02-01 19:32 ` [PATCH 13/13] hw/intc/arm_gicv3_its: Split error checks Peter Maydell
2022-02-03 4:26 ` Richard Henderson
2022-02-07 17:56 ` [PATCH 00/13] hw/intc/arm_gicv3_its: more cleanups, bugfixes Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.