Merge tag 'linux-user-for-7.0-pull-request' of https://gitlab.com/laurent_vivier/qemu into staging (2022-01-06 11:22:42 -0800)

hw/intc/gicv3_internal.h | 40 +++---

include/hw/arm/aspeed_soc.h | 2 +

include/hw/intc/arm_gicv3_its_common.h | 9 +-

hw/arm/aspeed_ast2600.c | 8 ++

hw/arm/npcm7xx_boards.c | 27 ++++

hw/intc/arm_gicv3_its.c | 234 +++++++++++++++------------------

target/arm/helper.c | 32 +++++

7 files changed, 197 insertions(+), 155 deletions(-)

The checks in the ITS on the rdbase values in guest commands are

off-by-one: they permit the guest to pass us a value equal to

s->gicv3->num_cpu, but the valid values are 0...num_cpu-1. This

meant the guest could cause us to index off the end of the

s->gicv3->cpu[] array when calling gicv3_redist_process_lpi(), and we

would probably crash.

(This is not a security bug, because this code is only usable

with emulation, not with KVM.)

Cc: qemu-stable@nongnu.org

Fixes: 17fb5e36aabd4b ("hw/intc: GICv3 redistributor ITS processing")

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

hw/intc/arm_gicv3_its.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,

*/

rdbase = (cte & GITS_CTE_RDBASE_PROCNUM_MASK) >> 1U;

