From: "Alex Bennée" <alex.bennee@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org, qemu-devel@nongnu.org,
Shashi Mallela <shashi.mallela@linaro.org>,
Andrew Jones <drjones@redhat.com>,
Eric Auger <eric.auger@redhat.com>,
Andre.przywara@arm.com
Subject: Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings
Date: Tue, 18 Jan 2022 17:37:23 +0000 [thread overview]
Message-ID: <87pmop7xfw.fsf@linaro.org> (raw)
In-Reply-To: <20220111171048.3545974-1-peter.maydell@linaro.org>
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
WARNING: multiple messages have this Message-ID (diff)
From: "Alex Bennée" <alex.bennee@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Jones <drjones@redhat.com>,
Andre.przywara@arm.com,
Shashi Mallela <shashi.mallela@linaro.org>,
qemu-devel@nongnu.org, Eric Auger <eric.auger@redhat.com>,
qemu-arm@nongnu.org
Subject: Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings
Date: Tue, 18 Jan 2022 17:37:23 +0000 [thread overview]
Message-ID: <87pmop7xfw.fsf@linaro.org> (raw)
In-Reply-To: <20220111171048.3545974-1-peter.maydell@linaro.org>
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
next prev parent reply other threads:[~2022-01-18 17:42 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-11 17:10 [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-18 17:13 ` Alex Bennée
2022-01-18 17:13 ` Alex Bennée
2022-01-28 1:33 ` Richard Henderson
2022-01-28 10:50 ` Peter Maydell
2022-01-28 10:50 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 02/13] hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-28 1:35 ` Richard Henderson
2022-01-28 10:51 ` Peter Maydell
2022-01-28 10:51 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 03/13] hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 04/13] hw/intc/arm_gicv3_its: Don't use data if reading command failed Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 05/13] hw/intc/arm_gicv3_its: Use enum for return value of process_* functions Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 06/13] hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd() Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 07/13] hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 08/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapti() Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 09/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapc() Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 10/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapd() Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-11 17:10 ` [PATCH v2 11/13] hw/intc/arm_gicv3_its: Factor out "find address of table entry" code Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-18 17:31 ` Alex Bennée
2022-01-18 17:31 ` Alex Bennée
2022-01-11 17:10 ` [PATCH v2 12/13] hw/intc/arm_gicv3_its: Check indexes before use, not after Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-18 17:32 ` Alex Bennée
2022-01-18 17:32 ` Alex Bennée
2022-01-28 1:42 ` Richard Henderson
2022-01-28 1:42 ` Richard Henderson
2022-01-11 17:10 ` [PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table Peter Maydell
2022-01-11 17:10 ` Peter Maydell
2022-01-18 17:37 ` Alex Bennée
2022-01-18 17:37 ` Alex Bennée
2022-01-28 1:44 ` Richard Henderson
2022-01-18 17:37 ` Alex Bennée [this message]
2022-01-18 17:37 ` [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings Alex Bennée
2022-01-18 19:41 ` Peter Maydell
2022-01-18 19:41 ` Peter Maydell
2022-01-18 23:29 ` Andre Przywara
2022-01-18 23:29 ` Andre Przywara
2022-01-19 10:15 ` Peter Maydell
2022-01-19 10:15 ` Peter Maydell
2022-01-19 21:15 ` Andre Przywara
2022-01-19 21:15 ` Andre Przywara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87pmop7xfw.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=Andre.przywara@arm.com \
--cc=drjones@redhat.com \
--cc=eric.auger@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=shashi.mallela@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.