Re: [PATCH] irqchip/gic-v4: Fix ordering between vmapp and vpe locks

[Zhou Wang <wangzhou1@hisilicon.com>]

On 2024/7/24 1:52, Marc Zyngier wrote:
> The (recently established) lock ordering mandates that the per-VM
> vmapp_lock is acquired before we take the per-VPE lock.
> 
> As it turns out, its_vpe_set_affinity() takes the VPE lock, and
> then calls into its_send_vmovp(), which itself takes the vmapp
> lock. Obviously, this isn't what we want.
> 
> As its_send_vmovp() is only called from its_vpe_set_affinity(),
> hoist the vmapp locking from the former into the latter, restoring
> the expected order.
> 
> Fixes: f0eb154c39471 ("irqchip/gic-v4: Substitute vmovp_lock for a per-VM lock")
> Reported-by: Zhou Wang <wangzhou1@hisilicon.com>

Hi Marc,

Tested-by: Zhou Wang <wangzhou1@hisilicon.com>

Thanks,
Zhou

> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> 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;
>  }
>