[PATCH v3 33/40] KVM: arm64: gic-v5: Add GICv5 EL1 sysreg userspace accessors

[Sascha Bischoff]

On Tue, 2026-07-21 at 15:32 +0100, Fuad Tabba wrote:
> Hi Sascha,
> 
> I found this one harder to follow than I expected, and I think what
> makes it hard to read is also what makes the two things below go
> wrong. A PPI is numbered two different ways here, by its position
> within the register and by its absolute index, and which of the two a
> given line needs is not obvious from the line itself.
> 
> Two things in set_gic_ppi_enabler(), set_gic_ppi_activer() and
> set_gic_ppi_pendr(), both latent while VGIC_V5_NR_PRIVATE_IRQS is 64.
> 
> The first is the loop bounds. All three walk:
> 
> > + start = VGIC_V5_NR_PRIVATE_IRQS * reg;
> > + end = start + VGIC_V5_NR_PRIVATE_IRQS;
> > + for (i = start; i < end; i++) {
> 
> but each register covers 64 PPIs, so the slice being handled is 64 *
> reg to 64 * reg + 63. In set_gic_ppi_enabler() and
> set_gic_ppi_activer() that is explicit just above the loop, for
> example:
> 
> > + bitmap_write(cpu_if->vgic_ppi_enabler, val, 64 * reg, 64);
> 
> With VGIC_V5_NR_PRIVATE_IRQS at 128 and reg at 1, the loop would run
> from 128 to 255, so the test_bit() inside it would read past the end
> of a bitmap that is VGIC_V5_NR_PRIVATE_IRQS bits wide. The absolute
> indexing there is what the bitmap wants, it is the bounds that do not
> match the 64-wide slice.

Hi Fuad,

I was clearly having a moment here. I suspect that I tried to remove as
many "magic constants" as possible, and had a bit of a brain fart here
and plugged in VGIC_V5_NR_PRIVATE_IRQS which is obviously not the
number of bits in a PPI register. Thanks for spotting it!

While it should be 64, it certainly should never be tied to that
constant! I've not spotted it as I limited PPIs to just the architected
ones, and those those it just so happens to work.

> 
> The second is only in set_gic_ppi_pendr():
> 
> > +            bool level = !!(val & BIT_ULL(i));
> 
> Here i is the absolute PPI number, but val is a u64 whichever
> register
> is being handled, so its bits are always numbered 0 to 63. With reg
> at
> 1 this would be BIT_ULL(64) and up.

Again, thanks for spotting this one. That should have been BIT_ULL(i -
start) to make sure that it is within the required 64-bits (under the
assumption that the above issue is also addressed).

> 
> I appreciate none of this is reachable today. All three start with
> 
> > +    if (VGIC_V5_NR_PRIVATE_IRQS == 64 && reg == 1)
> > +        return 0;
> 
> so reg is always 0 and the two numberings coincide. But the guard
> reads as though the non-64 case is meant to work, and as far as I can
> tell it would not. Would it make sense to derive the bounds from the
> register width rather than from VGIC_V5_NR_PRIVATE_IRQS, so that the
> loop and the data always describe the same slice?

Absolutely. As I said above, I've wrongly optimised this into a bug.
I've made this change, and have hopefully fixed my dumb bugs!

> 
> Cheers,
> /fuad

Thanks a lot,
Sascha