[PATCH 0/3] irqchip/gic-v4: Fix VMAPP/VMOVP races

[PATCH 0/3] irqchip/gic-v4: Fix VMAPP/VMOVP races

[Marc Zyngier]

In 20240701072305.4129823-1-tangnianyao@huawei.com, Nianyao reports
a number of possible races that can trigger on GICv4 implementations
using the ITSList feature.

These races involve concurrent VMOVP and VMAPP, the former happening
on vcpu load, while the latter is triggered on the first device being
MAPTI'd on a given ITS for this particular guest.

The main issue is that we want to establish the affinity at VMAPP time,
while vcpu_load wants to set the affinity where the vcpu actually runs.
Lock ordering constraints mean that we can't lock the VPE on VMAPP,
so things are modified without any lock. What could possibly go wrong?

THe fix is a bit involved, and relies on 3 things:

- Making sure that the initial affinity of a VPE is fixed at activate
  time, which is done early in the life of the vcpup, before it can run.

- Add a per-VM lock that can be taken instead of the global vmovp_lock,
  paving the way for a more manageable lock order.

- Take the per-VPE lock whenever modifying the VPE affinity, as expected
  everywhere else in the code.

With that, VMAPP and VMOVP can now run concurrently and still lead to
sensible results.

Marc Zyngier (3):
  irqchip/gic-v4: Always configure affinity on VPE activation
  irqchip/gic-v4: Substitute vmovp_lock for a per-VM lock
  irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

 drivers/irqchip/irq-gic-v3-its.c   | 48 ++++++++++++++----------------
 include/linux/irqchip/arm-gic-v4.h |  8 +++++
 2 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.39.2

[PATCH 1/3] irqchip/gic-v4: Always configure affinity on VPE activation

[Marc Zyngier]

We currently have two paths to set the initial affinity of a VPE:

- at activation time on GICv4 without the stupid VMOVP list, and
  on GICv4.1

- at map time for GICv4 with VMOVP list

The latter location may end-up modifying the affinity of VPE
that is currently running, making the results unpredictible.

Instead, unify the two paths, making sure we only set the
initial affinity at activation time.

Reported-by: Nianyao Tang <tangnianyao@huawei.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 3c755d5dad6e6..a00c5e8c4ea65 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1809,13 +1809,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
 
 		for (i = 0; i < vm->nr_vpes; i++) {
 			struct its_vpe *vpe = vm->vpes[i];
-			struct irq_data *d = irq_get_irq_data(vpe->irq);
 
-			/* Map the VPE to the first possible CPU */
-			vpe->col_idx = cpumask_first(cpu_online_mask);
 			its_send_vmapp(its, vpe, true);
 			its_send_vinvall(its, vpe);
-			irq_data_update_effective_affinity(d, cpumask_of(vpe->col_idx));
 		}
 	}
 
@@ -4562,6 +4558,10 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain,
 	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
 	struct its_node *its;
 
+	/* Map the VPE to the first possible CPU */
+	vpe->col_idx = cpumask_first(cpu_online_mask);
+	irq_data_update_effective_affinity(d, cpumask_of(vpe->col_idx));
+
 	/*
 	 * If we use the list map, we issue VMAPP on demand... Unless
 	 * we're on a GICv4.1 and we eagerly map the VPE on all ITSs
@@ -4570,9 +4570,6 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain,
 	if (!gic_requires_eager_mapping())
 		return 0;
 
-	/* Map the VPE to the first possible CPU */
-	vpe->col_idx = cpumask_first(cpu_online_mask);
-
 	list_for_each_entry(its, &its_nodes, entry) {
 		if (!is_v4(its))
 			continue;
@@ -4581,8 +4578,6 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain,
 		its_send_vinvall(its, vpe);
 	}
 
-	irq_data_update_effective_affinity(d, cpumask_of(vpe->col_idx));
-
 	return 0;
 }
 
-- 
2.39.2

[PATCH 2/3] irqchip/gic-v4: Substitute vmovp_lock for a per-VM lock

[Marc Zyngier]

vmovp_lock is abused in a number of cases to serialise updates
to vlpi_count[] and deal with map/unmap of a VM to ITSs.

Instead, provide a per-VM lock and revisit the use of vlpi_count[]
so that it is always wrapped in this per-VM vmapp_lock.

