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

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

[Hongda Deng]

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].
But there seems to be a bug in the way the offset is calculated when
vgic tries to store irouter.

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

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.

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@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

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

[Julien Grall]

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@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;

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

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

[Luca Fancellu]

Hi Julien,

>> /* 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?

I think that function is ok, because in there we have:

/* There is exactly 1 vIRQ per IROUTER */
offset = offset  / NR_BYTES_PER_IROUTER;

/* Get the index in the rank */
offset = offset & INTERRUPT_RANK_MASK;

Which is basically offset = (offset  / NR_BYTES_PER_IROUTER) & INTERRUPT_RANK_MASK;

Like in the counterpart (updated by this patch) vgic_store_irouter who has:

/* There is 1 vIRQ per IROUTER */
virq = offset / NR_BYTES_PER_IROUTER;

[…]

/* Get the index in the rank */
offset = virq & INTERRUPT_RANK_MASK;

Which is the same as above

Cheers,
Luca

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

[Julien Grall]

On 26/07/2022 18:54, Luca Fancellu wrote:
> 
> Hi Julien,

Hi Luca,

>>> /* 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?
> 
> I think that function is ok, because in there we have:
> 
> /* There is exactly 1 vIRQ per IROUTER */
> offset = offset  / NR_BYTES_PER_IROUTER;
> 
> /* Get the index in the rank */
> offset = offset & INTERRUPT_RANK_MASK;
> 
> Which is basically offset = (offset  / NR_BYTES_PER_IROUTER) & INTERRUPT_RANK_MASK;
> 
> Like in the counterpart (updated by this patch) vgic_store_irouter who has:
> 
> /* There is 1 vIRQ per IROUTER */
> virq = offset / NR_BYTES_PER_IROUTER;
> 
> […]
> 
> /* Get the index in the rank */
> offset = virq & INTERRUPT_RANK_MASK;
> 
> Which is the same as above

You are right. So the patch looks correct to me.

Although, I would still like the commit message to be clarified.

Cheers,

-- 
Julien Grall