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].

When vGIC tries to get the target vCPU, it first calculates the target
vCPU index via
  int target = read_atomic(&rank->vcpu[virq & INTERRUPT_RANK_MASK]);
and then it gets 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 computed as
  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 while getting the target vCPU index
in the rank. Actually (virq & INTERRUPT_RANK_MASK) would already get
the target vCPU index in the rank, it's wrong to add '&' before '=' when
calculate the offset.

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 the wrong target vCPU index in the rank
when updating 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@arm.com>
---
 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;
 
     new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
     old_vcpu = d->vcpu[read_atomic(&rank->vcpu[offset])];
-- 
2.25.1

Hi Hongda,

On 29/07/2022 09:36, 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].
> 
> When vGIC tries to get the target vCPU, it first calculates the target
> vCPU index via
>    int target = read_atomic(&rank->vcpu[virq & INTERRUPT_RANK_MASK]);
> and then it gets 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 computed as
>    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 while getting the target vCPU index
> in the rank. Actually (virq & INTERRUPT_RANK_MASK) would already get
> the target vCPU index in the rank, it's wrong to add '&' before '=' when
> calculate the offset.
> 
> 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 the wrong target vCPU index in the rank
> when updating 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@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

This will also want to be backported. I will add it in my queue.


> ---

For future series, please provide a changelog for every version.

>   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;
>   
>       new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
>       old_vcpu = d->vcpu[read_atomic(&rank->vcpu[offset])];

Cheers,

-- 
Julien Grall

On 03/08/2022 09:19, Julien Grall wrote:
> Hi Hongda,
> 
> On 29/07/2022 09:36, 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].
>>
>> When vGIC tries to get the target vCPU, it first calculates the target
>> vCPU index via
>>    int target = read_atomic(&rank->vcpu[virq & INTERRUPT_RANK_MASK]);
>> and then it gets 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 computed as
>>    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 while getting the target vCPU index
>> in the rank. Actually (virq & INTERRUPT_RANK_MASK) would already get
>> the target vCPU index in the rank, it's wrong to add '&' before '=' when
>> calculate the offset.
>>
>> 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 the wrong target vCPU index in the rank
>> when updating 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@arm.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Committed.

Cheers,

-- 
Julien Grall