This reduces the potential contention on a concurrent VMOVP command,
and paves the way for subsequent VPE locking that holding vmovp_lock
actively prevents due to the lock ordering.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c   | 27 ++++++++++++---------------
 include/linux/irqchip/arm-gic-v4.h |  8 ++++++++
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index a00c5e8c4ea65..b52d60097cad5 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1317,7 +1317,6 @@ static void its_send_vmovp(struct its_vpe *vpe)
 {
 	struct its_cmd_desc desc = {};
 	struct its_node *its;
-	unsigned long flags;
 	int col_id = vpe->col_idx;
 
 	desc.its_vmovp_cmd.vpe = vpe;
@@ -1329,6 +1328,12 @@ static void its_send_vmovp(struct its_vpe *vpe)
 		return;
 	}
 
+	/*
+	 * Protect against concurrent updates of the mapping state on
+	 * individual VMs.
+	 */
+	guard(raw_spinlock_irqsave)(&vpe->its_vm->vmapp_lock);
+
 	/*
 	 * Yet another marvel of the architecture. If using the
 	 * its_list "feature", we need to make sure that all ITSs
@@ -1337,8 +1342,7 @@ static void its_send_vmovp(struct its_vpe *vpe)
 	 *
 	 * Wall <-- Head.
 	 */
-	raw_spin_lock_irqsave(&vmovp_lock, flags);
-
+	guard(raw_spinlock)(&vmovp_lock);
 	desc.its_vmovp_cmd.seq_num = vmovp_seq_num++;
 	desc.its_vmovp_cmd.its_list = get_its_list(vpe->its_vm);
 
@@ -1353,8 +1357,6 @@ static void its_send_vmovp(struct its_vpe *vpe)
 		desc.its_vmovp_cmd.col = &its->collections[col_id];
 		its_send_single_vcommand(its, its_build_vmovp_cmd, &desc);
 	}
-
-	raw_spin_unlock_irqrestore(&vmovp_lock, flags);
 }
 
 static void its_send_vinvall(struct its_node *its, struct its_vpe *vpe)
@@ -1791,12 +1793,10 @@ static bool gic_requires_eager_mapping(void)
 
 static void its_map_vm(struct its_node *its, struct its_vm *vm)
 {
-	unsigned long flags;
-
 	if (gic_requires_eager_mapping())
 		return;
 
-	raw_spin_lock_irqsave(&vmovp_lock, flags);
+	guard(raw_spinlock_irqsave)(&vm->vmapp_lock);
 
 	/*
 	 * If the VM wasn't mapped yet, iterate over the vpes and get
@@ -1814,19 +1814,15 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
 			its_send_vinvall(its, vpe);
 		}
 	}
-
-	raw_spin_unlock_irqrestore(&vmovp_lock, flags);
 }
 
 static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
 {
-	unsigned long flags;
-
 	/* Not using the ITS list? Everything is always mapped. */
 	if (gic_requires_eager_mapping())
 		return;
 
-	raw_spin_lock_irqsave(&vmovp_lock, flags);
+	guard(raw_spinlock_irqsave)(&vm->vmapp_lock);
 
 	if (!--vm->vlpi_count[its->list_nr]) {
 		int i;
@@ -1834,8 +1830,6 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
 		for (i = 0; i < vm->nr_vpes; i++)
 			its_send_vmapp(its, vm->vpes[i], false);
 	}
-
-	raw_spin_unlock_irqrestore(&vmovp_lock, flags);
 }
 
 static int its_vlpi_map(struct irq_data *d, struct its_cmd_info *info)
@@ -3922,6 +3916,8 @@ static void its_vpe_invall(struct its_vpe *vpe)
 {
 	struct its_node *its;
 
+	guard(raw_spinlock_irqsave)(&vpe->its_vm->vmapp_lock);
+
 	list_for_each_entry(its, &its_nodes, entry) {
 		if (!is_v4(its))
 			continue;
@@ -4527,6 +4523,7 @@ static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int virq
 	vm->db_lpi_base = base;
 	vm->nr_db_lpis = nr_ids;
 	vm->vprop_page = vprop_page;
+	raw_spin_lock_init(&vm->vmapp_lock);
 
 	if (gic_rdists->has_rvpeid)
 		irqchip = &its_vpe_4_1_irq_chip;
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 2c63375bbd43f..ecabed6d33075 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -25,6 +25,14 @@ struct its_vm {
 	irq_hw_number_t		db_lpi_base;
 	unsigned long		*db_bitmap;
 	int			nr_db_lpis;
+	/*
+	 * Ensures mutual exclusion between updates to vlpi_count[]
+	 * and map/unmap when using the ITSList mechanism.
+	 *
+	 * The lock order for any sequence involving the ITSList is
+	 * vmapp_lock -> vpe_lock ->vmovp_lock.
+	 */
+	raw_spinlock_t		vmapp_lock;
 	u32			vlpi_count[GICv4_ITS_LIST_MAX];
 };
 
-- 
2.39.2

[PATCH 3/3] irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

[Marc Zyngier]

In order to make sure that vpe->col_idx is correctly sampled
when a VMAPP command is issued, we must hold the lock for the
VPE. This is now possible since the introduction of the per-VM
vmapp_lock, which can be taken before vpe_lock in the locking
order.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index b52d60097cad5..951ec140bcea2 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1810,7 +1810,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
 		for (i = 0; i < vm->nr_vpes; i++) {
 			struct its_vpe *vpe = vm->vpes[i];
 
-			its_send_vmapp(its, vpe, true);
+			scoped_guard(raw_spinlock, &vpe->vpe_lock)
+				its_send_vmapp(its, vpe, true);
+
 			its_send_vinvall(its, vpe);
 		}
 	}
@@ -1827,8 +1829,10 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
 	if (!--vm->vlpi_count[its->list_nr]) {
 		int i;
 
-		for (i = 0; i < vm->nr_vpes; i++)
+		for (i = 0; i < vm->nr_vpes; i++) {
+			guard(raw_spinlock)(&vm->vpes[i]->vpe_lock);
 			its_send_vmapp(its, vm->vpes[i], false);
+		}
 	}
 }
 
