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

[Peter Maydell]

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. Also, this
test is gicv2, which is unrelated to either the gicv3 code
or to the ITS...

>   /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.

>   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.

>   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.

>   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.

>   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.

>   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.

thanks
-- PMM