Re: [PATCH v2 3/3] [FUTURE] xen/arm: enable vPCI for domUs

[Rahul Singh <Rahul.Singh@arm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4452 bytes --]

Hi Stewart,

> On 7 Jul 2023, at 2:47 am, Stewart Hildebrand <Stewart.Hildebrand@amd.com> wrote:
>
> Remove is_hardware_domain check in has_vpci, and select HAS_VPCI_GUEST_SUPPORT
> in Kconfig.
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg00863.html
>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> As the tag implies, this patch is not intended to be merged (yet).
>
> Note that CONFIG_HAS_VPCI_GUEST_SUPPORT is not currently used in the upstream
> code base. It will be used by the vPCI series [1]. This patch is intended to be
> merged as part of the vPCI series.
>
> v1->v2:
> * new patch
> ---
> xen/arch/arm/Kconfig              | 1 +
> xen/arch/arm/include/asm/domain.h | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 4e0cc421ad48..75dfa2f5a82d 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -195,6 +195,7 @@ config PCI_PASSTHROUGH
> depends on ARM_64
> select HAS_PCI
> select HAS_VPCI
> + select HAS_VPCI_GUEST_SUPPORT

I tested this series on top of "SMMU handling for PCIe Passthrough on ARM” series on the N1SDP board
and observe the SMMUv3 fault.

Enable the Kconfig option PCI_PASSTHROUGH, ARM_SMMU_V3,HAS_ITS and "iommu=on”,
"pci_passthrough_enabled=on" cmd line parameter and after that, there is an SMMU fault
for the ITS doorbell register access from the PCI devices.

As there is no upstream support for ARM for vPCI MSI/MSI-X handling because of that SMMU fault is observed.

Linux Kernel will set the ITS doorbell register( physical address of doorbell register as IOMMU is not enabled in Kernel)
in PCI config space to set up the MSI-X interrupts, but there is no mapping in SMMU page tables because of that SMMU
fault is observed. To fix this we need to map the ITS doorbell register to SMMU page tables to avoid the fault.

We can fix this after setting the mapping for the ITS doorbell offset in the ITS code.

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 299b384250..8227a7a74b 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -682,6 +682,18 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
                                          BIT(size, UL), valid);
         if ( ret && valid )
             return ret;
+
+        if ( is_iommu_enabled(its->d) ) {
+            ret = map_mmio_regions(its->d, gaddr_to_gfn(its->doorbell_address),
+                           PFN_UP(ITS_DOORBELL_OFFSET),
+                           maddr_to_mfn(its->doorbell_address));
+            if ( ret < 0 )
+            {
+                printk(XENLOG_ERR "GICv3: Map ITS translation register d%d failed.\n",
+                        its->d->domain_id);
+                return ret;
+            }
+        }
     }

Also as per Julien's request, I tried to set up the IOMMU for the PCI device without
"pci_passthroigh_enable=on" and without HAS_VPCI everything works as expected
after applying below patches.

To test enable kconfig options HAS_PCI, ARM_SMMU_V3 and HAS_ITS and add below
patches to make it work.

    • Set the mapping for the ITS doorbell offset in the ITS code when iommu is enabled.
    • Reverted the patch that added the support for pci_passthrough_on.
    • Allow MMIO mapping of ECAM space to dom0 when vPCI is not enabled, as of now MMIO
      mapping for ECAM is based on pci_passthrough_enabled. We need this patch if we want to avoid
     enabling HAS_VPCI

Please find the attached patches in case you want to test at your end.



Regards,
Rahul