-- 
2.39.2

Re: [PATCH 0/3] irqchip/gic-v4: Fix VMAPP/VMOVP races

[Tangnianyao]

On 7/5/2024 17:31, Marc Zyngier wrote:
> In 20240701072305.4129823-1-tangnianyao@huawei.com, Nianyao reports
> a number of possible races that can trigger on GICv4 implementations
> using the ITSList feature.
>
> These races involve concurrent VMOVP and VMAPP, the former happening
> on vcpu load, while the latter is triggered on the first device being
> MAPTI'd on a given ITS for this particular guest.
>
> The main issue is that we want to establish the affinity at VMAPP time,
> while vcpu_load wants to set the affinity where the vcpu actually runs.
> Lock ordering constraints mean that we can't lock the VPE on VMAPP,
> so things are modified without any lock. What could possibly go wrong?
>
> THe fix is a bit involved, and relies on 3 things:
>
> - Making sure that the initial affinity of a VPE is fixed at activate
>   time, which is done early in the life of the vcpup, before it can run.
>
> - Add a per-VM lock that can be taken instead of the global vmovp_lock,
>   paving the way for a more manageable lock order.
>
> - Take the per-VPE lock whenever modifying the VPE affinity, as expected
>   everywhere else in the code.
>
> With that, VMAPP and VMOVP can now run concurrently and still lead to
> sensible results.
>
> Marc Zyngier (3):
>   irqchip/gic-v4: Always configure affinity on VPE activation
>   irqchip/gic-v4: Substitute vmovp_lock for a per-VM lock
>   irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued
>
>  drivers/irqchip/irq-gic-v3-its.c   | 48 ++++++++++++++----------------
>  include/linux/irqchip/arm-gic-v4.h |  8 +++++
>  2 files changed, 30 insertions(+), 26 deletions(-)
>

I've tested this patch series. It works.

Tested-by: Nianyao Tang <tangnianyao@huawei.com>

Re: [PATCH 0/3] irqchip/gic-v4: Fix VMAPP/VMOVP races

[Tangnianyao]

Hi, Marc

I meet another problem while fixing this in kernel 5.10.

Kernel 5.10 does not support guard and we replace it with raw_spin_lock/unlock.

When guest insmod nic drivers, it trigger host cpu power off somehow. The same

testcase runs quite good in kernel 6.6(both host and guest).

Would you fix this on long-term kernel 5.10? Or any sugguestion?


Thanks for your help.

Nianyao Tang



