Re: [PATCH v2 1/5] x86/pass-through: alter locking around pt_irq_{create,destroy}_bind()

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Wed, Sep 30, 2026 at 01:42:29PM +0200, Jan Beulich wrote:
> The questionable use of pcidevs_lock() there was discussed more than once.
> It really is pointless: The functions synchronize primarily via the per-
> domain event lock. They also may already be called with the global PCI
> devices lock not held: See hvm/vmsi.c:vpci_msi_update(),
> hvm/vmsi.c:vpci_msi_arch_update(), and hvm/vmsi.c:vpci_msi_disable().
> 
> It's merely msixtbl_pt_{,un}register() which require the global or per-
> domain PCI lock held. Switch to using the latter, which is also sufficient
> to acquire in read mode. While strictly speaking this is needed only for
> PT_IRQ_TYPE_MSI, keep it being unconditional. In vioapic_hwdom_map_gsi()
> also unlock earlier.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Replace locking rather than dropping it altogether.
> 
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -636,9 +636,9 @@ long arch_do_domctl(
>              ret = -EPERM;
>          else if ( is_iommu_enabled(d) )
>          {
> -            pcidevs_lock();
> +            read_lock(&d->pci_lock);
>              ret = pt_irq_create_bind(d, bind);
> -            pcidevs_unlock();
> +            read_unlock(&d->pci_lock);
>  
>              if ( ret < 0 )
>                  printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for 
> %pd\n",
> @@ -670,9 +670,9 @@ long arch_do_domctl(
>              ret = -EPERM;
>          else if ( is_iommu_enabled(d) )
>          {
> -            pcidevs_lock();
> +            read_lock(&d->pci_lock);
>              ret = pt_irq_destroy_bind(d, bind);
> -            pcidevs_unlock();
> +            read_unlock(&d->pci_lock);
>  
>              if ( ret < 0 )
>                  printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for 
> %pd\n",
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -197,8 +197,9 @@ static int vioapic_hwdom_map_gsi(unsigne
>          return ret;
>      }
>  
> -    pcidevs_lock();
> +    read_lock(&currd->pci_lock);
>      ret = pt_irq_create_bind(currd, &pt_irq_bind);
> +    read_unlock(&currd->pci_lock);

I think you need to extend the ->pci_lock'ed region so it also covers
unmap_domain_pirq(), otherwise the ASSERT_PDEV_LIST_IS_READ_LOCKED()
in there will trigger?  IE: the read_unlock() needs to be where the
pcidevs_unlock() is?

Thanks, Roger.