> default n
> help
>  This option enables PCI device passthrough
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index 1a13965a26b8..6e016b00bae1 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -298,7 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>
> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
>
> -#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); })
> +#define has_vpci(d)    ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); })
>
> struct arch_vcpu_io {
>     struct instr_details dabt_instr; /* when the instruction is decoded */
> --
> 2.41.0
>
>


[-- Attachment #1.2: Type: text/html, Size: 6969 bytes --]

[-- Attachment #2: 0001-Revert-xen-arm-Add-cmdline-boot-option-pci-passthrou.patch --]
[-- Type: application/octet-stream, Size: 4578 bytes --]

From 013124b496574c1eda10e04f119d2d227f101e25 Mon Sep 17 00:00:00 2001
Message-Id: <013124b496574c1eda10e04f119d2d227f101e25.1688725585.git.rahul.singh@arm.com>
From: Rahul Singh <rahul.singh@arm.com>
Date: Fri, 7 Jul 2023 09:48:04 +0100
Subject: [PATCH 1/3] Revert "xen/arm: Add cmdline boot option "pci-passthrough
 = <boolean>""

This reverts commit 15517ed61f55be6039aedcc99720ee07c772ed44.

Change-Id: I3ffe6b0d4a806de64f183ec8604b6f62bf24104a
---
 docs/misc/xen-command-line.pandoc |  7 -------
 xen/arch/arm/include/asm/pci.h    | 12 ------------
 xen/arch/arm/pci/pci.c            | 12 ------------
 xen/arch/x86/include/asm/pci.h    |  6 ------
 xen/drivers/pci/physdev.c         |  6 ------
 5 files changed, 43 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 4060ebdc5d..0f84447ae6 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1929,13 +1929,6 @@ All numbers specified must be hexadecimal ones.
 
 This option can be specified more than once (up to 8 times at present).
 
-### pci-passthrough (arm)
-> `= <boolean>`
-
-> Default: `false`
-
-Flag to enable or disable support for PCI passthrough
-
 ### pcid (x86)
 > `= <boolean> | xpti=<bool>`
 
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 4da6c41df8..77eeba634c 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -21,8 +21,6 @@
 
 #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
 
-extern bool pci_passthrough_enabled;
-
 /* Arch pci dev struct */
 struct arch_pci_dev {
     struct device dev;
@@ -115,11 +113,6 @@ pci_find_host_bridge_node(const struct pci_dev *pdev);
 int pci_get_host_bridge_segment(const struct dt_device_node *node,
                                 uint16_t *segment);
 
-static always_inline bool is_pci_passthrough_enabled(void)
-{
-    return pci_passthrough_enabled;
-}
-
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
 int pci_get_new_domain_nr(void);
@@ -136,11 +129,6 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
 
 struct arch_pci_dev { };
 
-static always_inline bool is_pci_passthrough_enabled(void)
-{
-    return false;
-}
-
 struct pci_dev;
 
 static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 78b97beaef..e0a63242ab 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -16,7 +16,6 @@
 #include <xen/device_tree.h>
 #include <xen/errno.h>
 #include <xen/init.h>
-#include <xen/param.h>
 #include <xen/pci.h>
 
 /*
@@ -75,19 +74,8 @@ static int __init acpi_pci_init(void)
 }
 #endif
 
-/* By default pci passthrough is disabled. */
-bool __read_mostly pci_passthrough_enabled;
-boolean_param("pci-passthrough", pci_passthrough_enabled);
-
 static int __init pci_init(void)
 {
-    /*
-     * Enable PCI passthrough when has been enabled explicitly
-     * (pci-passthrough=on).
-     */
-    if ( !pci_passthrough_enabled )
-        return 0;
-
     pci_segments_init();
 
     if ( acpi_disabled )
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index f4a58c8acf..3eb6fb8edf 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -49,12 +49,6 @@ bool_t pci_ro_mmcfg_decode(unsigned long mfn, unsigned int *seg,
 extern int pci_mmcfg_config_num;
 extern struct acpi_mcfg_allocation *pci_mmcfg_config;
 
-/* Unlike ARM, PCI passthrough is always enabled for x86. */
-static always_inline bool is_pci_passthrough_enabled(void)
-{
-    return true;
-}
-
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
 static inline bool pci_check_bar(const struct pci_dev *pdev,
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index 42db3e6d13..4f3e1a96c0 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -18,9 +18,6 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct pci_dev_info pdev_info;
         nodeid_t node = NUMA_NO_NODE;
 
-        if ( !is_pci_passthrough_enabled() )
-            return -EOPNOTSUPP;
-
         ret = -EFAULT;
         if ( copy_from_guest(&add, arg, 1) != 0 )
             break;
@@ -56,9 +53,6 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case PHYSDEVOP_pci_device_remove: {
         struct physdev_pci_device dev;
 
-        if ( !is_pci_passthrough_enabled() )
-            return -EOPNOTSUPP;
-
         ret = -EFAULT;
         if ( copy_from_guest(&dev, arg, 1) != 0 )
             break;
-- 
2.25.1


[-- Attachment #3: 0002-xen-arm-Fix-mapping-for-PCI-bridge-mmio-region.patch --]
[-- Type: application/octet-stream, Size: 2083 bytes --]

From 95243898d28a33a4f676f2fd052ae4274ac05099 Mon Sep 17 00:00:00 2001
Message-Id: <95243898d28a33a4f676f2fd052ae4274ac05099.1688725585.git.rahul.singh@arm.com>
In-Reply-To: <013124b496574c1eda10e04f119d2d227f101e25.1688725585.git.rahul.singh@arm.com>
References: <013124b496574c1eda10e04f119d2d227f101e25.1688725585.git.rahul.singh@arm.com>
From: Rahul Singh <rahul.singh@arm.com>
Date: Fri, 7 Jul 2023 09:49:23 +0100
Subject: [PATCH 2/3] xen/arm: Fix mapping for PCI bridge mmio region

Current code skip the mapping for PCI bridge MMIO region to dom0 when
pci_passthrough_enabled flag is set. Mapping shoule be skip when
has_vpci(d) is enabled for the domain, as we need to skip the mapping
only when VPCI handler are registered for ECAM.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Change-Id: Ieb63efb19b369b1b3d6fcd7937d8c79534dd846c
---
 xen/arch/arm/domain_build.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8d4aac2a5a..70d479e92f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1064,7 +1064,7 @@ static void __init assign_static_memory_11(struct domain *d,
 #endif
 
 /*
- * When PCI passthrough is available we want to keep the
+ * When HAS_PCI is enabled we want to keep the
  * "linux,pci-domain" in sync for every host bridge.
  *
  * Xen may not have a driver for all the host bridges. So we have
@@ -1080,9 +1080,6 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
     uint16_t segment;
     int res;
 
-    if ( !is_pci_passthrough_enabled() )
-        return 0;
-
     if ( !dt_device_type_is_equal(node, "pci") )
         return 0;
 
@@ -2524,7 +2521,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
         .d = d,
         .p2mt = p2mt,
         .skip_mapping = !own_device ||
-                        (is_pci_passthrough_enabled() &&
+                        (has_vpci(d) &&
                         (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))
     };
 
-- 
2.25.1


[-- Attachment #4: 0003-xen-arm-Map-ITS-dorrbell-register-to-IOMMU-page-tabl.patch --]
[-- Type: application/octet-stream, Size: 1596 bytes --]

From 44610a55a43c1adb4e2d4dae07943ef8e835fdb9 Mon Sep 17 00:00:00 2001
Message-Id: <44610a55a43c1adb4e2d4dae07943ef8e835fdb9.1688725585.git.rahul.singh@arm.com>
In-Reply-To: <013124b496574c1eda10e04f119d2d227f101e25.1688725585.git.rahul.singh@arm.com>
References: <013124b496574c1eda10e04f119d2d227f101e25.1688725585.git.rahul.singh@arm.com>
From: Rahul Singh <rahul.singh@arm.com>
Date: Fri, 7 Jul 2023 11:25:33 +0100
Subject: [PATCH 3/3] xen/arm: Map ITS dorrbell register to IOMMU page tables.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Change-Id: Id19373d65b94583c5a9f1b95854e6b2790dc695c
---
 xen/arch/arm/vgic-v3-its.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 299b384250..8227a7a74b 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -682,6 +682,18 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
                                          BIT(size, UL), valid);
         if ( ret && valid )
             return ret;
+
+        if ( is_iommu_enabled(its->d) ) {
+            ret = map_mmio_regions(its->d, gaddr_to_gfn(its->doorbell_address),
+                           PFN_UP(ITS_DOORBELL_OFFSET),
+                           maddr_to_mfn(its->doorbell_address));
+            if ( ret < 0 )
+            {
+                printk(XENLOG_ERR "GICv3: Map ITS translation register d%d failed.\n",
+                        its->d->domain_id);
+                return ret;
+            }
+        }
     }
 
     spin_lock(&its->its_lock);
-- 
2.25.1