On Mon, 13 Dec 2021 at 09:48, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Sun, 12 Dec 2021 at 20:43, Richard Henderson
> <richard.hender...@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
