Re: [RFC PATCH 07/10] xen: pci: add per-device locking

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Stefano,

Stefano Stabellini <sstabellini@xxxxxxxxxx> writes:

> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
>> Spinlock in struct pci_device will be used to protect access to device
>> itself. Right now it is used mostly by MSI code.
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> There are 2 instances of:
>
>     BUG_ON(list_empty(&dev->msi_list));
>
> in xen/arch/x86/msi.c:__pci_disable_msi and
> xen/arch/x86/msi.c:__pci_disable_msix which are not protected by
> pcidev_lock. However list_empty needs to be protected. (pci_disable_msi
> can also be called from xen/arch/x86/irq.c where it is not surrounded by
> pcidev_lock.)

I checked all call paths. pci_disable_msi() is called from three places in
xen/arch/x86/irq.c. As I can see, all three are "protected" with
ASSERT(pcidevs_locked()), or am I missing something?

>
> Given that they are BUG_ON, I wonder if we could remove them instead of
> adding locks there. It would make things simpler.

Well, I will be happy to remove them, if there are no objections.

>
>
>> ---
>>  xen/arch/x86/hvm/vmsi.c       |  6 +++++-
>>  xen/arch/x86/msi.c            | 16 ++++++++++++++++
>>  xen/drivers/passthrough/msi.c |  8 +++++++-
>>  xen/drivers/passthrough/pci.c |  2 ++
>>  xen/include/xen/pci.h         | 12 ++++++++++++
>>  5 files changed, 42 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
>> index 7fb1075673..c9e5f279c5 100644
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -203,10 +203,14 @@ static struct msi_desc *msixtbl_addr_to_desc(
>>  
>>      nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE;
>>  
>> +    pcidev_lock(entry->pdev);
>>      list_for_each_entry( desc, &entry->pdev->msi_list, list )
>>          if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX &&
>> -             desc->msi_attrib.entry_nr == nr_entry )
>> +             desc->msi_attrib.entry_nr == nr_entry ) {
>> +        pcidev_unlock(entry->pdev);
>
> code style
>
>
>>              return desc;
>> +    }
>> +    pcidev_unlock(entry->pdev);
>>  
>>      return NULL;
>>  }
>> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
>> index bccaccb98b..6b62c4f452 100644
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -389,6 +389,7 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool 
>> host, bool guest)
>>      default:
>>          return 0;
>>      }
>> +
>
> spurious change
>
>
>>      entry->msi_attrib.host_masked = host;
>>      entry->msi_attrib.guest_masked = guest;
>>  
>> @@ -585,12 +586,17 @@ static struct msi_desc *find_msi_entry(struct pci_dev 
>> *dev,
>>  {
>>      struct msi_desc *entry;
>>  
>> +    pcidev_lock(dev);
>>      list_for_each_entry( entry, &dev->msi_list, list )
>>      {
>>          if ( entry->msi_attrib.type == cap_id &&
>>               (irq == -1 || entry->irq == irq) )
>> +    {
>> +        pcidev_unlock(dev);
>>              return entry;
>> +    }
>>      }
>> +    pcidev_unlock(dev);
>>  
>>      return NULL;
>>  }
>> @@ -661,7 +667,9 @@ static int msi_capability_init(struct pci_dev *dev,
>>          maskbits |= ~(uint32_t)0 >> (32 - dev->msi_maxvec);
>>          pci_conf_write32(dev->sbdf, mpos, maskbits);
>>      }
>> +    pcidev_lock(dev);
>>      list_add_tail(&entry->list, &dev->msi_list);
>> +    pcidev_unlock(dev);
>>  
>>      *desc = entry;
>>      /* Restore the original MSI enabled bits  */
>> @@ -946,7 +954,9 @@ static int msix_capability_init(struct pci_dev *dev,
>>  
>>      pcidev_get(dev);
>>  
>> +    pcidev_lock(dev);
>>          list_add_tail(&entry->list, &dev->msi_list);
>> +    pcidev_unlock(dev);
>>          *desc = entry;
>>      }
>>  
>> @@ -1231,11 +1241,13 @@ static void msi_free_irqs(struct pci_dev* dev)
>>  {
>>      struct msi_desc *entry, *tmp;
>>  
>> +    pcidev_lock(dev);
>>      list_for_each_entry_safe( entry, tmp, &dev->msi_list, list )
>>      {
>>          pci_disable_msi(entry);
>>          msi_free_irq(entry);
>>      }
>> +    pcidev_unlock(dev);
>>  }
>>  
>>  void pci_cleanup_msi(struct pci_dev *pdev)
>> @@ -1354,6 +1366,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>>      if ( ret )
>>          return ret;
>>  
>> +    pcidev_lock(pdev);
>>      list_for_each_entry_safe( entry, tmp, &pdev->msi_list, list )
>>      {
>>          unsigned int i = 0, nr = 1;
>> @@ -1371,6 +1384,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>>              dprintk(XENLOG_ERR, "Restore MSI for %pp entry %u not set?\n",
>>                      &pdev->sbdf, i);
>>              spin_unlock_irqrestore(&desc->lock, flags);
>> +        pcidev_unlock(pdev);
>>              if ( type == PCI_CAP_ID_MSIX )
>>                  pci_conf_write16(pdev->sbdf, msix_control_reg(pos),
>>                                   control & ~PCI_MSIX_FLAGS_ENABLE);
>> @@ -1393,6 +1407,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>>              if ( unlikely(!memory_decoded(pdev)) )
>>              {
>>                  spin_unlock_irqrestore(&desc->lock, flags);
>> +            pcidev_unlock(pdev);
>>                  pci_conf_write16(pdev->sbdf, msix_control_reg(pos),
>>                                   control & ~PCI_MSIX_FLAGS_ENABLE);
>>                  return -ENXIO;
>> @@ -1438,6 +1453,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>>          pci_conf_write16(pdev->sbdf, msix_control_reg(pos),
>>                           control | PCI_MSIX_FLAGS_ENABLE);
>>  
>> +    pcidev_unlock(pdev);
>>      return 0;
>>  }
>>  
>> diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c
>> index ce1a450f6f..98f4d2721a 100644
>> --- a/xen/drivers/passthrough/msi.c
>> +++ b/xen/drivers/passthrough/msi.c
>> @@ -22,6 +22,7 @@ int pdev_msi_init(struct pci_dev *pdev)
>>  {
>>      unsigned int pos;
>>  
>> +    pcidev_lock(pdev);
>>      INIT_LIST_HEAD(&pdev->msi_list);
>>  
>>      pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> @@ -41,7 +42,10 @@ int pdev_msi_init(struct pci_dev *pdev)
>>          uint16_t ctrl;
>>  
>>          if ( !msix )
>> -            return -ENOMEM;
>> +        {
>> +             pcidev_unlock(pdev);
>> +             return -ENOMEM;
>> +        }
>>  
>>          spin_lock_init(&msix->table_lock);
>>  
>> @@ -51,6 +55,8 @@ int pdev_msi_init(struct pci_dev *pdev)
>>          pdev->msix = msix;
>>      }
>>  
>> +    pcidev_unlock(pdev);
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index c8da80b981..c83397211b 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1383,7 +1383,9 @@ static int cf_check _dump_pci_devices(struct pci_seg 
>> *pseg, void *arg)
>>              printk("%pd", pdev->domain);
>>          printk(" - node %-3d refcnt %d", (pdev->node != NUMA_NO_NODE) ? 
>> pdev->node : -1,
>>                 atomic_read(&pdev->refcnt));
>> +        pcidev_lock(pdev);
>>          pdev_dump_msi(pdev);
>> +        pcidev_unlock(pdev);
>>          printk("\n");
>>      }
>>      spin_unlock(&pseg->alldevs_lock);
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index e71a180ef3..d0a7339d84 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -106,6 +106,8 @@ struct pci_dev {
>>      uint8_t msi_maxvec;
>>      uint8_t phantom_stride;
>>  
>> +    /* Device lock */
>> +    spinlock_t lock;
>>      nodeid_t node; /* NUMA node */
>>  
>>      /* Device to be quarantined, don't automatically re-assign to dom0 */
>> @@ -235,6 +237,16 @@ int msixtbl_pt_register(struct domain *, struct pirq *, 
>> uint64_t gtable);
>>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
>>  void msixtbl_pt_cleanup(struct domain *d);
>>  
>> +static inline void pcidev_lock(struct pci_dev *pdev)
>> +{
>> +    spin_lock(&pdev->lock);
>> +}
>> +
>> +static inline void pcidev_unlock(struct pci_dev *pdev)
>> +{
>> +    spin_unlock(&pdev->lock);
>> +}
>> +
>>  #ifdef CONFIG_HVM
>>  int arch_pci_clean_pirqs(struct domain *d);
>>  #else
>> -- 
>> 2.36.1
>>