Re: [PATCH] arm/vgic-v3: fix virq offset in the rank
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
Re: [PATCH] arm/vgic-v3: fix virq offset in the rank
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
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(>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(>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
---
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(>vcpu[offset])];
Cheers,
--
Julien Grall
[PATCH] arm/vgic-v3: fix virq offset in the rank
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(>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(>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
---
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(>vcpu[offset])];
--
2.25.1
