[PATCH v2 0/4] vpci: first series in preparation for vpci on ARM

[PATCH v2 0/4] vpci: first series in preparation for vpci on ARM

[Volodymyr Babchuk]

Hello all,

While Oleksandr Andrushchenko is busy with more important matters, I
want to continue his work on preparing different Xen subsystems to
support PCI on ARM platform.

This patch series are mostly focused on next take of PCI locking
rework. It introduces reentrant read/write locking mechanism for
pcidevs, which will be immediatelly used to decouple readers and
writers in the PCI code. There are also some minor changes to a
related vpci code.

Oleksandr Andrushchenko (3):
  pci: add rwlock to pcidevs_lock machinery
  vpci: restrict unhandled read/write operations for guests
  vpci: use pcidevs locking to protect MMIO handlers

Volodymyr Babchuk (1):
  vpci: include xen/vmap.h to fix build on ARM

 xen/arch/x86/hvm/vmsi.c       | 46 +++++++++++++++++--------
 xen/arch/x86/irq.c            |  8 ++---
 xen/arch/x86/msi.c            | 16 ++++-----
 xen/drivers/passthrough/pci.c | 65 +++++++++++++++++++++++++++++++----
 xen/drivers/vpci/header.c     | 24 +++++++++++--
 xen/drivers/vpci/msi.c        | 21 +++++++----
 xen/drivers/vpci/msix.c       | 55 +++++++++++++++++++++++++----
 xen/drivers/vpci/vpci.c       | 29 +++++++++++++---
 xen/include/xen/pci.h         | 11 +++++-
 xen/include/xen/vpci.h        |  2 +-
 10 files changed, 223 insertions(+), 54 deletions(-)

-- 
2.36.1

[PATCH v2 1/4] pci: add rwlock to pcidevs_lock machinery

[Volodymyr Babchuk]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Currently pcidevs lock is a global recursive spinlock which is fine for
the existing use cases. It is used to both protect pdev instances
themselves from being removed while in use and to make sure the update
of the relevant pdev properties is synchronized.

Moving towards vPCI is used for guests this becomes problematic in terms
of lock contention. For example, during vpci_{read|write} the access to
pdev must be protected to prevent pdev disappearing under our feet.
This needs to be done with the help of pcidevs_{lock|unlock}.
On the other hand it is highly undesirable to lock all other pdev accesses
which only use pdevs in read mode, e.g. those which do not remove or
add pdevs.

For the above reasons introduce a read/write lock which will help
preventing locking contentions between pdev readers and writers. This
read/write lock replaces current recursive spinlock.

By default all existing code uses pcidevs_lock() which takes write
lock. This ensures that all existing locking logic stays the same.

To justify this change, replace locks in (V)MSI code with read
locks. This code is a perfect target, as (V)MSI code only requires
that pdevs will not disappear during handling, while (V)MSI state
is protected by own locks.

This is in preparation for vPCI to be used for guests.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---
Since v1:
- user per CPU variable to track recursive readers and writers
- use read locks in vmsi code
---
 xen/arch/x86/hvm/vmsi.c       | 22 ++++++------
 xen/arch/x86/irq.c            |  8 ++---
 xen/arch/x86/msi.c            | 16 ++++-----
 xen/drivers/passthrough/pci.c | 65 +++++++++++++++++++++++++++++++----
 xen/include/xen/pci.h         | 10 +++++-
 5 files changed, 91 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index d4a8c953e2..c1ede676d0 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -466,7 +466,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
     struct msixtbl_entry *entry, *new_entry;
     int r = -EINVAL;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     ASSERT(spin_is_locked(&d->event_lock));
 
     if ( !msixtbl_initialised(d) )
@@ -536,7 +536,7 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
     struct pci_dev *pdev;
     struct msixtbl_entry *entry;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     ASSERT(spin_is_locked(&d->event_lock));
 
     if ( !msixtbl_initialised(d) )
