| Message ID | 20211211191135.1764649-12-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: > The DTE.SIZE field is 5 bits, which means that DTE.SIZE + 1 > might in theory be 32. When calculating 1 << (DTE.SIZE + 1) > use 1ULL to ensure that we don't do this arithmetic at 32 bits > and shift the 1 off the end in this case. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) But then you assign the result to a uint32_t, so the patch is incomplete. r~
On 12/11/21 11:11 AM, Peter Maydell wrote: > > if (dte_valid) { > - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); > + max_eventid = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); Without changing the type of max_eventid, I think it'd be easiest to fix the off-by-one bug by not changing the comparisions, but changing this computation. E.g. max_eventid = (2 << FIELD_EX64(dte, DTE, SIZE)) - 1; so that the value becomes UINT32_MAX for SIZE=31. r~
On Sun, 12 Dec 2021 at 20:43, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 12/11/21 11:11 AM, Peter Maydell wrote: > > > > if (dte_valid) { > > - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); > > + max_eventid = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); > > Without changing the type of max_eventid, I think it'd be easiest to fix the off-by-one > bug by not changing the comparisions, but changing this computation. E.g. > > max_eventid = (2 << FIELD_EX64(dte, DTE, SIZE)) - 1; > > so that the value becomes UINT32_MAX for SIZE=31. I think I'd prefer to use a uint64_t. I think that part of the reason for all these off-by-one errors is a lack of consistency in how we chose to name variables and whether we put in them the max-allowed value or the 2^n value, so the series tries to standardize on "always call it num_thingy and always use the 2^n value". I prefer to keep the consistency rather than rearrange things in this one case so it can use a uint32_t. -- PMM
On Mon, 13 Dec 2021 at 09:48, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 12 Dec 2021 at 20:43, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > On 12/11/21 11:11 AM, Peter Maydell wrote: > > > > > > if (dte_valid) { > > > - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); > > > + max_eventid = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); > > > > Without changing the type of max_eventid, I think it'd be easiest to fix the off-by-one > > bug by not changing the comparisions, but changing this computation. E.g. > > > > max_eventid = (2 << FIELD_EX64(dte, DTE, SIZE)) - 1; > > > > so that the value becomes UINT32_MAX for SIZE=31. > > I think I'd prefer to use a uint64_t. I think that part of the reason > for all these off-by-one errors is a lack of consistency in how we > chose to name variables and whether we put in them the max-allowed > value or the 2^n value, so the series tries to standardize on > "always call it num_thingy and always use the 2^n value". I prefer > to keep the consistency rather than rearrange things in this > one case so it can use a uint32_t. Looking at the series, I'm going to squash this patch into the later "Fix event ID bounds checks" patch, and do all the fixing of the event ID check there in a single patch. -- PMM
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index 7a217b00f89..d6637229479 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -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); + max_eventid = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); @@ -376,7 +376,7 @@ 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); + max_eventid = 1ULL << (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)
The DTE.SIZE field is 5 bits, which means that DTE.SIZE + 1 might in theory be 32. When calculating 1 << (DTE.SIZE + 1) use 1ULL to ensure that we don't do this arithmetic at 32 bits and shift the 1 off the end in this case. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gicv3_its.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)