[PATCH v21 0/2] PCI devices passthrough on Arm, part 3

[PATCH v21 0/2] PCI devices passthrough on Arm, part 3

[Stewart Hildebrand]

This is next version of vPCI rework. Aim of this series is to prepare
ground for introducing PCI support on ARM platform.

in v21:
* drop ("xen/arm: check read handler behavior") as it was committed
* add ("arm/vpci: mask off upper bits in vPCI read mmio handler")

in v20:
 - drop ("vpci: acquire d->pci_lock in I/O handlers") as it was
   unnecessary

in v19:
 - ("xen/arm: check read handler behavior") is ready to be committed
 - add ("vpci: acquire d->pci_lock in I/O handlers")

in v18:
 - address warning in vpci test suite

in v17:
 - add ("xen/arm: check read handler behavior")
 - drop ("xen/arm: account IO handlers for emulated PCI MSI-X") as it
   should wait for future work
 - drop committed patches

in v16:
 - minor updates - see individual patches

in v15:
 - reorder so ("arm/vpci: honor access size when returning an error")
   comes first

in v14:
 - drop first 9 patches as they were committed
 - updated ("vpci/header: emulate PCI_COMMAND register for guests")

in v13:
 - drop ("xen/arm: vpci: permit access to guest vpci space") as it was
   unnecessary

in v12:
 - I (Stewart) coordinated with Volodomyr to send this whole series. So,
   add my (Stewart) Signed-off-by to all patches.
 - The biggest change is to re-work the PCI_COMMAND register patch.
   Additional feedback has also been addressed - see individual patches.
 - Drop ("pci: msi: pass pdev to pci_enable_msi() function") and
   ("pci: introduce per-domain PCI rwlock") as they were committed
 - Rename ("rangeset: add rangeset_empty() function")
       to ("rangeset: add rangeset_purge() function")
 - Rename ("vpci/header: rework exit path in init_bars")
       to ("vpci/header: rework exit path in init_header()")

in v11:
 - Added my (Volodymyr) Signed-off-by tag to all patches
 - Patch "vpci/header: emulate PCI_COMMAND register for guests" is in
   intermediate state, because it was agreed to rework it once Stewart's
   series on register handling are in.
 - Addressed comments, please see patch descriptions for details.

in v10:

 - Removed patch ("xen/arm: vpci: check guest range"), proper fix
   for the issue is part of ("vpci/header: emulate PCI_COMMAND
   register for guests")
 - Removed patch ("pci/header: reset the command register when adding
   devices")
 - Added patch ("rangeset: add rangeset_empty() function") because
   this function is needed in ("vpci/header: handle p2m range sets
   per BAR")
 - Added ("vpci/header: handle p2m range sets per BAR") which addressed
   an issue discovered by Andrii Chepurnyi during virtio integration
 - Added ("pci: msi: pass pdev to pci_enable_msi() function"), which is
   prereq for ("pci: introduce per-domain PCI rwlock")
 - Fixed "Since v9/v8/... " comments in changelogs to reduce confusion.
   I left "Since" entries for older versions, because they were added
   by original author of the patches.

in v9:

v9 includes addressed commentes from a previous one. Also it
introduces a couple patches from Stewart. This patches are related to
vPCI use on ARM. Patch "vpci/header: rework exit path in init_bars"
was factored-out from "vpci/header: handle p2m range sets per BAR".

in v8:

The biggest change from previous, mistakenly named, v7 series is how
locking is implemented. Instead of d->vpci_rwlock we introduce
d->pci_lock which has broader scope, as it protects not only domain's
vpci state, but domain's list of PCI devices as well.

As we discussed in IRC with Roger, it is not feasible to rework all
the existing code to use the new lock right away. It was agreed that
any write access to d->pdev_list will be protected by **both**
d->pci_lock in write mode and pcidevs_lock(). Read access on other
hand should be protected by either d->pci_lock in read mode or
pcidevs_lock(). It is expected that existing code will use
pcidevs_lock() and new users will use new rw lock. Of course, this
does not mean that new users shall not use pcidevs_lock() when it is
appropriate.

Changes from previous versions are described in each separate patch.

Oleksandr Andrushchenko (1):
  vpci: translate virtual PCI bus topology for guests

Stewart Hildebrand (1):
  arm/vpci: mask off upper bits in vPCI read mmio handler

 tools/tests/vpci/emul.h |  2 +-
 xen/arch/arm/vpci.c     |  6 ++++-
 xen/drivers/vpci/vpci.c | 53 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 53 insertions(+), 8 deletions(-)


base-commit: aea52ce607fe716acc56ad89f07e1513c89018eb
-- 
2.49.0

[PATCH v21 1/2] arm/vpci: mask off upper bits in vPCI read mmio handler

[Stewart Hildebrand]

On Arm, we expect read handlers to have the bits above the access size
zeroed. vPCI read handlers may return all 1s. Mask off the bits above
the access size.

Fixes: 9a5e22b64266 ("xen/arm: check read handler behavior")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 xen/arch/arm/vpci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index b63a356bb4a8..3a3ff5d0812c 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -37,7 +37,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
     if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
                         1U << info->dabt.size, &data) )
     {
-        *r = data;
+        *r = data & invalid;
         return 1;
     }
 
-- 
2.49.0

Re: [PATCH v21 1/2] arm/vpci: mask off upper bits in vPCI read mmio handler

[Stefano Stabellini]

On Thu, 8 May 2025, Stewart Hildebrand wrote:
> On Arm, we expect read handlers to have the bits above the access size
> zeroed. vPCI read handlers may return all 1s. Mask off the bits above
> the access size.
> 
> Fixes: 9a5e22b64266 ("xen/arm: check read handler behavior")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/vpci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index b63a356bb4a8..3a3ff5d0812c 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -37,7 +37,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>      if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>                          1U << info->dabt.size, &data) )
>      {
> -        *r = data;
> +        *r = data & invalid;
>          return 1;
>      }
>  
> -- 
> 2.49.0
> 

