* [PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter
@ 2022-07-29 8:36 Hongda Deng
2022-08-03 8:19 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Hongda Deng @ 2022-07-29 8:36 UTC (permalink / raw)
To: xen-devel
Cc: Wei.Chen, Hongda Deng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter
2022-07-29 8:36 [PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter Hongda Deng
@ 2022-08-03 8:19 ` Julien Grall
2022-08-03 9:08 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2022-08-03 8:19 UTC (permalink / raw)
To: Hongda Deng, xen-devel
Cc: Wei.Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter
2022-08-03 8:19 ` Julien Grall
@ 2022-08-03 9:08 ` Julien Grall
0 siblings, 0 replies; 3+ messages in thread
From: Julien Grall @ 2022-08-03 9:08 UTC (permalink / raw)
To: Hongda Deng, xen-devel
Cc: Wei.Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-03 9:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-29 8:36 [PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter Hongda Deng
2022-08-03 8:19 ` Julien Grall
2022-08-03 9:08 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.