On 7/8/2024 10:02, Tangnianyao wrote:
>
> On 7/5/2024 17:31, Marc Zyngier wrote:
>> In 20240701072305.4129823-1-tangnianyao@huawei.com, Nianyao reports
>> a number of possible races that can trigger on GICv4 implementations
>> using the ITSList feature.
>>
>> These races involve concurrent VMOVP and VMAPP, the former happening
>> on vcpu load, while the latter is triggered on the first device being
>> MAPTI'd on a given ITS for this particular guest.
>>
>> The main issue is that we want to establish the affinity at VMAPP time,
>> while vcpu_load wants to set the affinity where the vcpu actually runs.
>> Lock ordering constraints mean that we can't lock the VPE on VMAPP,
>> so things are modified without any lock. What could possibly go wrong?
>>
>> THe fix is a bit involved, and relies on 3 things:
>>
>> - Making sure that the initial affinity of a VPE is fixed at activate
>>   time, which is done early in the life of the vcpup, before it can run.
>>
>> - Add a per-VM lock that can be taken instead of the global vmovp_lock,
>>   paving the way for a more manageable lock order.
>>
>> - Take the per-VPE lock whenever modifying the VPE affinity, as expected
>>   everywhere else in the code.
>>
>> With that, VMAPP and VMOVP can now run concurrently and still lead to
>> sensible results.
>>
>> Marc Zyngier (3):
>>   irqchip/gic-v4: Always configure affinity on VPE activation
>>   irqchip/gic-v4: Substitute vmovp_lock for a per-VM lock
>>   irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued
>>
>>  drivers/irqchip/irq-gic-v3-its.c   | 48 ++++++++++++++----------------
>>  include/linux/irqchip/arm-gic-v4.h |  8 +++++
>>  2 files changed, 30 insertions(+), 26 deletions(-)
>>
> I've tested this patch series. It works.
>
> Tested-by: Nianyao Tang <tangnianyao@huawei.com>
>
> .
>

Re: [PATCH 0/3] irqchip/gic-v4: Fix VMAPP/VMOVP races

[Marc Zyngier]

On Wed, 17 Jul 2024 09:41:30 +0100,
Tangnianyao <tangnianyao@huawei.com> wrote:
> 
> Hi, Marc
> 
> I meet another problem while fixing this in kernel 5.10.
> 
> Kernel 5.10 does not support guard and we replace it with raw_spin_lock/unlock.
> When guest insmod nic drivers, it trigger host cpu power off somehow. The same
> testcase runs quite good in kernel 6.6(both host and guest).

This suggests that you got the locking order wrong, that one or
several CPUs deadlock, and that a watchdog fires.

> Would you fix this on long-term kernel 5.10? Or any sugguestion?

Sorry, but I have no intention to maintain such an ancient kernel,
which is why I did not Cc stable on these patches. I have no bandwidth
for this, nor any interest in it.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

[Zhou Wang]

On 2024/7/5 17:31, Marc Zyngier wrote:
> In order to make sure that vpe->col_idx is correctly sampled
> when a VMAPP command is issued, we must hold the lock for the
> VPE. This is now possible since the introduction of the per-VM
> vmapp_lock, which can be taken before vpe_lock in the locking
> order.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index b52d60097cad5..951ec140bcea2 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1810,7 +1810,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
>  		for (i = 0; i < vm->nr_vpes; i++) {
>  			struct its_vpe *vpe = vm->vpes[i];
>  
> -			its_send_vmapp(its, vpe, true);
> +			scoped_guard(raw_spinlock, &vpe->vpe_lock)
> +				its_send_vmapp(its, vpe, true);
> +
>  			its_send_vinvall(its, vpe);
>  		}
>  	}
> @@ -1827,8 +1829,10 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
>  	if (!--vm->vlpi_count[its->list_nr]) {
>  		int i;
>  
> -		for (i = 0; i < vm->nr_vpes; i++)
> +		for (i = 0; i < vm->nr_vpes; i++) {
> +			guard(raw_spinlock)(&vm->vpes[i]->vpe_lock);
>  			its_send_vmapp(its, vm->vpes[i], false);
> +		}
>  	}
>  }
>  

Hi Marc,

It looks like there is ABBA deadlock after applying this series:

In its_map_vm: vmapp_lock -> vpe_lock
In its_vpe_set_affinity: vpe_to_cpuid_lock(vpe_lock) -> its_send_vmovp(vmapp_lock)

Any idea about this?

Best,
Zhou

Re: [PATCH 3/3] irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

[Marc Zyngier]