[PATCH v21 2/2] vpci: translate virtual PCI bus topology for guests

[Stewart Hildebrand]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

There are two originators for the PCI configuration space access:
1. The domain that owns physical host bridge: MMIO handlers are
there so we can update vPCI register handlers with the values
written by the hardware domain, e.g. physical view of the registers
vs guest's view on the configuration space.
2. Guest access to the passed through PCI devices: we need to properly
map virtual bus topology to the physical one, e.g. pass the configuration
space access to the corresponding physical devices.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
In v21:
* adjust #ifdef
* remove redundant return path
In v20:
* call translate_virtual_device() from within locked context of
  vpci_{read,write}
* update commit message
In v19:
* move locking to pre-patch
In v18:
* address warning in vpci test suite
In v17:
* move lock to inside vpci_translate_virtual_device()
* acks were previously given for Arm [0] and vPCI [1], but since it was
  two releases ago and the patch has changed, I didn't pick them up

[0] https://lore.kernel.org/xen-devel/4afe33f2-72e6-4755-98ce-d7f9da374e90@xen.org/
[1] https://lore.kernel.org/xen-devel/Zk70udmiriruMt0r@macbook/

In v15:
- base on top of ("arm/vpci: honor access size when returning an error")
In v11:
- Fixed format issues
- Added ASSERT_UNREACHABLE() to the dummy implementation of
vpci_translate_virtual_device()
- Moved variable in vpci_sbdf_from_gpa(), now it is easier to follow
the logic in the function
Since v9:
- Commend about required lock replaced with ASSERT()
- Style fixes
- call to vpci_translate_virtual_device folded into vpci_sbdf_from_gpa
Since v8:
- locks moved out of vpci_translate_virtual_device()
Since v6:
- add pcidevs locking to vpci_translate_virtual_device
- update wrt to the new locking scheme
Since v5:
- add vpci_translate_virtual_device for #ifndef CONFIG_HAS_VPCI_GUEST_SUPPORT
  case to simplify ifdefery
- add ASSERT(!is_hardware_domain(d)); to vpci_translate_virtual_device
- reset output register on failed virtual SBDF translation
Since v4:
- indentation fixes
- constify struct domain
- updated commit message
- updates to the new locking scheme (pdev->vpci_lock)
Since v3:
- revisit locking
- move code to vpci.c
Since v2:
 - pass struct domain instead of struct vcpu
 - constify arguments where possible
 - gate relevant code with CONFIG_HAS_VPCI_GUEST_SUPPORT
New in v2
---
 tools/tests/vpci/emul.h |  2 +-
 xen/arch/arm/vpci.c     |  4 ++++
 xen/drivers/vpci/vpci.c | 53 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index da446bba86b4..dd048cffbf9d 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -89,7 +89,7 @@ typedef union {
 
 #define __hwdom_init
 
-#define is_hardware_domain(d) ((void)(d), false)
+#define is_hardware_domain(d) ((void)(d), true)
 
 #define has_vpci(d) true
 
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index 3a3ff5d0812c..0ce11ffcc508 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -34,6 +34,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
     /* data is needed to prevent a pointer cast on 32bit */
     unsigned long data;
 
+    ASSERT(!bridge == !is_hardware_domain(v->domain));
+
     if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
                         1U << info->dabt.size, &data) )
     {
@@ -52,6 +54,8 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
     struct pci_host_bridge *bridge = p;
     pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
 
+    ASSERT(!bridge == !is_hardware_domain(v->domain));
+
     return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
                            1U << info->dabt.size, r);
 }
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1e6aa5d799b9..d2f0f97e0a04 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -174,6 +174,35 @@ int vpci_assign_device(struct pci_dev *pdev)
 }
 #endif /* __XEN__ */
 
