Re: [PATCH 3/5] target/arm/ptw.c: Add Granule Bypass Windows

[Jim MacArthur <jim.macarthur@linaro.org>]

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