When mapping guest ITS collections, vgic_lpi_stress iterates over integers in the range [0, nr_cpus), passing them as the target_addr parameter to its_send_mapc_cmd(). These integers correspond to the selftest userspace vCPU IDs that we intend to map each ITS collection to.
However, its_encode_target() within its_send_mapc_cmd() expects a vCPU's redistributor address--not the vCPU ID--as the target_addr parameter. This is evident from how its_encode_target() encodes the target_addr parameter as:
its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 51, 16)
This shows that we right-shift the input target_addr parameter by 16 bits before encoding it. This makes sense when the parameter refers to redistributor addresses (e.g., 0x20000, 0x30000) but not vCPU IDs (e.g., 0x2, 0x3).
The current impact of passing vCPU IDs to its_send_mapc_cmd() is that all vCPU IDs become 0x0 after the bit shift. Thus, when vgic_its_cmd_handle_mapc() receives the ITS command in vgic-its.c, it always interprets the collection's target_vcpu as 0. All interrupts sent to collections will be processed by vCPU 0, which defeats the purpose of this multi-vCPU test.
Fix by left-shifting the vCPU parameter received by its_send_mapc_cmd 16 bits before passing it into its_encode_target for encoding.
Signed-off-by: Maximilian Dittgen mdittgen@amazon.com --- To validate the patch, I added the following debug code at the top of vgic_its_cmd_handle_mapc:
u64 raw_cmd2 = le64_to_cpu(its_cmd[2]); u32 target_addr = its_cmd_get_target_addr(its_cmd);
kvm_info("MAPC: coll_id=%d, raw_cmd[2]=0x%llx, parsed_target=%u\n", coll_id, raw_cmd2, target_addr); vcpu = kvm_get_vcpu_by_id(kvm, its_cmd_get_target_addr(its_cmd)); kvm_info("MAPC: coll_id=%d, vcpu_id=%d\n", coll_id, vcpu ? vcpu->vcpu_id : -1);
I then ran `./vgic_lpi_stress -v 3` to trigger the stress selftest with 3 vCPUs.
Before the patch, the debug logs read: kvm [20832]: MAPC: coll_id=0, raw_cmd[2]=0x8000000000000000, parsed_target=0 kvm [20832]: MAPC: coll_id=0, vcpu_id=0 kvm [20832]: MAPC: coll_id=1, raw_cmd[2]=0x8000000000000001, parsed_target=0 kvm [20832]: MAPC: coll_id=1, vcpu_id=0 kvm [20832]: MAPC: coll_id=2, raw_cmd[2]=0x8000000000000002, parsed_target=0 kvm [20832]: MAPC: coll_id=2, vcpu_id=0
Note the last bit of the cmd string reflects the collection ID, but the rest of the cmd string reads 0. The handler parses out vCPU 0 for all 3 mapc calls.
After the patch, the debug logs read: kvm [20019]: MAPC: coll_id=0, raw_cmd[2]=0x8000000000000000, parsed_target=0 kvm [20019]: MAPC: coll_id=0, vcpu_id=0 kvm [20019]: MAPC: coll_id=1, raw_cmd[2]=0x8000000000010001, parsed_target=1 kvm [20019]: MAPC: coll_id=1, vcpu_id=1 kvm [20019]: MAPC: coll_id=2, raw_cmd[2]=0x8000000000020002, parsed_target=2 kvm [20019]: MAPC: coll_id=2, vcpu_id=2
Note that the target vcpu and target collection are both visible in the cmd string. The handler parses out the correct vCPU for all 3 mapc calls. --- tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c b/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c index 09f270545646..23c46ad17221 100644 --- a/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c +++ b/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c @@ -15,6 +15,8 @@ #include "gic_v3.h" #include "processor.h"
+#define GITS_COLLECTION_TARGET_SHIFT 16 + static u64 its_read_u64(unsigned long offset) { return readq_relaxed(GITS_BASE_GVA + offset); @@ -217,7 +219,7 @@ void its_send_mapc_cmd(void *cmdq_base, u32 vcpu_id, u32 collection_id, bool val
its_encode_cmd(&cmd, GITS_CMD_MAPC); its_encode_collection(&cmd, collection_id); - its_encode_target(&cmd, vcpu_id); + its_encode_target(&cmd, vcpu_id << GITS_COLLECTION_TARGET_SHIFT); its_encode_valid(&cmd, valid);
its_send_cmd(cmdq_base, &cmd);
On Fri, 17 Oct 2025 17:19:18 +0100, Maximilian Dittgen mdittgen@amazon.de wrote:
When mapping guest ITS collections, vgic_lpi_stress iterates over integers in the range [0, nr_cpus), passing them as the target_addr parameter to its_send_mapc_cmd(). These integers correspond to the selftest userspace vCPU IDs that we intend to map each ITS collection to.
However, its_encode_target() within its_send_mapc_cmd() expects a vCPU's redistributor address--not the vCPU ID--as the target_addr parameter. This is evident from how its_encode_target() encodes the target_addr parameter as:
its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 51, 16)This shows that we right-shift the input target_addr parameter by 16 bits before encoding it. This makes sense when the parameter refers to redistributor addresses (e.g., 0x20000, 0x30000) but not vCPU IDs (e.g., 0x2, 0x3).
From the KVM ITS emulation code:
* We use linear CPU numbers for redistributor addressing, * so GITS_TYPER.PTA is 0.
It is not an address.
M.
On Fri, 17 Oct 2025 19:06:25 +0200, Marc Zyngier maz@kernel.org wrote:
* We use linear CPU numbers for redistributor addressing, * so GITS_TYPER.PTA is 0.It is not an address.
The issue is that its_encode_target in selftests is designed for physical redistriubtor addresses (GITS_TYPER.PTA = 1) and thus performs a right shift by 16 bits:
its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 51, 16);
When the vgic_lpi_stress selftest passes in a linear vCPU id as the redistributor address (GITS_TYPER.PTA = 0 behavior), The its_encode_target function shifts the CPU numbers 16 bits right, functionally zeroing them.
We need to either: - Align this specific selftest with GITS_TYPER.PTA = 0 and not use its_encode_target to encode the target vCPU id. Instead have a dedicated encode function for the use case without a bit shift. - Align all selftests with GITS_TYPER.PTA = 0 and refactor its_encode_target to skip the bit shift altogether. - Align selftests with GITS_TYPER.PTA = 1 and pass a redistributor address, not a vCPU id, into its_send_mapc_cmd().
Otherwise, the selftest's current behavior incorrectly maps all collections to target vCPU 0.
Thanks, Maximilian
Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Mon, 20 Oct 2025 13:12:20 +0100, Maximilian Dittgen mdittgen@amazon.de wrote:
On Fri, 17 Oct 2025 19:06:25 +0200, Marc Zyngier maz@kernel.org wrote:
* We use linear CPU numbers for redistributor addressing, * so GITS_TYPER.PTA is 0.It is not an address.
The issue is that its_encode_target in selftests is designed for physical redistriubtor addresses (GITS_TYPER.PTA = 1) and thus
No. Please read the spec.
performs a right shift by 16 bits:
its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 51, 16);When the vgic_lpi_stress selftest passes in a linear vCPU id as the redistributor address (GITS_TYPER.PTA = 0 behavior), The its_encode_target function shifts the CPU numbers 16 bits right, functionally zeroing them.
We need to either:
- Align this specific selftest with GITS_TYPER.PTA = 0 and not use its_encode_target to encode the target vCPU id. Instead have a dedicated encode function for the use case without a bit shift.
- Align all selftests with GITS_TYPER.PTA = 0 and refactor its_encode_target to skip the bit shift altogether.
- Align selftests with GITS_TYPER.PTA = 1 and pass a redistributor address, not a vCPU id, into its_send_mapc_cmd().
What part of "GITS_TYPER.PTA is 0" did you miss?
Otherwise, the selftest's current behavior incorrectly maps all collections to target vCPU 0.
To be clear: I don't object to the patch. I object to the nonsensical commit message.
You cannot say "I'm replacing the vcpu_id with an address". That's not for you to decide, as the emulated HW is *imposing* that decision on you. You don't even have the addresses at which the RDs are. You are merely *reformatting* the vcpu_id to fit a field that can *also* contain a 64kB-aligned address *when GITS_TYPER.PTA==1*. See 5.3.1 in the GICv3 spec.
And yes, what you have is the correct fix. Just wrap it as a helper (I'd suggest procnum_to_rdbase(), if you need a name for it).
This test also violate the architecture by not performing a SYNC after any command, which would also require the use of a properly formatted target field. But hey, correctness is overrated.
Thanks,
M.
On Mon, 20 Oct 2025 15:01:15 +0200, Marc Zyngier maz@kernel.org wrote:
To be clear: I don't object to the patch. I object to the nonsensical commit message.
And yes, what you have is the correct fix. Just wrap it as a helper I'd suggest procnum_to_rdbase(), if you need a name for it).
Thank you for the feedback, I've created a PATCH v2 with revised commit message and a procnum_to_rdbase() helper.
This test also violate the architecture by not performing a SYNC after any command, which would also require the use of a properly formatted target field. But hey, correctness is overrated.
I am currently working on a RFC patchset for per-vCPU vLPI enablement that extends this selftest to test the feature. I will package in SYNCs as a patch on the RFC.
Thanks, Maximilian
Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
linux-kselftest-mirror@lists.linaro.org