+/*
+ * Find the physical device which is mapped to the virtual device
+ * and translate virtual SBDF to the physical one.
+ */
+static const struct pci_dev *translate_virtual_device(const struct domain *d,
+                                                      pci_sbdf_t *sbdf)
+{
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+    const struct pci_dev *pdev;
+
+    ASSERT(!is_hardware_domain(d));
+    ASSERT(rw_is_locked(&d->pci_lock));
+
+    for_each_pdev ( d, pdev )
+    {
+        if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) )
+        {
+            /* Replace guest SBDF with the physical one. */
+            *sbdf = pdev->sbdf;
+            return pdev;
+        }
+    }
+#else /* !CONFIG_HAS_VPCI_GUEST_SUPPORT */
+    ASSERT_UNREACHABLE();
+#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
+
+    return NULL;
+}
+
 static int vpci_register_cmp(const struct vpci_register *r1,
                              const struct vpci_register *r2)
 {
@@ -453,9 +482,15 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
      * pci_lock is sufficient.
      */
     read_lock(&d->pci_lock);
-    pdev = pci_get_pdev(d, sbdf);
-    if ( !pdev && is_hardware_domain(d) )
-        pdev = pci_get_pdev(dom_xen, sbdf);
+    if ( is_hardware_domain(d) )
+    {
+        pdev = pci_get_pdev(d, sbdf);
+        if ( !pdev )
+            pdev = pci_get_pdev(dom_xen, sbdf);
+    }
+    else
+        pdev = translate_virtual_device(d, &sbdf);
+
     if ( !pdev || !pdev->vpci )
     {
         read_unlock(&d->pci_lock);
@@ -571,9 +606,15 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
      * are modifying BARs, so there is a room for improvement.
      */
     write_lock(&d->pci_lock);
-    pdev = pci_get_pdev(d, sbdf);
-    if ( !pdev && is_hardware_domain(d) )
-        pdev = pci_get_pdev(dom_xen, sbdf);
+    if ( is_hardware_domain(d) )
+    {
+        pdev = pci_get_pdev(d, sbdf);
+        if ( !pdev )
+            pdev = pci_get_pdev(dom_xen, sbdf);
+    }
+    else
+        pdev = translate_virtual_device(d, &sbdf);
+
     if ( !pdev || !pdev->vpci )
     {
         /* Ignore writes to read-only devices, which have no ->vpci. */
-- 
2.49.0

Re: [PATCH v21 2/2] vpci: translate virtual PCI bus topology for guests

[Roger Pau Monné]

On Thu, May 08, 2025 at 06:46:07AM -0400, Stewart Hildebrand wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> There are two originators for the PCI configuration space access:
> 1. The domain that owns physical host bridge: MMIO handlers are
> there so we can update vPCI register handlers with the values
> written by the hardware domain, e.g. physical view of the registers
> vs guest's view on the configuration space.
> 2. Guest access to the passed through PCI devices: we need to properly
> map virtual bus topology to the physical one, e.g. pass the configuration
> space access to the corresponding physical devices.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

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

There's the addition of the ASSERTs in vpci_mmio_{read,write}() which
could do with an ARM maintainer Ack/RB.

Thanks, Roger.

Re: [PATCH v21 2/2] vpci: translate virtual PCI bus topology for guests

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

On Fri, 9 May 2025, Roger Pau Monné wrote:
> On Thu, May 08, 2025 at 06:46:07AM -0400, Stewart Hildebrand wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > 
> > There are two originators for the PCI configuration space access:
> > 1. The domain that owns physical host bridge: MMIO handlers are
> > there so we can update vPCI register handlers with the values
> > written by the hardware domain, e.g. physical view of the registers
> > vs guest's view on the configuration space.
> > 2. Guest access to the passed through PCI devices: we need to properly
> > map virtual bus topology to the physical one, e.g. pass the configuration
> > space access to the corresponding physical devices.
> > 
> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> There's the addition of the ASSERTs in vpci_mmio_{read,write}() which
> could do with an ARM maintainer Ack/RB.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>