| Message ID | 20211211191135.1764649-11-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm gicv3 ITS: Various bug fixes and refactorings | expand |
On 12/11/21 11:11 AM, Peter Maydell wrote: > 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> > --- > hw/intc/gicv3_internal.h | 7 ++++--- > hw/intc/arm_gicv3_its.c | 20 +++++++++----------- > 2 files changed, 13 insertions(+), 14 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 12/11/21 20:11, Peter Maydell wrote: > 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> > --- > 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 5a63e9ed5ce..6a3b145f377 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -393,9 +393,10 @@ 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) Side note, both ITTADDR_LENGTH & ITTADDR_MASK definitions are now unused.
On Sun, Dec 12, 2021 at 10:16 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 12/11/21 20:11, Peter Maydell wrote: > > 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> > > --- > > 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 5a63e9ed5ce..6a3b145f377 100644 > > --- a/hw/intc/gicv3_internal.h > > +++ b/hw/intc/gicv3_internal.h > > @@ -393,9 +393,10 @@ 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) > > Side note, both ITTADDR_LENGTH & ITTADDR_MASK definitions are now unused. I misread the diff, once applying the series I noticed ITTADDR_MASK is still used in process_mapd().
Peter Maydell <peter.maydell@linaro.org> writes: > 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>
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 5a63e9ed5ce..6a3b145f377 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -393,9 +393,10 @@ 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 6f21c56fba2..7a217b00f89 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -114,7 +114,7 @@ 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) + @@ -141,7 +141,7 @@ 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 + @@ -255,10 +255,10 @@ 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); @@ -375,10 +375,8 @@ 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) @@ -529,9 +527,9 @@ 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;
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> --- hw/intc/gicv3_internal.h | 7 ++++--- hw/intc/arm_gicv3_its.c | 20 +++++++++----------- 2 files changed, 13 insertions(+), 14 deletions(-)