On Thu, May 28, 2026 at 11:52:09AM -0700, Richard Henderson wrote:
> On 5/28/26 08:34, Jim MacArthur wrote:
> > + /*
> > + * GPC Priority 1 (R_GMGRR):
> > + * If GPCCR_EL3.GPCBW is 1 and the configuration GPCBW
> > + * is invalid, the access fails as GPT walk fault at level 0.
> > + */
> > + if (FIELD_EX64(gpccr, GPCCR, GPCBW)) {
> > + /*
> > + * GPCBW is invalid if the base address is:
> > + * not aligned to the size programmed in BWSIZE, or
> > + * greater than or equal to the stride value configured by
> > BWSTRIDE.
> > + */
> > + uint64_t bw_size_mask = -1L << (bw_size_field + 31);
> > + if (bw_start & bw_size_mask) {
> > + goto fault_walk;
> > + }
> > + if (bw_start & bw_stride_mask) {
> > + goto fault_walk;
> > + }
> > + }
> > +
> > switch (FIELD_EX64(gpccr, GPCCR, PGS)) {
>
> ... here.
>
> None of the checks you're adding are correct:
> - Missing size and stride validation,
> - Alignment check vs size is incorrect; you wanted
>
> bw_start & MAKE_64BIT_MASK(0, bw_size + BW_ADDR_SHIFT)
>
> - Check vs stride is incorrect; you wanted bw_stride <= bw_addr.
>
> See GPCRegistersConsistent().
Thanks for the review Richard; the bw_size_mask check was indeed wrong.
On bw_stride_mask: it was intended to keep the high bits in bw_stride_mask
hence this bitwise comparison should have been equivalent to
bw_stride <= bw_addr; in fact the other use of bw_stride_mask was incorrect.
However, since both Arm and Intel GCC produce roughly the same code for both
I will replace it with the more readable arithmetic operation.
On size and stride validation: there are reserved values for these fields,
but the ARM does not say explicitly (that I can see) that they make GPCBW
/invalid/, so it wasn't clear to me that we should fault on these.
Jim
