Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Andre Przywara]

On Tue, 18 Jan 2022 19:41:56 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

Hi Peter, Alex,

thanks for the heads up!

> On Tue, 18 Jan 2022 at 17:42, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >  
> > > I've been working on the ITS to add support for the GICv4 functionality.
> > > In the course of that I found a handful of bugs in it and also some
> > > places where the code benefited from refactoring to make it a better
> > > base to put in the GICv4 parts. This patchset is just the bugfixes
> > > and cleanups, because there are enough patches here that I figured it
> > > made sense to send them out now rather than holding on to them.
> > >
> > > Most of these patches were in v1 and have been reviewed already.  
> >
> > I've reviewed the patches and they look good to me. kvm-unit-tests is
> > still failing some tests but the ones it fails hasn't changed from
> > before this patch:
> >
> >   ✗  env QEMU=$HOME/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 
> > ./run_tests.sh -g gic
> >   PASS gicv2-ipi (3 tests)
> >   PASS gicv2-mmio (17 tests, 1 skipped)
> >   FAIL gicv2-mmio-up (17 tests, 2 unexpected failures)
> >   FAIL gicv2-mmio-3p (17 tests, 3 unexpected failures)
> >   PASS gicv3-ipi (3 tests)
> >   PASS gicv2-active (1 tests)
> >   PASS gicv3-active (1 tests)
> >
> > That said running with -d unimp,guest_errors there are some things that
> > should probably be double checked, e.g.:  
> 
> Almost all of the logging seems to be where the test code is
> doing stuff that the GIC spec says isn't valid.

That sounds like a plausible explanation for a unit test suite, but
does not seem to be actually true in this case, see below.

> Also, this
> test is gicv2, which is unrelated to either the gicv3 code
> or to the ITS...

This is true.

> 
> >   /home/alex/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 -nodefaults 
> > -machine virt -accel tcg -cpu cortex-a57 -device virtio-serial-device 
> > -device virtconsole,chardev=
> >   ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial 
> > stdio -kernel arm/gic.flat -machine gic-version=2 -smp 1 -append "mmio" -d 
> > unimp,guest_errors
> >   PASS: gicv2: mmio: all CPUs have interrupts
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b  
> 
> This is GICD_IIDR, which is a 32-bit register. The test looks like it's
> trying to read it as 4 separate bytes, which is out of spec, and
> is why our implementation is warning about it.

Looking at k-u-t's arm/gic.c and QEMU's hw/intc/arm_gic.c I see two
problems here: QEMU implements word accesses as four successive calls to
gic_dist_readb() - which is probably fine if that helps code design,
but it won't allow it to actually spot access size issues. I just
remember that we spent some brain cells and CPP macros on getting the
access size right in KVM - hence those tests in kvm-unit-tests.
 
But more importantly it looks like GICD_IIDR is actually not
implemented: There is a dubious "if (offset < 0x08) return 0;" line,
but IIDR (offset 0x8) would actually fall through, and hit the bad_reg
label, which would return 0 (and cause the message, if enabled).
Also the name and how it's called suggests that this deals with bytes
only, but returns uint32_t, and for instance deals with bit 10 in
TYPER. I see how this eventually falls into place magically (the upper
three bytes return 0, and get ORed into the >8 bit result of offset 8),
but those messages are definitely false alarm then.

If that helps: from a GIC MMIO perspective 8-bit accesses are actually
the exception rather than the norm (ARM IHI 0048B.b 4.1.4 GIC register
access).

> >   INFO: gicv2: mmio: IIDR: 0x00000000
> >   gic_dist_writeb: Bad offset 4
> >   gic_dist_writeb: Bad offset 5
> >   gic_dist_writeb: Bad offset 6
> >   gic_dist_writeb: Bad offset 7
> >   gic_dist_writeb: Bad offset 4
> >   gic_dist_writeb: Bad offset 5
> >   gic_dist_writeb: Bad offset 6
> >   gic_dist_writeb: Bad offset 7
> >   gic_dist_writeb: Bad offset 4
> >   gic_dist_writeb: Bad offset 5
> >   gic_dist_writeb: Bad offset 6
> >   gic_dist_writeb: Bad offset 7  
> 
> These complaints are because the test is trying to write
> to GICD_TYPER, which is not permitted.

Writes are not permitted, yes, but k-u-t emits a proper str, so there
should be only three lines, not twelve.

> >   PASS: gicv2: mmio: GICD_TYPER is read-only
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b  
> 
> More attempts to do byte accesses to a word-only register.

The messages come actually again because IIDR is not handled at all,
and there are only four of them because of the design of gic_dist_read().
k-u-t issues a proper ldr here.
I think we refrained from actually testing illegal access sizes,
because that could trigger external aborts/SErrors on real hardware.

