| Message ID | 20220111171048.3545974-2-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm gicv3 ITS: Various bug fixes and refactorings | expand |
Peter Maydell <peter.maydell@linaro.org> writes: > In process_its_cmd() and process_mapti() we must check the > event ID against a limit defined by the size field in the DTE, > which specifies the number of ID bits minus one. Convert > this code to our num_foo convention: > * change the variable names > * use uint64_t and 1ULL when calculating the number > of valid event IDs, because DTE.SIZE is 5 bits and > so num_eventids may be up to 2^32 > * fix the off-by-one error in the comparison > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index fa3cdb57554..6d11fa02040 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -225,7 +225,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, > MemTxResult res = MEMTX_OK; > bool dte_valid; > uint64_t dte = 0; > - uint32_t max_eventid; > + uint64_t num_eventids; > uint16_t icid = 0; > uint32_t pIntid = 0; > bool ite_valid = false; > @@ -258,7 +258,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, > dte_valid = FIELD_EX64(dte, DTE, VALID); > > if (dte_valid) { > - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); > + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); > > ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); > > @@ -299,10 +299,11 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, > dte_valid ? "valid" : "invalid", > ite_valid ? "valid" : "invalid", > cte_valid ? "valid" : "invalid"); > - } else if (eventid > max_eventid) { > + } else if (eventid >= num_eventids) { > qemu_log_mask(LOG_GUEST_ERROR, > - "%s: invalid command attributes: eventid %d > %d\n", > - __func__, eventid, max_eventid); > + "%s: invalid command attributes: eventid %d >= %" > + PRId64 "\n", > + __func__, eventid, num_eventids); > } else { > /* > * Current implementation only supports rdbase == procnum > @@ -336,7 +337,8 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, > AddressSpace *as = &s->gicv3->dma_as; > uint32_t devid, eventid; > uint32_t pIntid = 0; > - uint32_t max_eventid, max_Intid; > + uint64_t num_eventids; > + uint32_t max_Intid; > bool dte_valid; > MemTxResult res = MEMTX_OK; > uint16_t icid = 0; > @@ -376,11 +378,11 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, > return result; > } > dte_valid = FIELD_EX64(dte, DTE, VALID); > - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); > + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); > max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1; > > if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids) > - || !dte_valid || (eventid > max_eventid) || > + || !dte_valid || (eventid >= num_eventids) || > (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) && > (pIntid != INTID_SPURIOUS))) { It seems process_mapti has the similar "catch all the errors" if leg that I split apart in process_its_command to make it easier to see what failed. However: Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On 1/12/22 04:10, Peter Maydell wrote: > In process_its_cmd() and process_mapti() we must check the > event ID against a limit defined by the size field in the DTE, > which specifies the number of ID bits minus one. Convert > this code to our num_foo convention: > * change the variable names > * use uint64_t and 1ULL when calculating the number > of valid event IDs, because DTE.SIZE is 5 bits and > so num_eventids may be up to 2^32 > * fix the off-by-one error in the comparison > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); Could be written 2 << N, instead of 1 << (N + 1). r~
On Fri, 28 Jan 2022 at 01:33, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 1/12/22 04:10, Peter Maydell wrote: > > In process_its_cmd() and process_mapti() we must check the > > event ID against a limit defined by the size field in the DTE, > > which specifies the number of ID bits minus one. Convert > > this code to our num_foo convention: > > * change the variable names > > * use uint64_t and 1ULL when calculating the number > > of valid event IDs, because DTE.SIZE is 5 bits and > > so num_eventids may be up to 2^32 > > * fix the off-by-one error in the comparison > > > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > > --- > > hw/intc/arm_gicv3_its.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > > > + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); > > Could be written 2 << N, instead of 1 << (N + 1). It could, but the spec defines the field as containing "number of supported bits, minus 1", so I think that using an expression that matches that is clearer. Aside: clang optimizes both of these expressions to the same thing; gcc does not: https://godbolt.org/z/nsz4Mdxhq (not that it will make a perf difference we care about here). -- PMM
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index fa3cdb57554..6d11fa02040 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -225,7 +225,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, MemTxResult res = MEMTX_OK; bool dte_valid; uint64_t dte = 0; - uint32_t max_eventid; + uint64_t num_eventids; uint16_t icid = 0; uint32_t pIntid = 0; bool ite_valid = false; @@ -258,7 +258,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, dte_valid = FIELD_EX64(dte, DTE, VALID); if (dte_valid) { - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); @@ -299,10 +299,11 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, dte_valid ? "valid" : "invalid", ite_valid ? "valid" : "invalid", cte_valid ? "valid" : "invalid"); - } else if (eventid > max_eventid) { + } else if (eventid >= num_eventids) { qemu_log_mask(LOG_GUEST_ERROR, - "%s: invalid command attributes: eventid %d > %d\n", - __func__, eventid, max_eventid); + "%s: invalid command attributes: eventid %d >= %" + PRId64 "\n", + __func__, eventid, num_eventids); } else { /* * Current implementation only supports rdbase == procnum @@ -336,7 +337,8 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, AddressSpace *as = &s->gicv3->dma_as; uint32_t devid, eventid; uint32_t pIntid = 0; - uint32_t max_eventid, max_Intid; + uint64_t num_eventids; + uint32_t max_Intid; bool dte_valid; MemTxResult res = MEMTX_OK; uint16_t icid = 0; @@ -376,11 +378,11 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, return result; } dte_valid = FIELD_EX64(dte, DTE, VALID); - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1; if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids) - || !dte_valid || (eventid > max_eventid) || + || !dte_valid || (eventid >= num_eventids) || (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) && (pIntid != INTID_SPURIOUS))) { qemu_log_mask(LOG_GUEST_ERROR,
In process_its_cmd() and process_mapti() we must check the event ID against a limit defined by the size field in the DTE, which specifies the number of ID bits minus one. Convert this code to our num_foo convention: * change the variable names * use uint64_t and 1ULL when calculating the number of valid event IDs, because DTE.SIZE is 5 bits and so num_eventids may be up to 2^32 * fix the off-by-one error in the comparison Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gicv3_its.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)