Re: [PATCH] arm/vgic-v3: fix virq offset in the rank

[Julien Grall <julien@xxxxxxx>]

Hi,

Title: I would suggest to mention the irouter in the title.

On 15/07/2022 11:46, Hongda Deng wrote:
> When vgic performs irouter registers emulation, to get the target vCPU
> via virq conveniently, Xen doesn't store the irouter value directly,
> instead it will use the value (affinities) in irouter to calculate the
> target vCPU, and then save the target vCPU in irq rank->vCPU[offset].
NIT: The field is technically called "vcpu".

> But there seems to be a bug in the way the offset is calculated when
> vgic tries to store irouter.
NIT: I would drop this sentence because below you suggest this is a bug.

> When vgic tries to get the target vcpu, it first calculates the target
Typo: When the vGIC...

> vcpu index via
>    int target = read_atomic(&rank->vcpu[virq & INTERRUPT_RANK_MASK]);
> and then get the target vcpu via
>    v->domain->vcpu[target];
>
> When vgic tries to store irouter for one virq, the target vcpu index
> in the rank is got via
>    offset &= virq & INTERRUPT_RANK_MASK;
> finally it gets the target vcpu via
>    d->vcpu[read_atomic(&rank->vcpu[offset])];
>
> There is a difference between them when gets the target vcpu index in
> the rank.
Typo: I think you mean 'getting' rather than 'gets'.

> Here (virq & INTERRUPT_RANK_MASK) would already get the  target vcpu
> index in the rank, so we don't need the '&' before '=' when calculate
> the offset.
This is a bit confusing to read. Through the commit message you give mix 
information about the issue. "don't need" to me means this is pointless 
but harmless. But then a bit below, you write this is a bug.
I would suggest to change the order with the next paragraph (it may need 
some tweaks) and replace the "don't need" with something more assertive.
> For example, the target vcpu index in the rank should be 6 for virq 38,
> but vgic will get offset=0 when vgic stores the irouter for this virq,
> and finally vgic will access wrong target vcpu index in the rank when
> it updates the irouter.
>
> Fixes: 5d495f4349b5 ("xen/arm: vgic: Optimize the way to store the target vCPU in 
> the rank")
> Signed-off-by: Hongda Deng <Hongda.Deng@xxxxxxx>
> ---
>   xen/arch/arm/vgic-v3.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index e4ba9a6476..7fb99a9ff2 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -135,7 +135,7 @@ static void vgic_store_irouter(struct domain *d, struct 
> vgic_irq_rank *rank,
>       ASSERT(virq >= 32);
>   
>       /* Get the index in the rank */
> -    offset &= virq & INTERRUPT_RANK_MASK;
> +    offset = virq & INTERRUPT_RANK_MASK;
AFAICT, vgic_fetch_irouter() has the same problem. Can you update it 
here as well?
>   
>       new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
>       old_vcpu = d->vcpu[read_atomic(&rank->vcpu[offset])];
Cheers,

--
Julien Grall