@@ -682,7 +682,7 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data,
 {
     unsigned int i;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
 
     if ( (address & MSI_ADDR_BASE_MASK) != MSI_ADDR_HEADER )
     {
@@ -724,7 +724,7 @@ void vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev)
 
     ASSERT(msi->arch.pirq != INVALID_PIRQ);
 
-    pcidevs_lock();
+    pcidevs_read_lock();
     for ( i = 0; i < msi->vectors && msi->arch.bound; i++ )
     {
         struct xen_domctl_bind_pt_irq unbind = {
@@ -743,7 +743,7 @@ void vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev)
 
     msi->arch.bound = !vpci_msi_update(pdev, msi->data, msi->address,
                                        msi->vectors, msi->arch.pirq, msi->mask);
-    pcidevs_unlock();
+    pcidevs_read_unlock();
 }
 
 static int vpci_msi_enable(const struct pci_dev *pdev, unsigned int nr,
@@ -783,10 +783,10 @@ int vpci_msi_arch_enable(struct vpci_msi *msi, const struct pci_dev *pdev,
         return rc;
     msi->arch.pirq = rc;
 
-    pcidevs_lock();
+    pcidevs_read_lock();
     msi->arch.bound = !vpci_msi_update(pdev, msi->data, msi->address, vectors,
                                        msi->arch.pirq, msi->mask);
-    pcidevs_unlock();
+    pcidevs_read_unlock();
 
     return 0;
 }
@@ -798,7 +798,7 @@ static void vpci_msi_disable(const struct pci_dev *pdev, int pirq,
 
     ASSERT(pirq != INVALID_PIRQ);
 
-    pcidevs_lock();
+    pcidevs_read_lock();
     for ( i = 0; i < nr && bound; i++ )
     {
         struct xen_domctl_bind_pt_irq bind = {
@@ -814,7 +814,7 @@ static void vpci_msi_disable(const struct pci_dev *pdev, int pirq,
     spin_lock(&pdev->domain->event_lock);
     unmap_domain_pirq(pdev->domain, pirq);
     spin_unlock(&pdev->domain->event_lock);
-    pcidevs_unlock();
+    pcidevs_read_unlock();
 }
 
 void vpci_msi_arch_disable(struct vpci_msi *msi, const struct pci_dev *pdev)
@@ -861,7 +861,7 @@ int vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,
 
     entry->arch.pirq = rc;
 
-    pcidevs_lock();
+    pcidevs_read_lock();
     rc = vpci_msi_update(pdev, entry->data, entry->addr, 1, entry->arch.pirq,
                          entry->masked);
     if ( rc )
@@ -869,7 +869,7 @@ int vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,
         vpci_msi_disable(pdev, entry->arch.pirq, 1, false);
         entry->arch.pirq = INVALID_PIRQ;
     }
-    pcidevs_unlock();
+    pcidevs_read_unlock();
 
     return rc;
 }
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index de30ee7779..7b4832ffb1 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2156,7 +2156,7 @@ int map_domain_pirq(
         struct pci_dev *pdev;
         unsigned int nr = 0;
 
-        ASSERT(pcidevs_locked());
+        ASSERT(pcidevs_read_locked());
 
         ret = -ENODEV;
         if ( !cpu_has_apic )
@@ -2313,7 +2313,7 @@ int unmap_domain_pirq(struct domain *d, int pirq)
     if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
         return -EINVAL;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     ASSERT(spin_is_locked(&d->event_lock));
 
     info = pirq_info(d, pirq);
@@ -2907,7 +2907,7 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
 
     msi->irq = irq;
 
-    pcidevs_lock();
+    pcidevs_read_lock();
     /* Verify or get pirq. */
     spin_lock(&d->event_lock);
     pirq = allocate_pirq(d, index, *pirq_p, irq, type, &msi->entry_nr);
@@ -2923,7 +2923,7 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
 
  done:
     spin_unlock(&d->event_lock);
-    pcidevs_unlock();
+    pcidevs_read_unlock();
     if ( ret )
     {
         switch ( type )
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 6be81e6c3b..6ce7b5523a 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -613,7 +613,7 @@ static int msi_capability_init(struct pci_dev *dev,
     u8 slot = PCI_SLOT(dev->devfn);
     u8 func = PCI_FUNC(dev->devfn);
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
     if ( !pos )
         return -ENODEV;
@@ -782,7 +782,7 @@ static int msix_capability_init(struct pci_dev *dev,
     if ( !pos )
         return -ENODEV;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
 
     control = pci_conf_read16(dev->sbdf, msix_control_reg(pos));
     /*
@@ -999,7 +999,7 @@ static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
     struct pci_dev *pdev;
     struct msi_desc *old_desc;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
     if ( !pdev )
         return -ENODEV;
@@ -1054,7 +1054,7 @@ static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc)
     struct pci_dev *pdev;
     struct msi_desc *old_desc;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
     if ( !pdev || !pdev->msix )
         return -ENODEV;
@@ -1145,7 +1145,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool off)
     if ( !use_msi )
         return 0;
 
-    pcidevs_lock();
+    pcidevs_read_lock();
     pdev = pci_get_pdev(seg, bus, devfn);
     if ( !pdev )
         rc = -ENODEV;
@@ -1158,7 +1158,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool off)
     }
     else
         rc = msix_capability_init(pdev, NULL, NULL);
-    pcidevs_unlock();
+    pcidevs_read_unlock();
 
     return rc;
 }
@@ -1169,7 +1169,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool off)
  */
 int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
 {
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
 
     if ( !use_msi )
         return -EPERM;
@@ -1305,7 +1305,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
     unsigned int type = 0, pos = 0;
     u16 control = 0;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
 
     if ( !use_msi )
         return -EOPNOTSUPP;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 938821e593..f93922acc8 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -50,21 +50,74 @@ struct pci_seg {
     } bus2bridge[MAX_BUSES];
 };
 
-static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_RWLOCK(_pcidevs_rwlock);
+static DEFINE_PER_CPU(unsigned int, pcidevs_read_cnt);
+static DEFINE_PER_CPU(unsigned int, pcidevs_write_cnt);
 
 void pcidevs_lock(void)
 {
-    spin_lock_recursive(&_pcidevs_lock);
+    pcidevs_write_lock();
 }
 
 void pcidevs_unlock(void)
 {
-    spin_unlock_recursive(&_pcidevs_lock);
+    pcidevs_write_unlock();
 }
 
-bool_t pcidevs_locked(void)
+bool pcidevs_locked(void)
 {
-    return !!spin_is_locked(&_pcidevs_lock);
+    return pcidevs_write_locked();
+}
+
+void pcidevs_read_lock(void)
+{
+    if ( this_cpu(pcidevs_read_cnt)++ == 0 )
+        read_lock(&_pcidevs_rwlock);
+}
+
+int pcidevs_read_trylock(void)
+{
+    int ret = 1;
+
+    if ( this_cpu(pcidevs_read_cnt) == 0 )
+        ret = read_trylock(&_pcidevs_rwlock);
+    if ( ret )
+        this_cpu(pcidevs_read_cnt)++;
+
+    return ret;
+}
+
+void pcidevs_read_unlock(void)
+{
+    ASSERT(this_cpu(pcidevs_read_cnt));
+
+    if ( --this_cpu(pcidevs_read_cnt) == 0 )
+        read_unlock(&_pcidevs_rwlock);
+}
+
+bool pcidevs_read_locked(void)
+{
+    /* Write lock implies read lock */
+    return this_cpu(pcidevs_read_cnt) || this_cpu(pcidevs_write_cnt);
+}
+
+void pcidevs_write_lock(void)
+{
+    if ( this_cpu(pcidevs_write_cnt)++ == 0 )
+        write_lock(&_pcidevs_rwlock);
+}
+
+void pcidevs_write_unlock(void)
+{
+    ASSERT(this_cpu(pcidevs_write_cnt));
+
+    if ( --this_cpu(pcidevs_write_cnt) == 0 )
+        write_unlock(&_pcidevs_rwlock);
+}
+
+bool pcidevs_write_locked(void)
+{
+    return rw_is_write_locked(&_pcidevs_rwlock);
 }
 
 static struct radix_tree_root pci_segments;
@@ -581,7 +634,7 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
     struct pci_seg *pseg = get_pseg(seg);
     struct pci_dev *pdev = NULL;
 
-    ASSERT(pcidevs_locked());
+    ASSERT(pcidevs_read_locked());
     ASSERT(seg != -1 || bus == -1);
     ASSERT(bus != -1 || devfn == -1);
 
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index f34368643c..052b2ddf9f 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -158,7 +158,15 @@ struct pci_dev {
 
 void pcidevs_lock(void);
 void pcidevs_unlock(void);
-bool_t __must_check pcidevs_locked(void);
+bool __must_check pcidevs_locked(void);
+
+void pcidevs_read_lock(void);
+void pcidevs_read_unlock(void);
+bool __must_check pcidevs_read_locked(void);
+
+void pcidevs_write_lock(void);
+void pcidevs_write_unlock(void);
+bool __must_check pcidevs_write_locked(void);
 
 bool_t pci_known_segment(u16 seg);
 bool_t pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
-- 
2.36.1

Re: [PATCH v2 1/4] pci: add rwlock to pcidevs_lock machinery

[Wei Chen]

Hi Volodymyr,

On 2022/7/19 5:15, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

>   
>       if ( !use_msi )
>           return -EOPNOTSUPP;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 938821e593..f93922acc8 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -50,21 +50,74 @@ struct pci_seg {
>       } bus2bridge[MAX_BUSES];
>   };
>   
> -static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
> +static DEFINE_RWLOCK(_pcidevs_rwlock);
> +static DEFINE_PER_CPU(unsigned int, pcidevs_read_cnt);
> +static DEFINE_PER_CPU(unsigned int, pcidevs_write_cnt);
>   
>   void pcidevs_lock(void)
>   {
> -    spin_lock_recursive(&_pcidevs_lock);
> +    pcidevs_write_lock();
>   }
>   
>   void pcidevs_unlock(void)
>   {
> -    spin_unlock_recursive(&_pcidevs_lock);
> +    pcidevs_write_unlock();
>   }
>   
> -bool_t pcidevs_locked(void)
> +bool pcidevs_locked(void)
>   {
> -    return !!spin_is_locked(&_pcidevs_lock);
> +    return pcidevs_write_locked();
> +}
> +
> +void pcidevs_read_lock(void)
> +{
> +    if ( this_cpu(pcidevs_read_cnt)++ == 0 )
> +        read_lock(&_pcidevs_rwlock);
> +}
> +

For my understanding, if pcidevs_read_cnt > 0, pcidevs_read_lock
will be unblocked.I am not sure if this behavior is consistent with
the original lock? According to my understanding, the original spinlock 
should be blocked all the time, if the lock is not acquired. Maybe
I have misunderstanding something, I am not very familiar with PCI
subsystem.

Cheers,
Wei Chen

> +int pcidevs_read_trylock(void)
> +{
> +    int ret = 1;
> +
> +    if ( this_cpu(pcidevs_read_cnt) == 0 )
> +        ret = read_trylock(&_pcidevs_rwlock);
> +    if ( ret )
> +        this_cpu(pcidevs_read_cnt)++;
> +
> +    return ret;
> +}
> +
> +void pcidevs_read_unlock(void)
> +{
> +    ASSERT(this_cpu(pcidevs_read_cnt));
> +
> +    if ( --this_cpu(pcidevs_read_cnt) == 0 )
> +        read_unlock(&_pcidevs_rwlock);
> +}
> +
> +bool pcidevs_read_locked(void)
> +{
> +    /* Write lock implies read lock */
> +    return this_cpu(pcidevs_read_cnt) || this_cpu(pcidevs_write_cnt);
> +}
> +
> +void pcidevs_write_lock(void)
> +{
> +    if ( this_cpu(pcidevs_write_cnt)++ == 0 )
> +        write_lock(&_pcidevs_rwlock);
> +}
> +
> +void pcidevs_write_unlock(void)
> +{
> +    ASSERT(this_cpu(pcidevs_write_cnt));
> +
> +    if ( --this_cpu(pcidevs_write_cnt) == 0 )
> +        write_unlock(&_pcidevs_rwlock);
> +}
> +
> +bool pcidevs_write_locked(void)
> +{
> +    return rw_is_write_locked(&_pcidevs_rwlock);
>   }
>   
>   static struct radix_tree_root pci_segments;
> @@ -581,7 +634,7 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
>       struct pci_seg *pseg = get_pseg(seg);
>       struct pci_dev *pdev = NULL;
>   
> -    ASSERT(pcidevs_locked());
> +    ASSERT(pcidevs_read_locked());
>       ASSERT(seg != -1 || bus == -1);
>       ASSERT(bus != -1 || devfn == -1);
>   
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index f34368643c..052b2ddf9f 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -158,7 +158,15 @@ struct pci_dev {
>   
>   void pcidevs_lock(void);
>   void pcidevs_unlock(void);
> -bool_t __must_check pcidevs_locked(void);
> +bool __must_check pcidevs_locked(void);
> +
> +void pcidevs_read_lock(void);
> +void pcidevs_read_unlock(void);
> +bool __must_check pcidevs_read_locked(void);
> +
> +void pcidevs_write_lock(void);
> +void pcidevs_write_unlock(void);
> +bool __must_check pcidevs_write_locked(void);
>   
>   bool_t pci_known_segment(u16 seg);
>   bool_t pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);

Re: [PATCH v2 1/4] pci: add rwlock to pcidevs_lock machinery

[Volodymyr Babchuk]

Hello Wei,

Wei Chen <Wei.Chen@arm.com> writes:

> Hi Volodymyr,
>
> On 2022/7/19 5:15, Volodymyr Babchuk wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
>>         if ( !use_msi )
>>           return -EOPNOTSUPP;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 938821e593..f93922acc8 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -50,21 +50,74 @@ struct pci_seg {
>>       } bus2bridge[MAX_BUSES];
>>   };
>>   -static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
>> +static DEFINE_RWLOCK(_pcidevs_rwlock);
>> +static DEFINE_PER_CPU(unsigned int, pcidevs_read_cnt);
>> +static DEFINE_PER_CPU(unsigned int, pcidevs_write_cnt);
>>     void pcidevs_lock(void)
>>   {
>> -    spin_lock_recursive(&_pcidevs_lock);
>> +    pcidevs_write_lock();
>>   }
>>     void pcidevs_unlock(void)
>>   {
>> -    spin_unlock_recursive(&_pcidevs_lock);
>> +    pcidevs_write_unlock();
>>   }
>>   -bool_t pcidevs_locked(void)
>> +bool pcidevs_locked(void)
>>   {
>> -    return !!spin_is_locked(&_pcidevs_lock);
>> +    return pcidevs_write_locked();
>> +}
>> +
>> +void pcidevs_read_lock(void)
>> +{
>> +    if ( this_cpu(pcidevs_read_cnt)++ == 0 )
>> +        read_lock(&_pcidevs_rwlock);
>> +}
>> +
>
> For my understanding, if pcidevs_read_cnt > 0, pcidevs_read_lock
> will be unblocked.I am not sure if this behavior is consistent with
> the original lock? According to my understanding, the original
> spinlock should be blocked all the time, if the lock is not
> acquired. Maybe

Original spinlock was recursive one. As read-write locks are
non-recursive in Xen, we need to implement some other mechanism to
support recursion. This code ensures that pCPU will not dead-lock itself
if it'll call pcidevs_read_lock() twice. Per-CPU counter ensures that
read_unlock() will be called only when pcidevs_read_unlock() calls is
balanced with pcidevs_read_lock()s.

> I have misunderstanding something, I am not very familiar with PCI
> subsystem.
>
> Cheers,
> Wei Chen

[...]

-- 
Volodymyr Babchuk at EPAM

[PATCH v2 2/4] vpci: restrict unhandled read/write operations for guests

[Volodymyr Babchuk]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

A guest would be able to read and write those registers which are not
emulated and have no respective vPCI handlers, so it will be possible
for it to access the hardware directly.
In order to prevent a guest from reads and writes from/to the unhandled
registers make sure only hardware domain can access the hardware directly
and restrict guests from doing so.

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

---
Since v6:
- do not use is_hwdom parameter for vpci_{read|write}_hw and use
  current->domain internally
- update commit message
New in v6
Moved into another series
---
 xen/drivers/vpci/vpci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 9fb3c05b2b..c7a40a2f41 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -215,6 +215,10 @@ static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg,
 {
     uint32_t data;
 
+    /* Guest domains are not allowed to read real hardware. */
+    if ( !is_hardware_domain(current->domain) )
+        return ~(uint32_t)0;
+
     switch ( size )
     {
     case 4:
@@ -255,9 +259,13 @@ static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg,
     return data;
 }
 
-static void vpci_write_hw(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
-                          uint32_t data)
+static void vpci_write_hw(pci_sbdf_t sbdf, unsigned int reg,
+                          unsigned int size, uint32_t data)
 {
+    /* Guest domains are not allowed to write real hardware. */
+    if ( !is_hardware_domain(current->domain) )
+        return;
+
     switch ( size )
     {
     case 4:
-- 
2.36.1

[PATCH v2 3/4] vpci: use pcidevs locking to protect MMIO handlers

[Volodymyr Babchuk]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

vPCI MMIO handlers are accessing pdevs without protecting this access
with pcidevs_{lock|unlock}. This is not a problem as of now as these
are only used by Dom0. But, towards vPCI is used also for guests, we need
to properly protect pdev and pdev->vpci from being removed while still
in use.

For that use pcidevs_read_{un}lock helpers.

This patch adds ASSERTs in the code to check that the rwlock is taken
and in appropriate mode. Some of such checks require changes to the
initialization of local variables which may be accessed before the
ASSERT checks the locking. For example see init_bars and mask_write.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---
Since v1:
- move pcidevs_read_{lock|unlock} into patch 1
---
 xen/arch/x86/hvm/vmsi.c   | 24 ++++++++++++++---
 xen/drivers/vpci/header.c | 24 +++++++++++++++--
 xen/drivers/vpci/msi.c    | 21 ++++++++++-----
 xen/drivers/vpci/msix.c   | 55 ++++++++++++++++++++++++++++++++++-----
 xen/drivers/vpci/vpci.c   | 16 +++++++++---
 xen/include/xen/pci.h     |  1 +
 xen/include/xen/vpci.h    |  2 +-
 7 files changed, 121 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index c1ede676d0..3f250f81a4 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -891,10 +891,16 @@ void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry)
     entry->arch.pirq = INVALID_PIRQ;
 }
 
-int vpci_msix_arch_print(const struct vpci_msix *msix)
+int vpci_msix_arch_print(const struct domain *d, const struct vpci_msix *msix)
 {
     unsigned int i;
 
+    /*
+     * FIXME: this is not immediately correct, as the lock can be grabbed
+     * by a different CPU. But this is better then nothing.
+     */
+    ASSERT(pcidevs_read_locked());
+
     for ( i = 0; i < msix->max_entries; i++ )
     {
         const struct vpci_msix_entry *entry = &msix->entries[i];
@@ -911,11 +917,23 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
         if ( i && !(i % 64) )
         {
             struct pci_dev *pdev = msix->pdev;
+            pci_sbdf_t sbdf = pdev->sbdf;
 
             spin_unlock(&msix->pdev->vpci->lock);
+            pcidevs_read_unlock();
+
+            /* NB: we still hold rcu_read_lock(&domlist_read_lock); here. */
             process_pending_softirqs();
-            /* NB: we assume that pdev cannot go away for an alive domain. */
-            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
+
+            if ( !pcidevs_read_trylock() )
+                return -EBUSY;
+            pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
+            /*
+             * FIXME: we may find a re-allocated pdev's copy here.
+             * Even occupying the same address as before. Do our best.
+             */
+            if ( !pdev || (pdev != msix->pdev) || !pdev->vpci ||
+                 !spin_trylock(&pdev->vpci->lock) )
                 return -EBUSY;
             if ( pdev->vpci->msix != msix )
             {
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index a1c928a0d2..e0461b1139 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -142,16 +142,19 @@ bool vpci_process_pending(struct vcpu *v)
         if ( rc == -ERESTART )
             return true;
 
+        pcidevs_read_lock();
         spin_lock(&v->vpci.pdev->vpci->lock);
         /* Disable memory decoding unconditionally on failure. */
         modify_decoding(v->vpci.pdev,
                         rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
                         !rc && v->vpci.rom_only);
         spin_unlock(&v->vpci.pdev->vpci->lock);
+        pcidevs_read_unlock();
 
         rangeset_destroy(v->vpci.mem);
         v->vpci.mem = NULL;
         if ( rc )
+        {
             /*
              * FIXME: in case of failure remove the device from the domain.
              * Note that there might still be leftover mappings. While this is
@@ -159,7 +162,10 @@ bool vpci_process_pending(struct vcpu *v)
              * killed in order to avoid leaking stale p2m mappings on
              * failure.
              */
+            pcidevs_write_lock();
             vpci_remove_device(v->vpci.pdev);
+            pcidevs_write_unlock();
+        }
     }
 
     return false;
@@ -172,7 +178,16 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
     int rc;
 
     while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) == -ERESTART )
+    {
+        /*
+         * It's safe to drop and re-acquire the lock in this context
+         * without risking pdev disappearing because devices cannot be
+         * removed until the initial domain has been started.
+         */
+        pcidevs_write_unlock();
         process_pending_softirqs();
+        pcidevs_write_lock();
+    }
     rangeset_destroy(mem);
     if ( !rc )
         modify_decoding(pdev, cmd, false);
@@ -450,10 +465,15 @@ static int cf_check init_bars(struct pci_dev *pdev)
     uint16_t cmd;
     uint64_t addr, size;
     unsigned int i, num_bars, rom_reg;
-    struct vpci_header *header = &pdev->vpci->header;
-    struct vpci_bar *bars = header->bars;
+    struct vpci_header *header;
+    struct vpci_bar *bars;
     int rc;
 
+    ASSERT(pcidevs_write_locked());
+
+    header = &pdev->vpci->header;
+    bars = header->bars;
+
     switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
     {
     case PCI_HEADER_TYPE_NORMAL:
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 8f2b59e61a..d864f740cf 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -184,12 +184,17 @@ static void cf_check mask_write(
 
 static int cf_check init_msi(struct pci_dev *pdev)
 {
-    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
-    unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
-                                           PCI_CAP_ID_MSI);
+    uint8_t slot, func;
+    unsigned int pos;
     uint16_t control;
     int ret;
 
+    ASSERT(pcidevs_write_locked());
+
+    slot = PCI_SLOT(pdev->devfn);
+    func = PCI_FUNC(pdev->devfn);
+    pos = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func, PCI_CAP_ID_MSI);
+
     if ( !pos )
         return 0;
 
@@ -277,6 +282,9 @@ void vpci_dump_msi(void)
 
         printk("vPCI MSI/MSI-X d%d\n", d->domain_id);
 
+        if ( !pcidevs_read_trylock() )
+            continue;
+
         for_each_pdev ( d, pdev )
         {
             const struct vpci_msi *msi;
@@ -310,7 +318,7 @@ void vpci_dump_msi(void)
                 printk("  entries: %u maskall: %d enabled: %d\n",
                        msix->max_entries, msix->masked, msix->enabled);
 
-                rc = vpci_msix_arch_print(msix);
+                rc = vpci_msix_arch_print(d, msix);
                 if ( rc )
                 {
                     /*
@@ -318,12 +326,13 @@ void vpci_dump_msi(void)
                      * holding the lock.
                      */
                     printk("unable to print all MSI-X entries: %d\n", rc);
-                    process_pending_softirqs();
-                    continue;
+                    goto pdev_done;
                 }
             }
 
             spin_unlock(&pdev->vpci->lock);
+ pdev_done:
+            pcidevs_read_unlock();
             process_pending_softirqs();
         }
     }
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index bea0cc7aed..35cc9280f4 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -144,9 +144,13 @@ static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr)
 
     list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
     {
-        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
+        const struct vpci_bar *bars;
         unsigned int i;
 
+        if ( !msix->pdev->vpci )
+            continue;
+
+        bars = msix->pdev->vpci->header.bars;
         for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
             if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
                  VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
@@ -158,7 +162,13 @@ static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr)
 
 static int cf_check msix_accept(struct vcpu *v, unsigned long addr)
 {
-    return !!msix_find(v->domain, addr);
+    int rc;
+
+    pcidevs_read_lock();
+    rc = !!msix_find(v->domain, addr);
+    pcidevs_read_unlock();
+
+    return rc;
 }
 
 static bool access_allowed(const struct pci_dev *pdev, unsigned long addr,
@@ -218,17 +228,26 @@ static int cf_check msix_read(
     struct vcpu *v, unsigned long addr, unsigned int len, unsigned long *data)
 {
     const struct domain *d = v->domain;
-    struct vpci_msix *msix = msix_find(d, addr);
+    struct vpci_msix *msix;
     const struct vpci_msix_entry *entry;
     unsigned int offset;
 
     *data = ~0ul;
 
+    pcidevs_read_lock();
+
+    msix = msix_find(d, addr);
     if ( !msix )
+    {
+        pcidevs_read_unlock();
         return X86EMUL_RETRY;
+    }
 
     if ( !access_allowed(msix->pdev, addr, len) )
+    {
+        pcidevs_read_unlock();
         return X86EMUL_OKAY;
+    }
 
     if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
     {
@@ -299,6 +318,7 @@ static int cf_check msix_read(
         break;
     }
     spin_unlock(&msix->pdev->vpci->lock);
+    pcidevs_read_unlock();
 
     return X86EMUL_OKAY;
 }
@@ -307,15 +327,24 @@ static int cf_check msix_write(
     struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
 {
     const struct domain *d = v->domain;
-    struct vpci_msix *msix = msix_find(d, addr);
+    struct vpci_msix *msix;
     struct vpci_msix_entry *entry;
     unsigned int offset;
 
+    pcidevs_read_lock();
+
+    msix = msix_find(d, addr);
     if ( !msix )
+    {
+        pcidevs_read_unlock();
         return X86EMUL_RETRY;
+    }
 
     if ( !access_allowed(msix->pdev, addr, len) )
+    {
+        pcidevs_read_unlock();
         return X86EMUL_OKAY;
+    }
 
     if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
     {
@@ -351,6 +380,7 @@ static int cf_check msix_write(
             break;
         }
 
+        pcidevs_read_unlock();
         return X86EMUL_OKAY;
     }
 
@@ -428,6 +458,7 @@ static int cf_check msix_write(
         break;
     }
     spin_unlock(&msix->pdev->vpci->lock);
+    pcidevs_read_unlock();
 
     return X86EMUL_OKAY;
 }
@@ -440,9 +471,13 @@ static const struct hvm_mmio_ops vpci_msix_table_ops = {
 
 int vpci_make_msix_hole(const struct pci_dev *pdev)
 {
-    struct domain *d = pdev->domain;
+    struct domain *d;
     unsigned int i;
 
+    ASSERT(pcidevs_read_locked());
+
+    d = pdev->domain;
+
     if ( !pdev->vpci->msix )
         return 0;
 
@@ -487,13 +522,19 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
 
 static int cf_check init_msix(struct pci_dev *pdev)
 {
-    struct domain *d = pdev->domain;
-    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
+    struct domain *d;
+    uint8_t slot, func;
     unsigned int msix_offset, i, max_entries;
     uint16_t control;
     struct vpci_msix *msix;
     int rc;
 
+    ASSERT(pcidevs_write_locked());
+
+    d = pdev->domain;
+    slot = PCI_SLOT(pdev->devfn);
+    func = PCI_FUNC(pdev->devfn);
+
     msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
                                       PCI_CAP_ID_MSIX);
     if ( !msix_offset )
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index c7a40a2f41..1559763479 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -37,7 +37,9 @@ extern vpci_register_init_t *const __end_vpci_array[];
 
 void vpci_remove_device(struct pci_dev *pdev)
 {
-    if ( !has_vpci(pdev->domain) )
+    ASSERT(pcidevs_write_locked());
+
+    if ( !has_vpci(pdev->domain) || !pdev->vpci )
         return;
 
     spin_lock(&pdev->vpci->lock);
@@ -332,10 +334,14 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
         return data;
     }
 
+    pcidevs_read_lock();
     /* Find the PCI dev matching the address. */
     pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
-    if ( !pdev )
+    if ( !pdev || (pdev && !pdev->vpci) )
+    {
+        pcidevs_read_unlock();
         return vpci_read_hw(sbdf, reg, size);
+    }
 
     spin_lock(&pdev->vpci->lock);
 
@@ -381,6 +387,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
         ASSERT(data_offset < size);
     }
     spin_unlock(&pdev->vpci->lock);
+    pcidevs_read_unlock();
 
     if ( data_offset < size )
     {
@@ -443,9 +450,11 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
      * Find the PCI dev matching the address.
      * Passthrough everything that's not trapped.
      */
+    pcidevs_read_lock();
     pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
-    if ( !pdev )
+    if ( !pdev || (pdev && !pdev->vpci) )
     {
+        pcidevs_read_unlock();
         vpci_write_hw(sbdf, reg, size, data);
         return;
     }
@@ -486,6 +495,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
         ASSERT(data_offset < size);
     }
     spin_unlock(&pdev->vpci->lock);
+    pcidevs_read_unlock();
 
     if ( data_offset < size )
         /* Tailing gap, write the remaining. */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 052b2ddf9f..c974ebdc94 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -161,6 +161,7 @@ void pcidevs_unlock(void);
 bool __must_check pcidevs_locked(void);
 
 void pcidevs_read_lock(void);
+int pcidevs_read_trylock(void);
 void pcidevs_read_unlock(void);
 bool __must_check pcidevs_read_locked(void);
 
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 67c9a0c631..7ab39839ff 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -175,7 +175,7 @@ int __must_check vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,
 int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
                                               const struct pci_dev *pdev);
 void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
-int vpci_msix_arch_print(const struct vpci_msix *msix);
+int vpci_msix_arch_print(const struct domain *d, const struct vpci_msix *msix);
 
 /*
  * Helper functions to fetch MSIX related data. They are used by both the
-- 
2.36.1

Re: [PATCH v2 3/4] vpci: use pcidevs locking to protect MMIO handlers

[Jan Beulich]

On 18.07.2022 23:15, Volodymyr Babchuk wrote:
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -891,10 +891,16 @@ void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry)
>      entry->arch.pirq = INVALID_PIRQ;
>  }
>  
> -int vpci_msix_arch_print(const struct vpci_msix *msix)
> +int vpci_msix_arch_print(const struct domain *d, const struct vpci_msix *msix)

I don't think the extra parameter is needed:

> @@ -911,11 +917,23 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>          if ( i && !(i % 64) )
>          {
>              struct pci_dev *pdev = msix->pdev;

You get hold of pdev here, and hence you can take the domain from pdev.

> +            pci_sbdf_t sbdf = pdev->sbdf;
>  
>              spin_unlock(&msix->pdev->vpci->lock);
> +            pcidevs_read_unlock();
> +
> +            /* NB: we still hold rcu_read_lock(&domlist_read_lock); here. */
>              process_pending_softirqs();
> -            /* NB: we assume that pdev cannot go away for an alive domain. */

I think this comment wants retaining, as the new one you add is about
a different aspect.

> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
> +
> +            if ( !pcidevs_read_trylock() )
> +                return -EBUSY;
> +            pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> +            /*
> +             * FIXME: we may find a re-allocated pdev's copy here.
> +             * Even occupying the same address as before. Do our best.
> +             */
> +            if ( !pdev || (pdev != msix->pdev) || !pdev->vpci ||

Despite the comment: What guarantees that msix isn't a dangling pointer
at this point? At the very least I think you need to check !pdev->vpci
first. And I'm afraid I don't view "do our best" as good enough here
(considering the patch doesn't carry an RFC tag). And no, I don't have
any good suggestion other than "our PCI device locking needs a complete
overhaul". Quite likely what we need is a refcounter per device, which
- as long as non-zero - prevents removal.

> +                 !spin_trylock(&pdev->vpci->lock) )
>                  return -EBUSY;

Don't you need to drop the pcidevs lock on this error path?

> @@ -450,10 +465,15 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      uint16_t cmd;
>      uint64_t addr, size;
>      unsigned int i, num_bars, rom_reg;
> -    struct vpci_header *header = &pdev->vpci->header;
> -    struct vpci_bar *bars = header->bars;
> +    struct vpci_header *header;
> +    struct vpci_bar *bars;
>      int rc;
>  
> +    ASSERT(pcidevs_write_locked());
> +
> +    header = &pdev->vpci->header;
> +    bars = header->bars;

I'm not convinced the code movement here does us any good. (Same
apparently elsewhere below.)

> @@ -277,6 +282,9 @@ void vpci_dump_msi(void)
>  
>          printk("vPCI MSI/MSI-X d%d\n", d->domain_id);
>  
> +        if ( !pcidevs_read_trylock() )
> +            continue;

Note how this lives ahead of ...

>          for_each_pdev ( d, pdev )
>          {

... the loop, while ...

> @@ -310,7 +318,7 @@ void vpci_dump_msi(void)
>                  printk("  entries: %u maskall: %d enabled: %d\n",
>                         msix->max_entries, msix->masked, msix->enabled);
>  
> -                rc = vpci_msix_arch_print(msix);
> +                rc = vpci_msix_arch_print(d, msix);
>                  if ( rc )
>                  {
>                      /*
> @@ -318,12 +326,13 @@ void vpci_dump_msi(void)
>                       * holding the lock.
>                       */
>                      printk("unable to print all MSI-X entries: %d\n", rc);
> -                    process_pending_softirqs();
> -                    continue;
> +                    goto pdev_done;
>                  }
>              }
>  
>              spin_unlock(&pdev->vpci->lock);
> + pdev_done:
> +            pcidevs_read_unlock();

... this is still inside the loop body.

> @@ -332,10 +334,14 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>          return data;
>      }
>  
> +    pcidevs_read_lock();
>      /* Find the PCI dev matching the address. */
>      pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> -    if ( !pdev )
> +    if ( !pdev || (pdev && !pdev->vpci) )

Simpler

    if ( !pdev || !pdev->vpci )

?

> @@ -381,6 +387,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>          ASSERT(data_offset < size);
>      }
>      spin_unlock(&pdev->vpci->lock);
> +    pcidevs_read_unlock();

I guess this is too early and wants to come after ...

>      if ( data_offset < size )
>      {

... this if, which - even if it doesn't use pdev - still accesses the
device.

Both comments equally apply to vpci_write().

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -161,6 +161,7 @@ void pcidevs_unlock(void);
>  bool __must_check pcidevs_locked(void);
>  
>  void pcidevs_read_lock(void);
> +int pcidevs_read_trylock(void);

This declaration wants adding alongside the introduction of the
function or, if the series was structured that way, at the time of the
dropping of "static" from the function (which from a Misra perspective
would likely be better).

Jan

Re: [PATCH v2 3/4] vpci: use pcidevs locking to protect MMIO handlers

[Volodymyr Babchuk]

Hello Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -891,10 +891,16 @@ void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry)
>>      entry->arch.pirq = INVALID_PIRQ;
>>  }
>>  
>> -int vpci_msix_arch_print(const struct vpci_msix *msix)
>> +int vpci_msix_arch_print(const struct domain *d, const struct vpci_msix *msix)
>
> I don't think the extra parameter is needed:
>
>> @@ -911,11 +917,23 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>          if ( i && !(i % 64) )
>>          {
>>              struct pci_dev *pdev = msix->pdev;
>
> You get hold of pdev here, and hence you can take the domain from pdev.

Yes, makes sense.

>> +            pci_sbdf_t sbdf = pdev->sbdf;
>>  
>>              spin_unlock(&msix->pdev->vpci->lock);
>> +            pcidevs_read_unlock();
>> +
>> +            /* NB: we still hold rcu_read_lock(&domlist_read_lock); here. */
>>              process_pending_softirqs();
>> -            /* NB: we assume that pdev cannot go away for an alive domain. */
>
> I think this comment wants retaining, as the new one you add is about
> a different aspect.
>
>> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
>> +
>> +            if ( !pcidevs_read_trylock() )
>> +                return -EBUSY;
>> +            pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
>> +            /*
>> +             * FIXME: we may find a re-allocated pdev's copy here.
>> +             * Even occupying the same address as before. Do our best.
>> +             */
>> +            if ( !pdev || (pdev != msix->pdev) || !pdev->vpci ||
>
> Despite the comment: What guarantees that msix isn't a dangling pointer
> at this point? At the very least I think you need to check !pdev->vpci
> first. And I'm afraid I don't view "do our best" as good enough here
> (considering the patch doesn't carry an RFC tag). And no, I don't have
> any good suggestion other than "our PCI device locking needs a complete
> overhaul". Quite likely what we need is a refcounter per device, which
> - as long as non-zero - prevents removal.

Refcounter itself is a good idea, but I'm not liking where all this
goes. We already are reworking locking by adding rw-locks with counters,
adding refcounter on top of this will complicate things even further.

I'm starting to think that complete PCI device locking rework may be
simpler solution, actually. By any chance, were there any prior
discussion on how proper locking should look like? 

>
>> +                 !spin_trylock(&pdev->vpci->lock) )
>>                  return -EBUSY;
>
> Don't you need to drop the pcidevs lock on this error path?

Yeah, you are right.

>
>> @@ -450,10 +465,15 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>      uint16_t cmd;
>>      uint64_t addr, size;
>>      unsigned int i, num_bars, rom_reg;
>> -    struct vpci_header *header = &pdev->vpci->header;
>> -    struct vpci_bar *bars = header->bars;
>> +    struct vpci_header *header;
>> +    struct vpci_bar *bars;
>>      int rc;
>>  
>> +    ASSERT(pcidevs_write_locked());
>> +
>> +    header = &pdev->vpci->header;
>> +    bars = header->bars;
>
> I'm not convinced the code movement here does us any good. (Same
> apparently elsewhere below.)
>
>> @@ -277,6 +282,9 @@ void vpci_dump_msi(void)
>>  
>>          printk("vPCI MSI/MSI-X d%d\n", d->domain_id);
>>  
>> +        if ( !pcidevs_read_trylock() )
>> +            continue;
>
> Note how this lives ahead of ...
>
>>          for_each_pdev ( d, pdev )
>>          {
>
> ... the loop, while ...
>
>> @@ -310,7 +318,7 @@ void vpci_dump_msi(void)
>>                  printk("  entries: %u maskall: %d enabled: %d\n",
>>                         msix->max_entries, msix->masked, msix->enabled);
>>  
>> -                rc = vpci_msix_arch_print(msix);
>> +                rc = vpci_msix_arch_print(d, msix);
>>                  if ( rc )
>>                  {
>>                      /*
>> @@ -318,12 +326,13 @@ void vpci_dump_msi(void)
>>                       * holding the lock.
>>                       */
>>                      printk("unable to print all MSI-X entries: %d\n", rc);
>> -                    process_pending_softirqs();
>> -                    continue;
>> +                    goto pdev_done;
>>                  }
>>              }
>>  
>>              spin_unlock(&pdev->vpci->lock);
>> + pdev_done:
>> +            pcidevs_read_unlock();
>
> ... this is still inside the loop body.
>
>> @@ -332,10 +334,14 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>>          return data;
>>      }
>>  
>> +    pcidevs_read_lock();
>>      /* Find the PCI dev matching the address. */
>>      pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
>> -    if ( !pdev )
>> +    if ( !pdev || (pdev && !pdev->vpci) )
>
> Simpler
>
>     if ( !pdev || !pdev->vpci )
>
> ?
>
>> @@ -381,6 +387,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>>          ASSERT(data_offset < size);
>>      }
>>      spin_unlock(&pdev->vpci->lock);
>> +    pcidevs_read_unlock();
>
> I guess this is too early and wants to come after ...
>
>>      if ( data_offset < size )
>>      {
>
> ... this if, which - even if it doesn't use pdev - still accesses the
> device.
>
> Both comments equally apply to vpci_write().
>
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -161,6 +161,7 @@ void pcidevs_unlock(void);
>>  bool __must_check pcidevs_locked(void);
>>  
>>  void pcidevs_read_lock(void);
>> +int pcidevs_read_trylock(void);
>
> This declaration wants adding alongside the introduction of the
> function or, if the series was structured that way, at the time of the
> dropping of "static" from the function (which from a Misra perspective
> would likely be better).
>
> Jan


-- 
Volodymyr Babchuk at EPAM

Re: [PATCH v2 3/4] vpci: use pcidevs locking to protect MMIO handlers

[Jan Beulich]

On 09.08.2022 22:33, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>>> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
>>> +
>>> +            if ( !pcidevs_read_trylock() )
>>> +                return -EBUSY;
>>> +            pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
>>> +            /*
>>> +             * FIXME: we may find a re-allocated pdev's copy here.
>>> +             * Even occupying the same address as before. Do our best.
>>> +             */
>>> +            if ( !pdev || (pdev != msix->pdev) || !pdev->vpci ||
>>
>> Despite the comment: What guarantees that msix isn't a dangling pointer
>> at this point? At the very least I think you need to check !pdev->vpci
>> first. And I'm afraid I don't view "do our best" as good enough here
>> (considering the patch doesn't carry an RFC tag). And no, I don't have
>> any good suggestion other than "our PCI device locking needs a complete
>> overhaul". Quite likely what we need is a refcounter per device, which
>> - as long as non-zero - prevents removal.
> 
> Refcounter itself is a good idea, but I'm not liking where all this
> goes. We already are reworking locking by adding rw-locks with counters,
> adding refcounter on top of this will complicate things even further.

I'm of quite the opposite opinion: A lot of the places will no longer
need to hold the pcidevs lock when instead they hold a reference; the
lock will only be needed to acquire a reference. Therefore refcounting
is likely to simplify things, presumably to the point where at least
recursive locking (and probably also converting to some r/w locking
scheme) won't be necessary. The main complicating factor is that all
places where a reference is needed will have to be located, and (quite
obviously I'm inclined to say) in particular all involved error paths
will need to be covered when it comes to dropping references no longer
needed.

> I'm starting to think that complete PCI device locking rework may be
> simpler solution, actually. By any chance, were there any prior
> discussion on how proper locking should look like? 

Well, there were prior discussions (you'd need to search the list, as
I have no pointers to hand), but I'm not sure a clear picture had
surfaced how "proper locking" would look like. I guess that's part of
the reason why the currently proposed locking model actually makes
things quite a bit more complicated.

Jan

[PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Volodymyr Babchuk]

Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
iounmap(), but not added corresponding include.

Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/drivers/vpci/vpci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1559763479..674c9b347d 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -19,6 +19,7 @@
 
 #include <xen/sched.h>
 #include <xen/vpci.h>
+#include <xen/vmap.h>
 
 /* Internal struct to store the emulated PCI registers. */
 struct vpci_register {
-- 
2.36.1

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Jan Beulich]

On 18.07.2022 23:15, Volodymyr Babchuk wrote:
> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
> iounmap(), but not added corresponding include.
> 
> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")

I don't think there's any active issue with the "missing" include:
That's only a problem once Arm has vPCI code enabled? In which
case I don't think a Fixes: tag is warranted.

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

With Roger away and on the basis that I'm sure we won't mind the
change:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Volodymyr Babchuk]

Hello Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
>> iounmap(), but not added corresponding include.
>> 
>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
>
> I don't think there's any active issue with the "missing" include:
> That's only a problem once Arm has vPCI code enabled? In which
> case I don't think a Fixes: tag is warranted.
>

Fair enough. May I ask committer to drop this tag?

>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> With Roger away and on the basis that I'm sure we won't mind the
> change:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thank you,

-- 
Volodymyr Babchuk at EPAM

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Jan Beulich]

On 19.07.2022 12:32, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
> 
>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
>>> iounmap(), but not added corresponding include.
>>>
>>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
>>
>> I don't think there's any active issue with the "missing" include:
>> That's only a problem once Arm has vPCI code enabled? In which
>> case I don't think a Fixes: tag is warranted.
> 
> Fair enough. May I ask committer to drop this tag?

I had taken respective note already, in case I end up committing this.
But this is the last patch of the series, so I can only guess whether
it might be okay to go in ahead of the other three patches.

Jan

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Oleksandr]

Hello all.


On 19.07.22 13:40, Jan Beulich wrote:
> On 19.07.2022 12:32, Volodymyr Babchuk wrote:
>> Jan Beulich <jbeulich@suse.com> writes:
>>
>>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>>>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
>>>> iounmap(), but not added corresponding include.
>>>>
>>>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
>>> I don't think there's any active issue with the "missing" include:
>>> That's only a problem once Arm has vPCI code enabled? In which
>>> case I don't think a Fixes: tag is warranted.
>> Fair enough. May I ask committer to drop this tag?
> I had taken respective note already, in case I end up committing this.
> But this is the last patch of the series, so I can only guess whether
> it might be okay to go in ahead of the other three patches.
>
> Jan


I am wondering, where this patch could be 4.17 material?

The patch series seem to get stuck, but the current patch just adds a 
missing include to fix a build on Arm, so it is completely independent. 
I agree, there is no issue with the current code base as vPCI is 
disabled on Arm, so nothing to fix right now. But as PCI 
passthrough/vPCI on Arm is in the development stage, the developers 
enable that support in their builds. I think the risk is rather low than 
high.



-- 
Regards,

Oleksandr Tyshchenko

RE: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Henry Wang]

(+ Arm maintainers)

Hi Oleksandr,

> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Subject: Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM
> Hello all.
> On 19.07.22 13:40, Jan Beulich wrote:
> > On 19.07.2022 12:32, Volodymyr Babchuk wrote:
> >> Jan Beulich <jbeulich@suse.com> writes:
> >>
> >>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
> >>>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
> >>>> iounmap(), but not added corresponding include.
> >>>>
> >>>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
> >>> I don't think there's any active issue with the "missing" include:
> >>> That's only a problem once Arm has vPCI code enabled? In which
> >>> case I don't think a Fixes: tag is warranted.
> >> Fair enough. May I ask committer to drop this tag?
> > I had taken respective note already, in case I end up committing this.
> > But this is the last patch of the series, so I can only guess whether
> > it might be okay to go in ahead of the other three patches.
> >
> > Jan
> 
> 
> I am wondering, where this patch could be 4.17 material?
> 
> The patch series seem to get stuck, but the current patch just adds a
> missing include to fix a build on Arm, so it is completely independent.
> I agree, there is no issue with the current code base as vPCI is
> disabled on Arm, so nothing to fix right now. But as PCI
> passthrough/vPCI on Arm is in the development stage, the developers
> enable that support in their builds. I think the risk is rather low than
> high.

It seems reasonable to me, but I am curious about what Arm maintainers
and PCI maintainers think. From the history discussion in this thread I
think it is pretty safe to include this in 4.17. Thanks for the ping.

Kind regards,
Henry


> 
> 
> 
> --
> Regards,
> 
> Oleksandr Tyshchenko

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Bertrand Marquis]

Hi,

> On 21 Oct 2022, at 15:40, Henry Wang <Henry.Wang@arm.com> wrote:
> 
> (+ Arm maintainers)
> 
> Hi Oleksandr,
> 
>> -----Original Message-----
>> From: Oleksandr <olekstysh@gmail.com>
>> Subject: Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM
>> Hello all.
>> On 19.07.22 13:40, Jan Beulich wrote:
>>> On 19.07.2022 12:32, Volodymyr Babchuk wrote:
>>>> Jan Beulich <jbeulich@suse.com> writes:
>>>> 
>>>>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>>>>>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
>>>>>> iounmap(), but not added corresponding include.
>>>>>> 
>>>>>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
>>>>> I don't think there's any active issue with the "missing" include:
>>>>> That's only a problem once Arm has vPCI code enabled? In which
>>>>> case I don't think a Fixes: tag is warranted.
>>>> Fair enough. May I ask committer to drop this tag?
>>> I had taken respective note already, in case I end up committing this.
>>> But this is the last patch of the series, so I can only guess whether
>>> it might be okay to go in ahead of the other three patches.
>>> 
>>> Jan
>> 
>> 
>> I am wondering, where this patch could be 4.17 material?
>> 
>> The patch series seem to get stuck, but the current patch just adds a
>> missing include to fix a build on Arm, so it is completely independent.
>> I agree, there is no issue with the current code base as vPCI is
>> disabled on Arm, so nothing to fix right now. But as PCI
>> passthrough/vPCI on Arm is in the development stage, the developers
>> enable that support in their builds. I think the risk is rather low than
>> high.
> 
> It seems reasonable to me, but I am curious about what Arm maintainers
> and PCI maintainers think. From the history discussion in this thread I
> think it is pretty safe to include this in 4.17. Thanks for the ping.

I think this can safely go in for 4.17.

Cheers
Bertrand

> 
> Kind regards,
> Henry
> 
> 
>> 
>> 
>> 
>> --
>> Regards,
>> 
>> Oleksandr Tyshchenko

RE: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Henry Wang]

Hi Bertrand,

> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM
> 
> Hi,
> 
> > On 21 Oct 2022, at 15:40, Henry Wang <Henry.Wang@arm.com> wrote:
> >
> > (+ Arm maintainers)
> >
> > Hi Oleksandr,
> >
> >> -----Original Message-----
> >> From: Oleksandr <olekstysh@gmail.com>
> >> Subject: Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM
> >> Hello all.
> >> On 19.07.22 13:40, Jan Beulich wrote:
> >>> On 19.07.2022 12:32, Volodymyr Babchuk wrote:
> >>>> Jan Beulich <jbeulich@suse.com> writes:
> >>>>
> >>>>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
> >>>>>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
> >>>>>> iounmap(), but not added corresponding include.
> >>>>>>
> >>>>>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
> >>>>> I don't think there's any active issue with the "missing" include:
> >>>>> That's only a problem once Arm has vPCI code enabled? In which
> >>>>> case I don't think a Fixes: tag is warranted.
> >>>> Fair enough. May I ask committer to drop this tag?
> >>> I had taken respective note already, in case I end up committing this.
> >>> But this is the last patch of the series, so I can only guess whether
> >>> it might be okay to go in ahead of the other three patches.
> >>>
> >>> Jan
> >>
> >>
> >> I am wondering, where this patch could be 4.17 material?
> >>
> >> The patch series seem to get stuck, but the current patch just adds a
> >> missing include to fix a build on Arm, so it is completely independent.
> >> I agree, there is no issue with the current code base as vPCI is
> >> disabled on Arm, so nothing to fix right now. But as PCI
> >> passthrough/vPCI on Arm is in the development stage, the developers
> >> enable that support in their builds. I think the risk is rather low than
> >> high.
> >
> > It seems reasonable to me, but I am curious about what Arm maintainers
> > and PCI maintainers think. From the history discussion in this thread I
> > think it is pretty safe to include this in 4.17. Thanks for the ping.
> 
> I think this can safely go in for 4.17.
> 
> Cheers
> Bertrand

Thanks for the feedback :) Feel free to add my:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry


> 
> >
> > Kind regards,
> > Henry
> >
> >
> >>
> >>
> >>
> >> --
> >> Regards,
> >>
> >> Oleksandr Tyshchenko
> 

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Jan Beulich]

On 22.10.2022 02:50, Henry Wang wrote:
>> -----Original Message-----
>> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
>> Subject: Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM
>>
>>> On 21 Oct 2022, at 15:40, Henry Wang <Henry.Wang@arm.com> wrote:
>>>
>>> (+ Arm maintainers)
>>>
>>>> -----Original Message-----
>>>> From: Oleksandr <olekstysh@gmail.com>
>>>> Subject: Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM
>>>> On 19.07.22 13:40, Jan Beulich wrote:
>>>>> On 19.07.2022 12:32, Volodymyr Babchuk wrote:
>>>>>> Jan Beulich <jbeulich@suse.com> writes:
>>>>>>
>>>>>>> On 18.07.2022 23:15, Volodymyr Babchuk wrote:
>>>>>>>> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
>>>>>>>> iounmap(), but not added corresponding include.
>>>>>>>>
>>>>>>>> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
>>>>>>> I don't think there's any active issue with the "missing" include:
>>>>>>> That's only a problem once Arm has vPCI code enabled? In which
>>>>>>> case I don't think a Fixes: tag is warranted.
>>>>>> Fair enough. May I ask committer to drop this tag?
>>>>> I had taken respective note already, in case I end up committing this.
>>>>> But this is the last patch of the series, so I can only guess whether
>>>>> it might be okay to go in ahead of the other three patches.
>>>>>
>>>>> Jan
>>>>
>>>>
>>>> I am wondering, where this patch could be 4.17 material?
>>>>
>>>> The patch series seem to get stuck, but the current patch just adds a
>>>> missing include to fix a build on Arm, so it is completely independent.
>>>> I agree, there is no issue with the current code base as vPCI is
>>>> disabled on Arm, so nothing to fix right now. But as PCI
>>>> passthrough/vPCI on Arm is in the development stage, the developers
>>>> enable that support in their builds. I think the risk is rather low than
>>>> high.
>>>
>>> It seems reasonable to me, but I am curious about what Arm maintainers
>>> and PCI maintainers think. From the history discussion in this thread I
>>> think it is pretty safe to include this in 4.17. Thanks for the ping.
>>
>> I think this can safely go in for 4.17.
>>
>> Cheers
>> Bertrand
> 
> Thanks for the feedback :) Feel free to add my:
> 
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Okay, recorded, but first of all this patch needs Roger's ack.

Jan

Re: [PATCH v2 4/4] vpci: include xen/vmap.h to fix build on ARM

[Roger Pau Monné]

On Mon, Jul 18, 2022 at 09:15:43PM +0000, Volodymyr Babchuk wrote:
> Patch b4f211606011 ("vpci/msix: fix PBA accesses") introduced call to
> iounmap(), but not added corresponding include.
> 
> Fixes: b4f211606011 ("vpci/msix: fix PBA accesses")
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

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

Thanks, Roger.