Re: [Xen-devel] [PATCH v2 07/12] x86/IRQ: fix locking around vector management

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 08, 2019 at 07:10:59AM -0600, Jan Beulich wrote:
> All of __{assign,bind,clear}_irq_vector() manipulate struct irq_desc
> fields, and hence ought to be called with the descriptor lock held in
> addition to vector_lock. This is currently the case for only
> set_desc_affinity() (in the common case) and destroy_irq(), which also
> clarifies what the nesting behavior between the locks has to be.
> Reflect the new expectation by having these functions all take a
> descriptor as parameter instead of an interrupt number.
> 
> Also take care of the two special cases of calls to set_desc_affinity():
> set_ioapic_affinity_irq() and VT-d's dma_msi_set_affinity() get called
> directly as well, and in these cases the descriptor locks hadn't got
> acquired till now. For set_ioapic_affinity_irq() this means acquiring /
> releasing of the IO-APIC lock can be plain spin_{,un}lock() then.
> 
> Drop one of the two leading underscores from all three functions at
> the same time.
> 
> There's one case left where descriptors get manipulated with just
> vector_lock held: setup_vector_irq() assumes its caller to acquire
> vector_lock, and hence can't itself acquire the descriptor locks (wrong
> lock order). I don't currently see how to address this.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2134,11 +2134,16 @@ static void adjust_irq_affinity(struct a
>      unsigned int node = rhsa ? pxm_to_node(rhsa->proximity_domain)
>                               : NUMA_NO_NODE;
>      const cpumask_t *cpumask = &cpu_online_map;
> +    struct irq_desc *desc;
>  
>      if ( node < MAX_NUMNODES && node_online(node) &&
>           cpumask_intersects(&node_to_cpumask(node), cpumask) )
>          cpumask = &node_to_cpumask(node);
> -    dma_msi_set_affinity(irq_to_desc(drhd->iommu->msi.irq), cpumask);
> +
> +    desc = irq_to_desc(drhd->iommu->msi.irq);
> +    spin_lock_irq(&desc->lock);

I would use the irqsave/irqrestore variants here for extra safety.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel