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

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.

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.

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?

Cheers,
/fuad