On Fri, 19 Jul 2024 10:42:02 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> On 2024/7/5 17:31, Marc Zyngier wrote:
> > In order to make sure that vpe->col_idx is correctly sampled
> > when a VMAPP command is issued, we must hold the lock for the
> > VPE. This is now possible since the introduction of the per-VM
> > vmapp_lock, which can be taken before vpe_lock in the locking
> > order.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/irqchip/irq-gic-v3-its.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index b52d60097cad5..951ec140bcea2 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -1810,7 +1810,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
> >  		for (i = 0; i < vm->nr_vpes; i++) {
> >  			struct its_vpe *vpe = vm->vpes[i];
> >  
> > -			its_send_vmapp(its, vpe, true);
> > +			scoped_guard(raw_spinlock, &vpe->vpe_lock)
> > +				its_send_vmapp(its, vpe, true);
> > +
> >  			its_send_vinvall(its, vpe);
> >  		}
> >  	}
> > @@ -1827,8 +1829,10 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
> >  	if (!--vm->vlpi_count[its->list_nr]) {
> >  		int i;
> >  
> > -		for (i = 0; i < vm->nr_vpes; i++)
> > +		for (i = 0; i < vm->nr_vpes; i++) {
> > +			guard(raw_spinlock)(&vm->vpes[i]->vpe_lock);
> >  			its_send_vmapp(its, vm->vpes[i], false);
> > +		}
> >  	}
> >  }
> >  
> 
> Hi Marc,
> 
> It looks like there is ABBA deadlock after applying this series:
> 
> In its_map_vm: vmapp_lock -> vpe_lock
> In its_vpe_set_affinity: vpe_to_cpuid_lock(vpe_lock) -> its_send_vmovp(vmapp_lock)
> 
> Any idea about this?

Hmmm, well spotted. That's an annoying one.

Can you give the below hack a go? I've only lightly tested it, as my
D05 box is on its last leg (it is literally falling apart) and I don't
have any other GICv4.x box to test on.

Thanks,

	M.

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 951ec140bcea2..b88c6011c8771 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1328,12 +1328,6 @@ static void its_send_vmovp(struct its_vpe *vpe)
 		return;
 	}
 
-	/*
-	 * Protect against concurrent updates of the mapping state on
-	 * individual VMs.
-	 */
-	guard(raw_spinlock_irqsave)(&vpe->its_vm->vmapp_lock);
-
 	/*
 	 * Yet another marvel of the architecture. If using the
 	 * its_list "feature", we need to make sure that all ITSs
@@ -3808,7 +3802,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
 	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
 	unsigned int from, cpu = nr_cpu_ids;
 	struct cpumask *table_mask;
-	unsigned long flags;
+	unsigned long flags, vmapp_flags;
 
 	/*
 	 * Changing affinity is mega expensive, so let's be as lazy as
@@ -3822,7 +3816,14 @@ static int its_vpe_set_affinity(struct irq_data *d,
 	 * protect us, and that we must ensure nobody samples vpe->col_idx
 	 * during the update, hence the lock below which must also be
 	 * taken on any vLPI handling path that evaluates vpe->col_idx.
+	 *
+	 * Finally, we must protect ourselves against concurrent
+	 * updates of the mapping state on this VM should the ITS list
+	 * be in use.
 	 */
+	if (its_list_map)
+		raw_spin_lock_irqsave(&vpe->its_vm->vmapp_lock, vmapp_flags);
+
 	from = vpe_to_cpuid_lock(vpe, &flags);
 	table_mask = gic_data_rdist_cpu(from)->vpe_table_mask;
 
@@ -3852,6 +3853,9 @@ static int its_vpe_set_affinity(struct irq_data *d,
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 	vpe_to_cpuid_unlock(vpe, flags);
 
+	if (its_list_map)
+		raw_spin_unlock_irqrestore(&vpe->its_vm->vmapp_lock, vmapp_flags);
+
 	return IRQ_SET_MASK_OK_DONE;
 }
 

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

[Zhou Wang]

