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.
Changes from v1:
* first half of the series is now upstream
* patch 1 now has the '1ULL' and uint64_t fixes that were
partly split across two patches in the old series and partly missing
* new patches 12 and 13
NB: I left the returns of -1 in patch 11.
Patches still needing review: 1, 12, 13
thanks
-- PMM
Peter Maydell (13):
hw/intc/arm_gicv3_its: Fix event ID bounds checks
hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention
hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value
hw/intc/arm_gicv3_its: Don't use data if reading command failed
hw/intc/arm_gicv3_its: Use enum for return value of process_*
functions
hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd()
hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting
hw/intc/arm_gicv3_its: Fix return codes in process_mapti()
hw/intc/arm_gicv3_its: Fix return codes in process_mapc()
hw/intc/arm_gicv3_its: Fix return codes in process_mapd()
hw/intc/arm_gicv3_its: Factor out "find address of table entry" code
hw/intc/arm_gicv3_its: Check indexes before use, not after
hw/intc/arm_gicv3_its: Range-check ICID before indexing into
collection table
hw/intc/arm_gicv3_its.c | 492 ++++++++++++++++++----------------------
1 file changed, 225 insertions(+), 267 deletions(-)
--
2.25.1
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.: /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 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 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 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 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 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 > > Changes from v1: > * first half of the series is now upstream > * patch 1 now has the '1ULL' and uint64_t fixes that were > partly split across two patches in the old series and partly missing > * new patches 12 and 13 > > NB: I left the returns of -1 in patch 11. > > Patches still needing review: 1, 12, 13 > > thanks > -- PMM > > Peter Maydell (13): > hw/intc/arm_gicv3_its: Fix event ID bounds checks > hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention > hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value > hw/intc/arm_gicv3_its: Don't use data if reading command failed > hw/intc/arm_gicv3_its: Use enum for return value of process_* > functions > hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd() > hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting > hw/intc/arm_gicv3_its: Fix return codes in process_mapti() > hw/intc/arm_gicv3_its: Fix return codes in process_mapc() > hw/intc/arm_gicv3_its: Fix return codes in process_mapd() > hw/intc/arm_gicv3_its: Factor out "find address of table entry" code > hw/intc/arm_gicv3_its: Check indexes before use, not after > hw/intc/arm_gicv3_its: Range-check ICID before indexing into > collection table > > hw/intc/arm_gicv3_its.c | 492 ++++++++++++++++++---------------------- > 1 file changed, 225 insertions(+), 267 deletions(-) -- Alex Bennée
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
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
On Tue, 18 Jan 2022 at 23:30, Andre Przywara <andre.przywara@arm.com> wrote: > 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. Thanks for looking at this. I should have read the code rather than dashing off a reply last thing in the evening based just on the test case output! I think I was confusing how our GICv3 emulation handles register accesses (with separate functions for byte/halfword/word/quad accesses) with the GICv2 emulation (which as you say calls down into the byte emulation code wherever it can). > 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). Mmm. I actually have a patch in target-arm.next from Petr Pavlu which implements GICC_IIDR, but we do indeed not implement the distributor equivalent. > 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). Yes. We got this right in the GICv3 emulation design, where almost all the logic is in the 32-bit accessor functions and the 8/16-bit functions deal only with the very few registers that have to permit non-word accesses. The GICv2 code is a lot older (and to be fair to it, started out as 11MPcore interrupt controller emulation, and I bet the docs of that were not very specific about what registers could or could not be accessed byte at a time). Unless we want to rewrite all that logic in the GICv2 emulation (which I at least do not :-)) I think we'll have to live with the warnings about bad-offsets reporting for each byte rather than just once for the word access. -- PMM
On Wed, 19 Jan 2022 10:15:52 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: Hi Peter, > On Tue, 18 Jan 2022 at 23:30, Andre Przywara <andre.przywara@arm.com> wrote: > > 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. > > Thanks for looking at this. I should have read the code rather > than dashing off a reply last thing in the evening based just > on the test case output! I think I was confusing how our GICv3 > emulation handles register accesses (with separate functions for > byte/halfword/word/quad accesses) with the GICv2 emulation > (which as you say calls down into the byte emulation code > wherever it can). No worries! > > 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). > > Mmm. I actually have a patch in target-arm.next from Petr Pavlu > which implements GICC_IIDR, but we do indeed not implement the > distributor equivalent. Well, returning 0 is actually not the worst idea. Using proper ID values might not even be feasible for QEMU, or would create some hassle with versioning. With 0 all a user can assume is spec compliance. > > 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). > > Yes. We got this right in the GICv3 emulation design, where almost > all the logic is in the 32-bit accessor functions and the 8/16-bit > functions deal only with the very few registers that have to > permit non-word accesses. Indeed. I dusted off my old GICv3 MMIO patches for kvm-unit-tests, and QEMU passed with flying colours. With the debug switch I see it reporting exactly the violating accesses we except to see. Will send those patches ASAP. > The GICv2 code is a lot older (and to > be fair to it, started out as 11MPcore interrupt controller > emulation, and I bet the docs of that were not very specific about > what registers could or could not be accessed byte at a time). > Unless we want to rewrite all that logic in the GICv2 emulation > (which I at least do not :-)) ... and I can't ... > I think we'll have to live with > the warnings about bad-offsets reporting for each byte rather > than just once for the word access. Yeah, if those warnings appear only with that debug switch, there is probably little reason to change that code just because of this. At least it seemed to work quite well over the years. Cheers, Andre P.S. I changed k-u-t to special case the UP case, so that TCG passes. But now KVM fails, of course. So I will have to make a patch for the kernel ...
© 2016 - 2024 Red Hat, Inc.