- if (rdbase > s->gicv3->num_cpu) {

+ if (rdbase >= s->gicv3->num_cpu) {

return result;

}

@@ -XXX,XX +XXX,XX @@ static bool process_mapc(GICv3ITSState *s, uint32_t offset)

valid = (value & CMD_FIELD_VALID_MASK);

- if ((icid > s->ct.maxids.max_collids) || (rdbase > s->gicv3->num_cpu)) {

+ if ((icid > s->ct.maxids.max_collids) || (rdbase >= s->gicv3->num_cpu)) {

qemu_log_mask(LOG_GUEST_ERROR,

"ITS MAPC: invalid collection table attributes "

"icid %d rdbase %" PRIu64 "\n", icid, rdbase);

--

2.25.1

We currently define a bitmask for the GITS_CTLR ENABLED bit in

two ways: as ITS_CTLR_ENABLED, and via the FIELD() macro as

R_GITS_CTLR_ENABLED_MASK. Consistently use the FIELD macro version

everywhere and remove the redundant ITS_CTLR_ENABLED define.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---

hw/intc/gicv3_internal.h | 2 --

hw/intc/arm_gicv3_its.c | 20 ++++++++++----------

2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/gicv3_internal.h

+++ b/hw/intc/gicv3_internal.h

@@ -XXX,XX +XXX,XX @@ FIELD(GITS_TYPER, CIL, 36, 1)

#define GITS_IDREGS 0xFFD0

-#define ITS_CTLR_ENABLED (1U) /* ITS Enabled */

-

#define GITS_BASER_RO_MASK (R_GITS_BASER_ENTRYSIZE_MASK | \

R_GITS_BASER_TYPE_MASK)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static void process_cmdq(GICv3ITSState *s)

uint8_t cmd;

int i;

- if (!(s->ctlr & ITS_CTLR_ENABLED)) {

+ if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {

return;

}

@@ -XXX,XX +XXX,XX @@ static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,

switch (offset) {

case GITS_TRANSLATER:

- if (s->ctlr & ITS_CTLR_ENABLED) {

+ if (s->ctlr & R_GITS_CTLR_ENABLED_MASK) {

devid = attrs.requester_id;

result = process_its_cmd(s, data, devid, NONE);

}

@@ -XXX,XX +XXX,XX @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,

switch (offset) {

case GITS_CTLR:

if (value & R_GITS_CTLR_ENABLED_MASK) {

- s->ctlr |= ITS_CTLR_ENABLED;

+ s->ctlr |= R_GITS_CTLR_ENABLED_MASK;

extract_table_params(s);

extract_cmdq_params(s);

s->creadr = 0;

process_cmdq(s);

} else {

- s->ctlr &= ~ITS_CTLR_ENABLED;

+ s->ctlr &= ~R_GITS_CTLR_ENABLED_MASK;

}

break;

case GITS_CBASER:

@@ -XXX,XX +XXX,XX @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,

* IMPDEF choice:- GITS_CBASER register becomes RO if ITS is

* already enabled

*/

- if (!(s->ctlr & ITS_CTLR_ENABLED)) {

+ if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {

s->cbaser = deposit64(s->cbaser, 0, 32, value);

s->creadr = 0;

s->cwriter = s->creadr;

@@ -XXX,XX +XXX,XX @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,

* IMPDEF choice:- GITS_CBASER register becomes RO if ITS is

* already enabled

*/

- if (!(s->ctlr & ITS_CTLR_ENABLED)) {

+ if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {

s->cbaser = deposit64(s->cbaser, 32, 32, value);

s->creadr = 0;

s->cwriter = s->creadr;

@@ -XXX,XX +XXX,XX @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,

* IMPDEF choice:- GITS_BASERn register becomes RO if ITS is

* already enabled

*/

- if (!(s->ctlr & ITS_CTLR_ENABLED)) {

+ if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {

index = (offset - GITS_BASER) / 8;

if (offset & 7) {

@@ -XXX,XX +XXX,XX @@ static bool its_writell(GICv3ITSState *s, hwaddr offset,

* IMPDEF choice:- GITS_BASERn register becomes RO if ITS is

* already enabled

*/

- if (!(s->ctlr & ITS_CTLR_ENABLED)) {

+ if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {

index = (offset - GITS_BASER) / 8;

s->baser[index] &= GITS_BASER_RO_MASK;

s->baser[index] |= (value & ~GITS_BASER_RO_MASK);

@@ -XXX,XX +XXX,XX @@ static bool its_writell(GICv3ITSState *s, hwaddr offset,

* IMPDEF choice:- GITS_CBASER register becomes RO if ITS is

* already enabled

*/

- if (!(s->ctlr & ITS_CTLR_ENABLED)) {

+ if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {

s->cbaser = value;

s->creadr = 0;

s->cwriter = s->creadr;

@@ -XXX,XX +XXX,XX @@ static void gicv3_its_reset(DeviceState *dev)

static void gicv3_its_post_load(GICv3ITSState *s)

{

- if (s->ctlr & ITS_CTLR_ENABLED) {

+ if (s->ctlr & R_GITS_CTLR_ENABLED_MASK) {

extract_table_params(s);

extract_cmdq_params(s);

}

--

2.25.1

The TableDesc struct defines properties of the in-guest-memory tables

which the guest tells us about by writing to the GITS_BASER<n>

registers. This struct currently has a union 'maxids', but all the

fields of the union have the same type (uint32_t) and do the same

thing (record one-greater-than the maximum ID value that can be used

as an index into the table).

We're about to add another table type (the GICv4 vPE table); rather

than adding another specifically-named union field for that table

type with the same type as the other union fields, remove the union

entirely and just have a 'uint32_t max_ids' struct field.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

include/hw/intc/arm_gicv3_its_common.h | 5 +----

hw/intc/arm_gicv3_its.c | 20 ++++++++++----------

2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h

index XXXXXXX..XXXXXXX 100644

--- a/include/hw/intc/arm_gicv3_its_common.h

+++ b/include/hw/intc/arm_gicv3_its_common.h

@@ -XXX,XX +XXX,XX @@ typedef struct {

uint16_t entry_sz;

uint32_t page_sz;

uint32_t max_entries;

- union {

- uint32_t max_devids;

- uint32_t max_collids;

- } maxids;

+ uint32_t max_ids;

uint64_t base_addr;

} TableDesc;

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,

* In this implementation, in case of guest errors we ignore the

* command and move onto the next command in the queue.

*/

- if (devid > s->dt.maxids.max_devids) {

+ if (devid > s->dt.max_ids) {

qemu_log_mask(LOG_GUEST_ERROR,

"%s: invalid command attributes: devid %d>%d",

- __func__, devid, s->dt.maxids.max_devids);

+ __func__, devid, s->dt.max_ids);

} else if (!dte_valid || !ite_valid || !cte_valid) {

qemu_log_mask(LOG_GUEST_ERROR,

@@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,

max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;

}

- if ((devid > s->dt.maxids.max_devids) || (icid > s->ct.maxids.max_collids)

+ if ((devid > s->dt.max_ids) || (icid > s->ct.max_ids)

|| !dte_valid || (eventid > max_eventid) ||

(!ignore_pInt && (((pIntid < GICV3_LPI_INTID_START) ||

(pIntid > max_Intid)) && (pIntid != INTID_SPURIOUS)))) {

@@ -XXX,XX +XXX,XX @@ static bool process_mapc(GICv3ITSState *s, uint32_t offset)

valid = (value & CMD_FIELD_VALID_MASK);

- if ((icid > s->ct.maxids.max_collids) || (rdbase >= s->gicv3->num_cpu)) {

+ if ((icid > s->ct.max_ids) || (rdbase >= s->gicv3->num_cpu)) {

qemu_log_mask(LOG_GUEST_ERROR,

"ITS MAPC: invalid collection table attributes "

"icid %d rdbase %" PRIu64 "\n", icid, rdbase);

@@ -XXX,XX +XXX,XX @@ static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset)

valid = (value & CMD_FIELD_VALID_MASK);

- if ((devid > s->dt.maxids.max_devids) ||

+ if ((devid > s->dt.max_ids) ||

(size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {

qemu_log_mask(LOG_GUEST_ERROR,

"ITS MAPD: invalid device table attributes "

@@ -XXX,XX +XXX,XX @@ static void extract_table_params(GICv3ITSState *s)

(page_sz / s->dt.entry_sz));

}

- s->dt.maxids.max_devids = (1UL << (FIELD_EX64(s->typer, GITS_TYPER,

- DEVBITS) + 1));

+ s->dt.max_ids = (1UL << (FIELD_EX64(s->typer, GITS_TYPER,

+ DEVBITS) + 1));

s->dt.base_addr = baser_base_addr(value, page_sz);

@@ -XXX,XX +XXX,XX @@ static void extract_table_params(GICv3ITSState *s)

}

if (FIELD_EX64(s->typer, GITS_TYPER, CIL)) {

- s->ct.maxids.max_collids = (1UL << (FIELD_EX64(s->typer,

- GITS_TYPER, CIDBITS) + 1));

+ s->ct.max_ids = (1UL << (FIELD_EX64(s->typer,

+ GITS_TYPER, CIDBITS) + 1));

} else {

/* 16-bit CollectionId supported when CIL == 0 */

- s->ct.maxids.max_collids = (1UL << 16);

+ s->ct.max_ids = (1UL << 16);

}

s->ct.base_addr = baser_base_addr(value, page_sz);

--

2.25.1

In extract_table_params() we process each GITS_BASER<n> register. If

the register's Valid bit is not set, this means there is no

in-guest-memory table and so we should not try to interpret the other

fields in the register. This was incorrectly coded as a 'return'

rather than a 'break', so instead of looping round to process the

next GITS_BASER<n> we would stop entirely, treating any later tables

as being not valid also.

This has no real guest-visible effects because (since we don't have

GITS_TYPER.HCC != 0) the guest must in any case set up all the

GITS_BASER<n> to point to valid tables, so this only happens in an

odd misbehaving-guest corner case.

Fix the check to 'break', so that we leave the case statement and

loop back around to the next GITS_BASER<n>.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

hw/intc/arm_gicv3_its.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static void extract_table_params(GICv3ITSState *s)

s->dt.valid = FIELD_EX64(value, GITS_BASER, VALID);

if (!s->dt.valid) {

- return;

+ break;

}

s->dt.page_sz = page_sz;

@@ -XXX,XX +XXX,XX @@ static void extract_table_params(GICv3ITSState *s)

* hence writes are discarded if ct.valid is 0

*/

if (!s->ct.valid) {

- return;

+ break;

}

s->ct.page_sz = page_sz;

--

2.25.1

We set the TableDesc entry_sz field from the appropriate

GITS_BASER.ENTRYSIZE field. That ID register field specifies the

number of bytes per table entry minus one. However when we use

td->entry_sz we assume it to be the number of bytes per table entry

(for instance we calculate the number of entries in a page by

dividing the page size by the entry size).

The effects of this bug are:

* we miscalculate the maximum number of entries in the table,

so our checks on guest index values are wrong (too lax)

* when looking up an entry in the second level of an indirect

table, we calculate an incorrect index into the L2 table.

Because we make the same incorrect calculation on both

reads and writes of the L2 table, the guest won't notice

unless it's unlucky enough to use an index value that

causes us to index off the end of the L2 table page and

cause guest memory corruption in whatever follows

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

hw/intc/arm_gicv3_its.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static void extract_table_params(GICv3ITSState *s)

}

td->page_sz = page_sz;

td->indirect = FIELD_EX64(value, GITS_BASER, INDIRECT);

- td->entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE);

+ td->entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE) + 1;

td->base_addr = baser_base_addr(value, page_sz);

if (!td->indirect) {

td->max_entries = (num_pages * page_sz) / td->entry_sz;

--

2.25.1

The GITS_TYPE_PHYSICAL define is the value we set the

GITS_TYPER.Physical field to -- this is 1 to indicate that we support

physical LPIs. (Support for virtual LPIs is the GITS_TYPER.Virtual

field.) We also use this define as the *value* that we write into an

interrupt translation table entry's INTTYPE field, which should be 1

for a physical interrupt and 0 for a virtual interrupt. Finally, we

use it as a *mask* when we read the interrupt translation table entry

INTTYPE field.

Untangle this confusion: define an ITE_INTTYPE_VIRTUAL and

ITE_INTTYPE_PHYSICAL to be the valid values of the ITE INTTYPE

field, and replace the ad-hoc collection of ITE_ENTRY_* defines with

use of the FIELD() macro to define the fields of an ITE and the

FIELD_EX64() and FIELD_DP64() macros to read and write them.

We use ITE in the new setup, rather than ITE_ENTRY, because

ITE stands for "Interrupt translation entry" and so the extra

"entry" would be redundant.

We take the opportunity to correct the name of the field that holds

the GICv4 'doorbell' interrupt ID (this is always the value 1023 in a

GICv3, which is why we were calling it the 'spurious' field).

The GITS_TYPE_PHYSICAL define is then used in only one place, where

we set the initial GITS_TYPER value. Since GITS_TYPER.Physical is

essentially a boolean, hiding the '1' value behind a macro is more

confusing than helpful, so expand out the macro there and remove the

define entirely.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

hw/intc/gicv3_internal.h | 26 ++++++++++++++------------

hw/intc/arm_gicv3_its.c | 30 +++++++++++++-----------------

2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/gicv3_internal.h

+++ b/hw/intc/gicv3_internal.h

@@ -XXX,XX +XXX,XX @@ FIELD(MAPC, RDBASE, 16, 32)

#define L2_TABLE_VALID_MASK CMD_FIELD_VALID_MASK

#define TABLE_ENTRY_VALID_MASK (1ULL << 0)

-/**

- * Default features advertised by this version of ITS

- */

-/* Physical LPIs supported */

-#define GITS_TYPE_PHYSICAL (1U << 0)

-

/*

* 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: | 1023 | IntNum | IntType | Valid |

+ * Values: | Doorbell | 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)

*/

#define ITS_ITT_ENTRY_SIZE 0xC

-#define ITE_ENTRY_INTTYPE_SHIFT 1

-#define ITE_ENTRY_INTID_SHIFT 2

-#define ITE_ENTRY_INTID_MASK MAKE_64BIT_MASK(2, 24)

-#define ITE_ENTRY_INTSP_SHIFT 26

-#define ITE_ENTRY_ICID_MASK MAKE_64BIT_MASK(0, 16)

+

+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)

+

+/* Possible values for ITE_L INTTYPE */

+#define ITE_INTTYPE_VIRTUAL 0

+#define ITE_INTTYPE_PHYSICAL 1

/* 16 bits EventId */

#define ITS_IDBITS GICD_TYPER_IDBITS

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,

MEMTXATTRS_UNSPECIFIED, res);

if (*res == MEMTX_OK) {

- if (ite.itel & TABLE_ENTRY_VALID_MASK) {

- if ((ite.itel >> ITE_ENTRY_INTTYPE_SHIFT) &

- GITS_TYPE_PHYSICAL) {

- *pIntid = (ite.itel & ITE_ENTRY_INTID_MASK) >>

- ITE_ENTRY_INTID_SHIFT;

- *icid = ite.iteh & ITE_ENTRY_ICID_MASK;

+ 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_EX32(ite.iteh, ITE_H, ICID);

status = true;

}

}

@@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,

MemTxResult res = MEMTX_OK;

uint16_t icid = 0;

uint64_t dte = 0;

- IteEntry ite;

- uint32_t int_spurious = INTID_SPURIOUS;

bool result = false;

devid = ((value & DEVID_MASK) >> DEVID_SHIFT);

@@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,

*/

} else {

/* add ite entry to interrupt translation table */

- ite.itel = (dte_valid & TABLE_ENTRY_VALID_MASK) |

- (GITS_TYPE_PHYSICAL << ITE_ENTRY_INTTYPE_SHIFT);

-

+ IteEntry ite = {};

+ ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid);

+ ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);

if (ignore_pInt) {

- ite.itel |= (eventid << ITE_ENTRY_INTID_SHIFT);

+ ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, eventid);

} else {

- ite.itel |= (pIntid << ITE_ENTRY_INTID_SHIFT);

+ ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid);

}

- ite.itel |= (int_spurious << ITE_ENTRY_INTSP_SHIFT);

- ite.iteh = icid;

+ ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);

+ ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);

result = update_ite(s, eventid, dte, ite);

}

@@ -XXX,XX +XXX,XX @@ static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)

"gicv3-its-sysmem");

/* set the ITS default features supported */

- s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,

- GITS_TYPE_PHYSICAL);

+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL, 1);

s->typer = FIELD_DP64(s->typer, GITS_TYPER, ITT_ENTRY_SIZE,

ITS_ITT_ENTRY_SIZE - 1);

s->typer = FIELD_DP64(s->typer, GITS_TYPER, IDBITS, ITS_IDBITS);

--

2.25.1

Currently the ITS code that reads and writes DTEs uses open-coded

shift-and-mask to assemble the various fields into the 64-bit DTE

word. The names of the macros used for mask and shift values are

also somewhat inconsistent, and don't follow our usual convention

that a MASK macro should specify the bits in their place in the word.

Replace all these with use of the FIELD macro.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

hw/intc/gicv3_internal.h | 7 ++++---

hw/intc/arm_gicv3_its.c | 20 +++++++++-----------

2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/gicv3_internal.h

+++ b/hw/intc/gicv3_internal.h

@@ -XXX,XX +XXX,XX @@ FIELD(ITE_H, VPEID, 16, 16)

* Valid = 1 bit,ITTAddr = 44 bits,Size = 5 bits

*/

#define GITS_DTE_SIZE (0x8ULL)

-#define GITS_DTE_ITTADDR_SHIFT 6

-#define GITS_DTE_ITTADDR_MASK MAKE_64BIT_MASK(GITS_DTE_ITTADDR_SHIFT, \

- ITTADDR_LENGTH)

+

+FIELD(DTE, VALID, 0, 1)

+FIELD(DTE, SIZE, 1, 5)

+FIELD(DTE, ITTADDR, 6, 44)

/*

* 8 bytes Collection Table Entry size

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_its.c

+++ b/hw/intc/arm_gicv3_its.c

@@ -XXX,XX +XXX,XX @@ static bool update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,

uint64_t itt_addr;

MemTxResult res = MEMTX_OK;

- itt_addr = (dte & GITS_DTE_ITTADDR_MASK) >> GITS_DTE_ITTADDR_SHIFT;

+ 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) +

@@ -XXX,XX +XXX,XX @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,

bool status = false;

IteEntry ite = {};

- itt_addr = (dte & GITS_DTE_ITTADDR_MASK) >> GITS_DTE_ITTADDR_SHIFT;

+ itt_addr = FIELD_EX64(dte, DTE, ITTADDR);

itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */

ite.itel = address_space_ldq_le(as, itt_addr +

@@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,

if (res != MEMTX_OK) {

return result;

}

- dte_valid = dte & TABLE_ENTRY_VALID_MASK;

+ dte_valid = FIELD_EX64(dte, DTE, VALID);

if (dte_valid) {

- max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));

+ max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);

ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);

@@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,

if (res != MEMTX_OK) {

return result;

}

- dte_valid = dte & TABLE_ENTRY_VALID_MASK;

-

- max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));

-

+ dte_valid = FIELD_EX64(dte, DTE, VALID);

+ max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);

max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;

if ((devid > s->dt.max_ids) || (icid > s->ct.max_ids)

@@ -XXX,XX +XXX,XX @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,

if (s->dt.valid) {

if (valid) {

/* add mapping entry to device table */

- dte = (valid & TABLE_ENTRY_VALID_MASK) |

- ((size & SIZE_MASK) << 1U) |

- (itt_addr << GITS_DTE_ITTADDR_SHIFT);

+ dte = FIELD_DP64(dte, DTE, VALID, 1);

+ dte = FIELD_DP64(dte, DTE, SIZE, size);

+ dte = FIELD_DP64(dte, DTE, ITTADDR, itt_addr);

}

} else {

return true;

--

2.25.1

The comment says that in our CTE format the RDBase field is 36 bits;

in fact for us it is only 16 bits, because we use the RDBase format

where it specifies a 16-bit CPU number. The code already uses

RDBASE_PROCNUM_LENGTH (16) as the field width, so fix the comment

to match it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

hw/intc/gicv3_internal.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/gicv3_internal.h

+++ b/hw/intc/gicv3_internal.h

@@ -XXX,XX +XXX,XX @@ FIELD(DTE, ITTADDR, 6, 44)

/*

* 8 bytes Collection Table Entry size

- * Valid = 1 bit,RDBase = 36 bits(considering max RDBASE)

+ * Valid = 1 bit, RDBase = 16 bits

*/

#define GITS_CTE_SIZE (0x8ULL)

#define GITS_CTE_RDBASE_PROCNUM_MASK MAKE_64BIT_MASK(1, RDBASE_PROCNUM_LENGTH)

--

2.25.1