On 2024/7/19 19:31, Marc Zyngier wrote:
> On Fri, 19 Jul 2024 10:42:02 +0100,
> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>
>> On 2024/7/5 17:31, Marc Zyngier wrote:
>>> In order to make sure that vpe->col_idx is correctly sampled
>>> when a VMAPP command is issued, we must hold the lock for the
>>> VPE. This is now possible since the introduction of the per-VM
>>> vmapp_lock, which can be taken before vpe_lock in the locking
>>> order.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  drivers/irqchip/irq-gic-v3-its.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index b52d60097cad5..951ec140bcea2 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -1810,7 +1810,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
>>>  		for (i = 0; i < vm->nr_vpes; i++) {
>>>  			struct its_vpe *vpe = vm->vpes[i];
>>>  
>>> -			its_send_vmapp(its, vpe, true);
>>> +			scoped_guard(raw_spinlock, &vpe->vpe_lock)
>>> +				its_send_vmapp(its, vpe, true);
>>> +
>>>  			its_send_vinvall(its, vpe);
>>>  		}
>>>  	}
>>> @@ -1827,8 +1829,10 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
>>>  	if (!--vm->vlpi_count[its->list_nr]) {
>>>  		int i;
>>>  
>>> -		for (i = 0; i < vm->nr_vpes; i++)
>>> +		for (i = 0; i < vm->nr_vpes; i++) {
>>> +			guard(raw_spinlock)(&vm->vpes[i]->vpe_lock);
>>>  			its_send_vmapp(its, vm->vpes[i], false);
>>> +		}
>>>  	}
>>>  }
>>>  
>>
>> Hi Marc,
>>
>> It looks like there is ABBA deadlock after applying this series:
>>
>> In its_map_vm: vmapp_lock -> vpe_lock
>> In its_vpe_set_affinity: vpe_to_cpuid_lock(vpe_lock) -> its_send_vmovp(vmapp_lock)
>>
>> Any idea about this?
> 
> Hmmm, well spotted. That's an annoying one.
> 
> Can you give the below hack a go? I've only lightly tested it, as my
> D05 box is on its last leg (it is literally falling apart) and I don't
> have any other GICv4.x box to test on.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 951ec140bcea2..b88c6011c8771 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1328,12 +1328,6 @@ static void its_send_vmovp(struct its_vpe *vpe)
>  		return;
>  	}
>  
> -	/*
> -	 * Protect against concurrent updates of the mapping state on
> -	 * individual VMs.
> -	 */
> -	guard(raw_spinlock_irqsave)(&vpe->its_vm->vmapp_lock);
> -
>  	/*
>  	 * Yet another marvel of the architecture. If using the
>  	 * its_list "feature", we need to make sure that all ITSs
> @@ -3808,7 +3802,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
>  	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
>  	unsigned int from, cpu = nr_cpu_ids;
>  	struct cpumask *table_mask;
> -	unsigned long flags;
> +	unsigned long flags, vmapp_flags;
>  
>  	/*
>  	 * Changing affinity is mega expensive, so let's be as lazy as
> @@ -3822,7 +3816,14 @@ static int its_vpe_set_affinity(struct irq_data *d,
>  	 * protect us, and that we must ensure nobody samples vpe->col_idx
>  	 * during the update, hence the lock below which must also be
>  	 * taken on any vLPI handling path that evaluates vpe->col_idx.
> +	 *
> +	 * Finally, we must protect ourselves against concurrent
> +	 * updates of the mapping state on this VM should the ITS list
> +	 * be in use.
>  	 */
> +	if (its_list_map)
> +		raw_spin_lock_irqsave(&vpe->its_vm->vmapp_lock, vmapp_flags);
> +
>  	from = vpe_to_cpuid_lock(vpe, &flags);
>  	table_mask = gic_data_rdist_cpu(from)->vpe_table_mask;
>  
> @@ -3852,6 +3853,9 @@ static int its_vpe_set_affinity(struct irq_data *d,
>  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>  	vpe_to_cpuid_unlock(vpe, flags);
>  
> +	if (its_list_map)
> +		raw_spin_unlock_irqrestore(&vpe->its_vm->vmapp_lock, vmapp_flags);
> +
>  	return IRQ_SET_MASK_OK_DONE;
>  }
>

Hi Marc，

We add above code to do test again. Now it is OK.

Bests,
Zhou

> 

Re: [PATCH 3/3] irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

[Marc Zyngier]

