Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> 
> Enable the use of IOMMU + PCI in dom0 without having to specify
> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
> 
> Enable pci_init() for initializing Xen's internal PCI subsystem, and
> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> ---
> hmm. Since
>   dec9e02f3190 ("xen: avoid generation of stub <asm/pci.h> header")
> Should we also move is_pci_passthrough_enabled() back to 
> xen/arch/arm/include/asm/pci.h ?
> Not sure if PPC/RISC-V will plan on using this check.
> 
> v8-v9:
> * move iommu_enabled check inside is_pci_passthrough_enabled()
> 
> v7->v8:
> * bring back x86 definition of is_pci_passthrough_enabled()
> 
> v6->v7:
> * remove x86 definition of is_pci_passthrough_enabled()
> * update comments
> * make pci_physdev_op checks stricter
> 
> v5->v6:
> * new patch - this effectively replaces
>   ("Revert "xen/arm: Add cmdline boot option "pci-passthrough = <boolean>""")
> ---
>  xen/arch/arm/domain_build.c    |  2 +-
>  xen/arch/arm/include/asm/pci.h |  5 +----
>  xen/arch/arm/pci/pci.c         | 11 ++++++++++-
>  xen/arch/x86/include/asm/pci.h |  2 +-
>  xen/drivers/pci/physdev.c      |  4 ++--
>  xen/include/xen/pci.h          |  2 +-
>  6 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7b47abade1..fbd6db9438 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -526,7 +526,7 @@ static int __init handle_linux_pci_domain(struct 
> kernel_info *kinfo,
>      uint16_t segment;
>      int res;
>  
> -    if ( !is_pci_passthrough_enabled() )
> +    if ( !is_pci_passthrough_enabled(false) )
>          return 0;
>  
>      if ( !dt_device_type_is_equal(node, "pci") )
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 7f77226c9b..3ae85b4666 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -111,10 +111,7 @@ 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;
> -}
> +bool is_pci_passthrough_enabled(bool dom0);
>  
>  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..79bb8728a4 100644
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -16,9 +16,18 @@
>  #include <xen/device_tree.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
> +#include <xen/iommu.h>
>  #include <xen/param.h>
>  #include <xen/pci.h>
>  
> +bool is_pci_passthrough_enabled(bool dom0)
> +{
> +    if ( dom0 )
> +        return pci_passthrough_enabled || iommu_enabled;

Could this be written as:

    if ( is_hardware_domain(current->domain) )
        return pci_passthrough_enabled || iommu_enabled;

If so, then I think it would be better, if not:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



> +    return pci_passthrough_enabled;
> +}
> +
>  /*
>   * PIRQ event channels are not supported on Arm, so nothing to do.
>   */
> @@ -85,7 +94,7 @@ static int __init pci_init(void)
>       * Enable PCI passthrough when has been enabled explicitly
>       * (pci-passthrough=on).
>       */
> -    if ( !pci_passthrough_enabled )
> +    if ( !is_pci_passthrough_enabled(true) )
>          return 0;
>  
>      pci_segments_init();
> diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
> index fd5480d67d..bffeaa507d 100644
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -50,7 +50,7 @@ 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)
> +static always_inline bool is_pci_passthrough_enabled(__maybe_unused bool 
> dom0)
>  {
>      return true;
>  }
> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
> index 0161a85e1e..18448b94b3 100644
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -19,7 +19,7 @@ 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() )
> +        if ( !is_pci_passthrough_enabled(true) )
>              return -EOPNOTSUPP;
>  
>          ret = -EFAULT;
> @@ -57,7 +57,7 @@ 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() )
> +        if ( !is_pci_passthrough_enabled(true) )
>              return -EOPNOTSUPP;
>  
>          ret = -EFAULT;
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index f784e91160..c4a49cf584 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -74,7 +74,7 @@ typedef union {
>  
>  struct arch_pci_dev { };
>  
> -static inline bool is_pci_passthrough_enabled(void)
> +static inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
>  {
>      return false;
>  }
> -- 
> 2.34.1
>