> >   gic_dist_writeb: Bad offset 8
> >   gic_dist_writeb: Bad offset 9
> >   gic_dist_writeb: Bad offset a
> >   gic_dist_writeb: Bad offset b
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b
> >   gic_dist_writeb: Bad offset 8
> >   gic_dist_writeb: Bad offset 9
> >   gic_dist_writeb: Bad offset a
> >   gic_dist_writeb: Bad offset b
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b
> >   PASS: gicv2: mmio: GICD_IIDR is read-only
> >   gic_dist_writeb: Bad offset fe8
> >   gic_dist_writeb: Bad offset fe9
> >   gic_dist_writeb: Bad offset fea
> >   gic_dist_writeb: Bad offset feb
> >   gic_dist_writeb: Bad offset fe8
> >   gic_dist_writeb: Bad offset fe9
> >   gic_dist_writeb: Bad offset fea
> >   gic_dist_writeb: Bad offset feb
> >   gic_dist_writeb: Bad offset fe8
> >   gic_dist_writeb: Bad offset fe9
> >   gic_dist_writeb: Bad offset fea
> >   gic_dist_writeb: Bad offset feb  
> 
> Writing bytes to a register that is both read-only and also 32-bit only.

Yes, the read-only violation is expected, but the code only does ldr.

> >   PASS: gicv2: mmio: ICPIDR2 is read-only
> >   INFO: gicv2: mmio: value of ICPIDR2: 0x0000002b
> >   PASS: gicv2: mmio: IPRIORITYR: consistent priority masking
> >   INFO: gicv2: mmio: IPRIORITYR: priority mask is 0xffffffff
> >   PASS: gicv2: mmio: IPRIORITYR: implements at least 4 priority bits
> >   INFO: gicv2: mmio: IPRIORITYR: 8 priority bits implemented
> >   PASS: gicv2: mmio: IPRIORITYR: clearing priorities
> >   gic_dist_readb: Bad offset 520
> >   gic_dist_readb: Bad offset 521
> >   gic_dist_readb: Bad offset 522
> >   gic_dist_readb: Bad offset 523
> >   gic_dist_writeb: Bad offset 520
> >   gic_dist_writeb: Bad offset 521
> >   gic_dist_writeb: Bad offset 522
> >   gic_dist_writeb: Bad offset 523
> >   gic_dist_readb: Bad offset 520
> >   gic_dist_readb: Bad offset 521
> >   gic_dist_readb: Bad offset 522
> >   gic_dist_readb: Bad offset 523
> >   gic_dist_writeb: Bad offset 520
> >   gic_dist_writeb: Bad offset 521
> >   gic_dist_writeb: Bad offset 522
> >   gic_dist_writeb: Bad offset 523
> >   gic_dist_readb: Bad offset 520
> >   gic_dist_readb: Bad offset 521
> >   gic_dist_readb: Bad offset 522
> >   gic_dist_readb: Bad offset 523  
> 
> I suspect from what the following test says that this is an
> attempt to write beyond the end of the valid IPRIORITYR registers,
> which isn't permitted.

Trying to manipulate non-implemented SPIs is not really useful (and
indeed typically points to guest bugs), but it is permitted by the
GICv2 spec, which says: "A register field corresponding to an
unimplemented interrupt is RAZ/WI." - which is actually what bad_reg
implements - just minus the message.

> >   PASS: gicv2: mmio: IPRIORITYR: accesses beyond limit RAZ/WI
> >   PASS: gicv2: mmio: IPRIORITYR: accessing last SPIs
> >   PASS: gicv2: mmio: IPRIORITYR: priorities are preserved
> >   PASS: gicv2: mmio: IPRIORITYR: byte reads successful
> >   PASS: gicv2: mmio: IPRIORITYR: byte writes successful
> >   PASS: gicv2: mmio: ITARGETSR: bits for non-existent CPUs masked
> >   INFO: gicv2: mmio: ITARGETSR: 7 non-existent CPUs
> >   PASS: gicv2: mmio: ITARGETSR: accesses beyond limit RAZ/WI
> >   FAIL: gicv2: mmio: ITARGETSR: register content preserved
> >   INFO: gicv2: mmio: ITARGETSR: writing 01010001 reads back as 00000000
> >   PASS: gicv2: mmio: ITARGETSR: byte reads successful
> >   FAIL: gicv2: mmio: ITARGETSR: byte writes successful
> >   INFO: gicv2: mmio: ITARGETSR: writing 0x1f into bytes 2 => 0x00000000
> >   SUMMARY: 17 tests, 2 unexpected failures  
> 
> These ITARGETSR failures are not correct (or you're not running the
> test case the way it's supposed to be). Your command line runs
> only one CPU, and for a uniprocessor GIC the ITARGETRSn registers
> are supposed to be RAZ/WI, whereas the test seems to be expecting
> something else.

Interesting, indeed the *whole* of GICD_ITARGETSRs are RAZ/WI on a
uniprocessor implementation, where is also says that bits for
non-implemented CPU interfaces as RAZ/WI, which would suggest that at
least bit 0 is preserved (what this test checks).
I will double check the spec again on what uniprocessor means
precisely, and then send a kvm-unit-tests patch.

But running with -smp [2..8] still reports issues - but we know of these
for a while, I think, and they are not really critical.

Cheers,
Andre