On Tue, 23 Jul 2024 02:51:32 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> On 2024/7/19 19:31, Marc Zyngier wrote:
> > On Fri, 19 Jul 2024 10:42:02 +0100,
> > Zhou Wang <wangzhou1@hisilicon.com> wrote:
> >>
> >> On 2024/7/5 17:31, Marc Zyngier wrote:
> >>> In order to make sure that vpe->col_idx is correctly sampled
> >>> when a VMAPP command is issued, we must hold the lock for the
> >>> VPE. This is now possible since the introduction of the per-VM
> >>> vmapp_lock, which can be taken before vpe_lock in the locking
> >>> order.
> >>>
> >>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >>> ---
> >>>  drivers/irqchip/irq-gic-v3-its.c | 8 ++++++--
> >>>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> >>> index b52d60097cad5..951ec140bcea2 100644
> >>> --- a/drivers/irqchip/irq-gic-v3-its.c
> >>> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >>> @@ -1810,7 +1810,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
> >>>  		for (i = 0; i < vm->nr_vpes; i++) {
> >>>  			struct its_vpe *vpe = vm->vpes[i];
> >>>  
> >>> -			its_send_vmapp(its, vpe, true);
> >>> +			scoped_guard(raw_spinlock, &vpe->vpe_lock)
> >>> +				its_send_vmapp(its, vpe, true);
> >>> +
> >>>  			its_send_vinvall(its, vpe);
> >>>  		}
> >>>  	}
> >>> @@ -1827,8 +1829,10 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
> >>>  	if (!--vm->vlpi_count[its->list_nr]) {
> >>>  		int i;
> >>>  
> >>> -		for (i = 0; i < vm->nr_vpes; i++)
> >>> +		for (i = 0; i < vm->nr_vpes; i++) {
> >>> +			guard(raw_spinlock)(&vm->vpes[i]->vpe_lock);
> >>>  			its_send_vmapp(its, vm->vpes[i], false);
> >>> +		}
> >>>  	}
> >>>  }
> >>>  
> >>
> >> Hi Marc,
> >>
> >> It looks like there is ABBA deadlock after applying this series:
> >>
> >> In its_map_vm: vmapp_lock -> vpe_lock
> >> In its_vpe_set_affinity: vpe_to_cpuid_lock(vpe_lock) -> its_send_vmovp(vmapp_lock)
> >>
> >> Any idea about this?
> > 
> > Hmmm, well spotted. That's an annoying one.
> > 
> > Can you give the below hack a go? I've only lightly tested it, as my
> > D05 box is on its last leg (it is literally falling apart) and I don't
> > have any other GICv4.x box to test on.
> > 
> > Thanks,
> > 
> > 	M.
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 951ec140bcea2..b88c6011c8771 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -1328,12 +1328,6 @@ static void its_send_vmovp(struct its_vpe *vpe)
> >  		return;
> >  	}
> >  
> > -	/*
> > -	 * Protect against concurrent updates of the mapping state on
> > -	 * individual VMs.
> > -	 */
> > -	guard(raw_spinlock_irqsave)(&vpe->its_vm->vmapp_lock);
> > -
> >  	/*
> >  	 * Yet another marvel of the architecture. If using the
> >  	 * its_list "feature", we need to make sure that all ITSs
> > @@ -3808,7 +3802,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
> >  	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> >  	unsigned int from, cpu = nr_cpu_ids;
> >  	struct cpumask *table_mask;
> > -	unsigned long flags;
> > +	unsigned long flags, vmapp_flags;
> >  
> >  	/*
> >  	 * Changing affinity is mega expensive, so let's be as lazy as
> > @@ -3822,7 +3816,14 @@ static int its_vpe_set_affinity(struct irq_data *d,
> >  	 * protect us, and that we must ensure nobody samples vpe->col_idx
> >  	 * during the update, hence the lock below which must also be
> >  	 * taken on any vLPI handling path that evaluates vpe->col_idx.
> > +	 *
> > +	 * Finally, we must protect ourselves against concurrent
> > +	 * updates of the mapping state on this VM should the ITS list
> > +	 * be in use.
> >  	 */
> > +	if (its_list_map)
> > +		raw_spin_lock_irqsave(&vpe->its_vm->vmapp_lock, vmapp_flags);
> > +
> >  	from = vpe_to_cpuid_lock(vpe, &flags);
> >  	table_mask = gic_data_rdist_cpu(from)->vpe_table_mask;
> >  
> > @@ -3852,6 +3853,9 @@ static int its_vpe_set_affinity(struct irq_data *d,
> >  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
> >  	vpe_to_cpuid_unlock(vpe, flags);
> >  
> > +	if (its_list_map)
> > +		raw_spin_unlock_irqrestore(&vpe->its_vm->vmapp_lock, vmapp_flags);
> > +
> >  	return IRQ_SET_MASK_OK_DONE;
> >  }
> >
> 
> Hi Marc，
> 
> We add above code to do test again. Now it is OK.

Great, thanks for giving it a go. I have just posted an actual patch
(with the exact same change) at [1]. It would be good if you could
give it a Tested-by: tag.

Thanks,

	M.

[1] https://lore.kernel.org/r/20240723175203.3193882-1-maz@kernel.org

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] irqchip/gic-v4: Make sure a VPE is locked when VMAPP is issued

[Zhou Wang]

