Re: [PATCH] KVM: selftests: fix ITS collection target addresses in vgic_lpi_stress

[Marc Zyngier <maz@kernel.org>]

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.

-- 
Without deviation from the norm, progress is not possible.