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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2025 14:34, Mykyta Poturai wrote:
> --- 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;

As I think I said before - the function's name now no longer expresses
what it really checks. That (imo heavily) misleading at the use sites
of this function.

> +    return pci_passthrough_enabled;
> +}

Personally I also view it as undesirable that the global
pci_passthrough_enabled is evaluated twice in this function. Better
might be e.g.

bool is_pci_passthrough_enabled(bool dom0)
{
    if ( pci_passthrough_enabled )
        return true;

    return dom0 && iommu_enabled;
}

Yet then I'm not a maintainer of this code.

> @@ -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) )

There's no Dom0 in sight anywhere here, is there? How can you pass true
as argument for the "dom0" parameter?

> --- 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)

Function parmeters don't need such annotation.

> --- 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;

Seeing the function's parameter name, how do you know it's Dom0 calling
here?

Jan