Conversation
| } else { | ||
| return (uint8_t)(1U << interrupt->phys.route); | ||
| unsigned long route = interrupt->phys.route & MPIDR_AFF_MSK; | ||
| for (cpuid_t i = 0; i < platform.cpu_num; i++) { |
There was a problem hiding this comment.
This runs on the routing path, so I'd rather not do a table walk here at all. vgic_int_set_route already has the target phys_id in hand when it computes phys.route, so can we cache it there and make this a plain 1UL << interrupt->phys.cpu? phys.route stays the MPIDR we write to GICD_IROUTER, we just keep the cpu id next to it (INVALID_CPUID for the invalid route). phys.redist already holds the same kind of value for private interrupts, so I think it can be that same field, turning the union into a struct with route and cpu.
There was a problem hiding this comment.
Thanks — updated in f277bad.
phys is now a struct with route (MPIDR for GICD_IROUTER) and cpu (phys_id; INVALID_CPUID when the route is invalid).
As you suggested, vgic_int_set_route() stores both the MPIDR and the phys_id index, and vgic_int_ptarget_mask() just reads the cached index (1UL << interrupt->phys.cpu) with no table walk.
phys.cpu also replaces phys.redist for private interrupts — same phys_id value, just one field for both cases.
Address review on bao-project#403: avoid walking cpu_id_to_mpidr() on the routing path. Store the target phys_id next to phys.route in vgic_int_set_route() (struct phys { route, cpu }), and build the IPI mask as 1UL << phys.cpu. Reuse phys.cpu for private redist. Signed-off-by: AnHyoungBin <nks00404@gmail.com>
Address review on bao-project#403: turn phys into a struct with an anonymous union of redist/cpu (same phys_id) plus route, so private IRQ call sites keep using phys.redist while SPI caching uses phys.cpu. Signed-off-by: AnHyoungBin <nks00404@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Address review on bao-project#403: turn phys into a struct with an anonymous union of redist/cpu (same phys_id) plus route, so private IRQ call sites keep using phys.redist while SPI caching uses phys.cpu. Signed-off-by: AnHyoungBin <nks00404@gmail.com>
2e0b3e2 to
a530c22
Compare
vgic_int_ptarget_mask() builds the pCPU bitmap for VGIC_ROUTE IPIs. On GICv3, interrupt->phys.route is the physical MPIDR stored by vgic_int_set_route(), not a linear CPU index. Shifting it (e.g. 1U << 0x100 on pCPU16) is wrong and undefined when the shift exceeds the width of unsigned. The uint8_t return also truncates bits for phys_id >= 8, so broadcast masks drop CPUs 8 and above. Map phys.route back to phys_id via cpu_id_to_mpidr() and return cpumap_t on both GICv3 and GICv2. Leave phys.route as MPIDR for gicd_set_route() and affinity compares. Verified on QEMU virt (aarch64, GICv3, -smp 17): IROUTER of an SPI targeted at pCPU16 yields mask 1 << 16 instead of a truncated or shifted-MPIDR mask. Issue: bao-project#389 Signed-off-by: AnHyoungBin <nks00404@gmail.com>
Address review on bao-project#403: avoid walking cpu_id_to_mpidr() on the routing path. Store the target phys_id next to phys.route in vgic_int_set_route() (struct phys { route, cpu }), and build the IPI mask as 1UL << phys.cpu. Reuse phys.cpu for private redist. Signed-off-by: AnHyoungBin <nks00404@gmail.com>
Address review on bao-project#403: turn phys into a struct with an anonymous union of redist/cpu (same phys_id) plus route, so private IRQ call sites keep using phys.redist while SPI caching uses phys.cpu. Signed-off-by: AnHyoungBin <nks00404@gmail.com>
a530c22 to
2e14ccf
Compare
josecm
left a comment
There was a problem hiding this comment.
Looks good to me now, thanks for going through the two rounds. Two remaining things, one in the SGI path that this PR doesn't touch, and a history request.
Same shift in vgic_icc_sgir_handler
The IRM branch there still does cpu()->vcpu->vm->cpus & ~(1U << cpu()->vcpu->phys_id), which is the same narrow shift this PR fixes in vgic_int_ptarget_mask (a broadcast SGI from a pCPU at 32 or above is undefined). Since we're here, can we make it 1UL too? The vgicv2.c twin doesn't matter, GICv2 caps at 8 cpus.
History
Before merging, can you squash the three commits into one? The second and third are review fixups and don't stand on their own.
I've rebased the branch on main, so please pull it before making these changes.
vgic_int_ptarget_mask() builds the pCPU bitmap for VGIC_ROUTE IPIs. On GICv3, interrupt->phys.route is the physical MPIDR stored by vgic_int_set_route(), not a linear CPU index. Shifting it (e.g. 1U << 0x100 on pCPU16) is wrong and undefined when the shift exceeds the width of unsigned. The uint8_t return also truncates bits for phys_id >= 8, so broadcast masks drop CPUs 8 and above.
Map phys.route back to phys_id via cpu_id_to_mpidr() and return cpumap_t on both GICv3 and GICv2. Leave phys.route as MPIDR for gicd_set_route() and affinity compares.
Verified on QEMU virt (aarch64, GICv3, -smp 17): IROUTER of an SPI targeted at pCPU16 yields mask 1 << 16 instead of a truncated or shifted-MPIDR mask.
Issue: #389