On 2024/7/24 1:56, Marc Zyngier wrote:
> On Tue, 23 Jul 2024 02:51:32 +0100,
> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>
>> On 2024/7/19 19:31, Marc Zyngier wrote:
>>> On Fri, 19 Jul 2024 10:42:02 +0100,
>>> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>>>
>>>> On 2024/7/5 17:31, Marc Zyngier wrote:
>>>>> In order to make sure that vpe->col_idx is correctly sampled
>>>>> when a VMAPP command is issued, we must hold the lock for the
>>>>> VPE. This is now possible since the introduction of the per-VM
>>>>> vmapp_lock, which can be taken before vpe_lock in the locking
>>>>> order.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  drivers/irqchip/irq-gic-v3-its.c | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>>> index b52d60097cad5..951ec140bcea2 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>>> @@ -1810,7 +1810,9 @@ static void its_map_vm(struct its_node *its, struct its_vm *vm)
>>>>>  		for (i = 0; i < vm->nr_vpes; i++) {
>>>>>  			struct its_vpe *vpe = vm->vpes[i];
>>>>>  
>>>>> -			its_send_vmapp(its, vpe, true);
>>>>> +			scoped_guard(raw_spinlock, &vpe->vpe_lock)
>>>>> +				its_send_vmapp(its, vpe, true);
>>>>> +
>>>>>  			its_send_vinvall(its, vpe);
>>>>>  		}
>>>>>  	}
>>>>> @@ -1827,8 +1829,10 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
>>>>>  	if (!--vm->vlpi_count[its->list_nr]) {
>>>>>  		int i;
>>>>>  
>>>>> -		for (i = 0; i < vm->nr_vpes; i++)
>>>>> +		for (i = 0; i < vm->nr_vpes; i++) {
>>>>> +			guard(raw_spinlock)(&vm->vpes[i]->vpe_lock);
>>>>>  			its_send_vmapp(its, vm->vpes[i], false);
>>>>> +		}
>>>>>  	}
>>>>>  }
>>>>>  
>>>>
>>>> Hi Marc,
>>>>
>>>> It looks like there is ABBA deadlock after applying this series:
>>>>
>>>> In its_map_vm: vmapp_lock -> vpe_lock
>>>> In its_vpe_set_affinity: vpe_to_cpuid_lock(vpe_lock) -> its_send_vmovp(vmapp_lock)
>>>>
>>>> Any idea about this?
>>>
>>> Hmmm, well spotted. That's an annoying one.
>>>
>>> Can you give the below hack a go? I've only lightly tested it, as my
>>> D05 box is on its last leg (it is literally falling apart) and I don't
>>> have any other GICv4.x box to test on.
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 951ec140bcea2..b88c6011c8771 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -1328,12 +1328,6 @@ static void its_send_vmovp(struct its_vpe *vpe)
>>>  		return;
>>>  	}
>>>  
>>> -	/*
>>> -	 * Protect against concurrent updates of the mapping state on
>>> -	 * individual VMs.
>>> -	 */
>>> -	guard(raw_spinlock_irqsave)(&vpe->its_vm->vmapp_lock);
>>> -
>>>  	/*
>>>  	 * Yet another marvel of the architecture. If using the
>>>  	 * its_list "feature", we need to make sure that all ITSs
>>> @@ -3808,7 +3802,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
>>>  	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
>>>  	unsigned int from, cpu = nr_cpu_ids;
>>>  	struct cpumask *table_mask;
>>> -	unsigned long flags;
>>> +	unsigned long flags, vmapp_flags;
>>>  
>>>  	/*
>>>  	 * Changing affinity is mega expensive, so let's be as lazy as
>>> @@ -3822,7 +3816,14 @@ static int its_vpe_set_affinity(struct irq_data *d,
>>>  	 * protect us, and that we must ensure nobody samples vpe->col_idx
>>>  	 * during the update, hence the lock below which must also be
>>>  	 * taken on any vLPI handling path that evaluates vpe->col_idx.
>>> +	 *
>>> +	 * Finally, we must protect ourselves against concurrent
>>> +	 * updates of the mapping state on this VM should the ITS list
>>> +	 * be in use.
>>>  	 */
>>> +	if (its_list_map)
>>> +		raw_spin_lock_irqsave(&vpe->its_vm->vmapp_lock, vmapp_flags);
>>> +
>>>  	from = vpe_to_cpuid_lock(vpe, &flags);
>>>  	table_mask = gic_data_rdist_cpu(from)->vpe_table_mask;
>>>  
>>> @@ -3852,6 +3853,9 @@ static int its_vpe_set_affinity(struct irq_data *d,
>>>  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>>>  	vpe_to_cpuid_unlock(vpe, flags);
>>>  
>>> +	if (its_list_map)
>>> +		raw_spin_unlock_irqrestore(&vpe->its_vm->vmapp_lock, vmapp_flags);
>>> +
>>>  	return IRQ_SET_MASK_OK_DONE;
>>>  }
>>>
>>
>> Hi Marc，
>>
>> We add above code to do test again. Now it is OK.
> 
> Great, thanks for giving it a go. I have just posted an actual patch
> (with the exact same change) at [1]. It would be good if you could
> give it a Tested-by: tag.

Sure, I will give it a Tested-by.

Thanks,
Zhou

> 
> Thanks,
> 
> 	M.
> 
> [1] https://lore.kernel.org/r/20240723175203.3193882-1-maz@kernel.org
>