From: Stewart Hildebrand <stewart.hildebrand@amd.com>
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@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
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;
+
+ 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.1Hi Mykyta, On 14/03/2025 13:34, Mykyta Poturai wrote: > From: Stewart Hildebrand <stewart.hildebrand@amd.com> > > 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. It would be good to explain why Xen cannot initialize the PCI controller. Asking, because the reason is the PCI controller is too complex, then you will likely need the same approach for PCI passthrough... > > Enable pci_init() for initializing Xen's internal PCI subsystem, and > allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled. Effectively, wouldn't this mean dom0 always *have* to call PHYSDEVOP_pci_device_add? Otherwise, how would dom0 know whether it needs to call PHSYDEVOP_pci_device_add? Cheers, -- Julien Grall
On 28.04.25 11:54, Julien Grall wrote: > Hi Mykyta, > > On 14/03/2025 13:34, Mykyta Poturai wrote: >> From: Stewart Hildebrand <stewart.hildebrand@amd.com> >> >> 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. > > It would be good to explain why Xen cannot initialize the PCI > controller. Asking, because the reason is the PCI controller is too > complex, then you will likely need the same approach for PCI passthrough... I think the main reason for this is complexity and the possibility of additional dependencies: there could be external clocks or reset pins that the PCI host depends on for working correctly. I will add this to the commit message. Regarding PCI passthrough, it is already using the same approach (at least on Arm). There are patches for enabling Xen on Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't yet got to test them in a meaningful way. >> >> Enable pci_init() for initializing Xen's internal PCI subsystem, and >> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled. > > Effectively, wouldn't this mean dom0 always *have* to call > PHYSDEVOP_pci_device_add? Otherwise, how would dom0 know whether it > needs to call PHSYDEVOP_pci_device_add? > > Cheers, > Yes, I can't say for every system but with PCI host behind SMMU the PHYSDEVOP_pci_device_add call is required to use DMA. -- Mykyta
Hi, On 28/04/2025 13:31, Mykyta Poturai wrote: > On 28.04.25 11:54, Julien Grall wrote: >> Hi Mykyta, >> >> On 14/03/2025 13:34, Mykyta Poturai wrote: >>> From: Stewart Hildebrand <stewart.hildebrand@amd.com> >>> >>> 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. >> >> It would be good to explain why Xen cannot initialize the PCI >> controller. Asking, because the reason is the PCI controller is too >> complex, then you will likely need the same approach for PCI passthrough... > > I think the main reason for this is complexity and the possibility of > additional dependencies: there could be external clocks or reset pins > that the PCI host depends on for working correctly. I will add this to > the commit message. Regarding PCI passthrough, it is already using the > same approach (at least on Arm). There are patches for enabling Xen on > Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't > yet got to test them in a meaningful way. Can you provide a link to the series? I would like to make sure we have a coherent approach. In particular, it is not clear to me how Dom0 and Xen will be able to coordinate the access to the PCI controller. Are we going to have a mediator? > >>> >>> Enable pci_init() for initializing Xen's internal PCI subsystem, and >>> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled. >> >> Effectively, wouldn't this mean dom0 always *have* to call >> PHYSDEVOP_pci_device_add? Otherwise, how would dom0 know whether it >> needs to call PHSYDEVOP_pci_device_add? >> >> Cheers, >> > > Yes, I can't say for every system but with PCI host behind SMMU the > PHYSDEVOP_pci_device_add call is required to use DMA. Dom0 will not be able to know whether a device is protected by an IOMMU. So I guess it means the OS will need to be able to cope with an error (like on x86). Cheers, -- Julien Grall
On 28.04.25 15:55, Julien Grall wrote: > Hi, > > On 28/04/2025 13:31, Mykyta Poturai wrote: >> On 28.04.25 11:54, Julien Grall wrote: >>> Hi Mykyta, >>> >>> On 14/03/2025 13:34, Mykyta Poturai wrote: >>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com> >>>> >>>> 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. >>> >>> It would be good to explain why Xen cannot initialize the PCI >>> controller. Asking, because the reason is the PCI controller is too >>> complex, then you will likely need the same approach for PCI >>> passthrough... >> >> I think the main reason for this is complexity and the possibility of >> additional dependencies: there could be external clocks or reset pins >> that the PCI host depends on for working correctly. I will add this to >> the commit message. Regarding PCI passthrough, it is already using the >> same approach (at least on Arm). There are patches for enabling Xen on >> Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't >> yet got to test them in a meaningful way. > > Can you provide a link to the series? I would like to make sure we have > a coherent approach. In particular, it is not clear to me how Dom0 and > Xen will be able to coordinate the access to the PCI controller. Are we > going to have a mediator? > Here is a link to my WIP branch https://github.com/Deedone/xen/tree/pci_passthrough_wip Although it is slightly outdated due to recent review activity, I will updated it soon with all of the recent changes. Luca's commits begin from c68a5cbb1a7d17907551159c99ab5c87ce0dedf9 I wasn't able to find them sent to any mailing lists, but I plan to also send them after the base case with Dom0 enumeration stabilizes. There is no mediator, Dom0 configures the host controller, enumerates child devices, and then gives complete access to some of them to Xen. Xen only does "logical" operations with child devices and does not change any of the host controller base settings. To ensure that Dom0 will not mess with the child devices, tools bind them to the stub driver. Theoretically, Dom0 can bind to those devices again and break something, but it can also do a lot of other breaking stuff so I don't think there is a good reason to invent some forms of protection from it. After some time with pci-scan changes merged it should become possible to make Xen do the enumeration, and only give Dom0 the virtual PCI bus, which would prevent it from accessing non-owned devices. -- Mykyta
Hi Mykyta, On 28/04/2025 15:28, Mykyta Poturai wrote: > On 28.04.25 15:55, Julien Grall wrote: >> Hi, >> >> On 28/04/2025 13:31, Mykyta Poturai wrote: >>> On 28.04.25 11:54, Julien Grall wrote: >>>> Hi Mykyta, >>>> >>>> On 14/03/2025 13:34, Mykyta Poturai wrote: >>>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com> >>>>> >>>>> 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. >>>> >>>> It would be good to explain why Xen cannot initialize the PCI >>>> controller. Asking, because the reason is the PCI controller is too >>>> complex, then you will likely need the same approach for PCI >>>> passthrough... >>> >>> I think the main reason for this is complexity and the possibility of >>> additional dependencies: there could be external clocks or reset pins >>> that the PCI host depends on for working correctly. I will add this to >>> the commit message. Regarding PCI passthrough, it is already using the >>> same approach (at least on Arm). There are patches for enabling Xen on >>> Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't >>> yet got to test them in a meaningful way. >> >> Can you provide a link to the series? I would like to make sure we have >> a coherent approach. In particular, it is not clear to me how Dom0 and >> Xen will be able to coordinate the access to the PCI controller. Are we >> going to have a mediator? >> > > Here is a link to my WIP branch > https://github.com/Deedone/xen/tree/pci_passthrough_wip > Although it is slightly outdated due to recent review activity, I will > updated it soon with all of the recent changes. > > Luca's commits begin from c68a5cbb1a7d17907551159c99ab5c87ce0dedf9 > > I wasn't able to find them sent to any mailing lists, but I plan to also > send them after the base case with Dom0 enumeration stabilizes. I don't think you can stabilize one without the other. I am worry the interface may not work properly for PCI passthrough. So until then, I would say this should be marked as unsupported (maybe protected by CONFIG_UNSUPPORTED). > > There is no mediator, Dom0 configures the host controller, enumerates > child devices, and then gives complete access to some of them to Xen. > Xen only does "logical" operations with child devices and does not > change any of the host controller base settings. I am not sure I fully understanding this. Both dom0 will need to access the configuration space. So you would need to ensure there is only one accessing the configuration space at a give time. > To ensure that Dom0 > will not mess with the child devices, tools bind them to the stub > driver. Theoretically, Dom0 can bind to those devices again and break > something, but it can also do a lot of other breaking stuff so I don't > think there is a good reason to invent some forms of protection from it. We should not trust dom0 to do the right thing. But reading ... > > After some time with pci-scan changes merged it should become possible > to make Xen do the enumeration, and only give Dom0 the virtual PCI bus, > which would prevent it from accessing non-owned devices. ... this it sounds like this would be temporary. Do you have patches already on the mailing list? Cheers, -- Julien Grall
On 28.04.25 21:15, Julien Grall wrote: > Hi Mykyta, > > On 28/04/2025 15:28, Mykyta Poturai wrote: >> On 28.04.25 15:55, Julien Grall wrote: >>> Hi, >>> >>> On 28/04/2025 13:31, Mykyta Poturai wrote: >>>> On 28.04.25 11:54, Julien Grall wrote: >>>>> Hi Mykyta, >>>>> >>>>> On 14/03/2025 13:34, Mykyta Poturai wrote: >>>>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com> >>>>>> >>>>>> 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. >>>>> >>>>> It would be good to explain why Xen cannot initialize the PCI >>>>> controller. Asking, because the reason is the PCI controller is too >>>>> complex, then you will likely need the same approach for PCI >>>>> passthrough... >>>> >>>> I think the main reason for this is complexity and the possibility of >>>> additional dependencies: there could be external clocks or reset pins >>>> that the PCI host depends on for working correctly. I will add this to >>>> the commit message. Regarding PCI passthrough, it is already using the >>>> same approach (at least on Arm). There are patches for enabling Xen on >>>> Arm to perform bus enumeration by itself by Luca Fancellu, but I >>>> haven't >>>> yet got to test them in a meaningful way. >>> >>> Can you provide a link to the series? I would like to make sure we have >>> a coherent approach. In particular, it is not clear to me how Dom0 and >>> Xen will be able to coordinate the access to the PCI controller. Are we >>> going to have a mediator? >>> >> >> Here is a link to my WIP branch >> https://github.com/Deedone/xen/tree/pci_passthrough_wip >> Although it is slightly outdated due to recent review activity, I will >> updated it soon with all of the recent changes. >> >> Luca's commits begin from c68a5cbb1a7d17907551159c99ab5c87ce0dedf9 >> >> I wasn't able to find them sent to any mailing lists, but I plan to also >> send them after the base case with Dom0 enumeration stabilizes. > > I don't think you can stabilize one without the other. I am worry the > interface may not work properly for PCI passthrough. So until then, I > would say this should be marked as unsupported (maybe protected by > CONFIG_UNSUPPORTED). It is currently not possible to enable HAS_PCI on Arm at all. I will add the UNSUPPORTED guard along with the future changes to Kconfig that will make enabling HAS_PCI on Arm possible. >> >> There is no mediator, Dom0 configures the host controller, enumerates >> child devices, and then gives complete access to some of them to Xen. >> Xen only does "logical" operations with child devices and does not >> change any of the host controller base settings. > > I am not sure I fully understanding this. Both dom0 will need to access > the configuration space. So you would need to ensure there is only one > accessing the configuration space at a give time. > If a controller doesn't require specific configuration for mapping child bus then Xen should access only child's config space and if Dom0 ignores this device there will be no conflicts. For more complex controllers like the ones on R-Car boards it is possible to implement safer ways of mapping config space via Xen, example patch here[1] as temporary solution. >> To ensure that Dom0 >> will not mess with the child devices, tools bind them to the stub >> driver. Theoretically, Dom0 can bind to those devices again and break >> something, but it can also do a lot of other breaking stuff so I don't >> think there is a good reason to invent some forms of protection from it. > > We should not trust dom0 to do the right thing. But reading ... > >> >> After some time with pci-scan changes merged it should become possible >> to make Xen do the enumeration, and only give Dom0 the virtual PCI bus, >> which would prevent it from accessing non-owned devices. > > ... this it sounds like this would be temporary. Do you have patches > already on the mailing list? > > Cheers, > Not yet, I am planning to start working on them after dealing with MSI support on Arm. [1]: https://github.com/xen-troops/meta-xt-prod-devel-rcar-gen4/blob/master/layers/meta-xt-domd-gen4/recipes-kernel/linux/linux-renesas/0005-HACK-pcie-renesas-emulate-reading-from-ECAM-under-Xe.patch -- Mykyta
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>
> 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@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> 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@kernel.org>
> + 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
>
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
On 17.03.25 17:07, Jan Beulich wrote:
> 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.
Hi Jan,
I've spent some more time thinking about how to better deal with this.
In the end, I think your earlier suggestion about introducing a new arch
specific function is the best approach, but I want to agree on the
naming before sending new patches. Would "arch_requires_pci_physdev" be
an appropriate name in your opinion?
At the call sites it will look like this:
case PHYSDEVOP_pci_device_remove: {
struct physdev_pci_device dev;
if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
return -EOPNOTSUPP;
--
Mykyta
On 28.04.2025 10:21, Mykyta Poturai wrote:
> On 17.03.25 17:07, Jan Beulich wrote:
>> 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.
>
> I've spent some more time thinking about how to better deal with this.
> In the end, I think your earlier suggestion about introducing a new arch
> specific function is the best approach, but I want to agree on the
> naming before sending new patches. Would "arch_requires_pci_physdev" be
> an appropriate name in your opinion?
>
> At the call sites it will look like this:
> case PHYSDEVOP_pci_device_remove: {
> struct physdev_pci_device dev;
>
> if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
> return -EOPNOTSUPP;
There are several questions that affect naming: Is it really "requires"? Is
it really all PCI-related physdevops? Is the ordering of naming elements in
line with what we use elsewhere (arch_ first is, but perhaps either pci or
physdevop wants to move earlier)?
Jan
On 28.04.25 12:01, Jan Beulich wrote:
> On 28.04.2025 10:21, Mykyta Poturai wrote:
>> On 17.03.25 17:07, Jan Beulich wrote:
>>> 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.
>>
>> I've spent some more time thinking about how to better deal with this.
>> In the end, I think your earlier suggestion about introducing a new arch
>> specific function is the best approach, but I want to agree on the
>> naming before sending new patches. Would "arch_requires_pci_physdev" be
>> an appropriate name in your opinion?
>>
>> At the call sites it will look like this:
>> case PHYSDEVOP_pci_device_remove: {
>> struct physdev_pci_device dev;
>>
>> if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
>> return -EOPNOTSUPP;
>
> There are several questions that affect naming: Is it really "requires"? Is
> it really all PCI-related physdevops? Is the ordering of naming elements in
> line with what we use elsewhere (arch_ first is, but perhaps either pci or
> physdevop wants to move earlier)?
>
> Jan
I understand the issue with the ordering, will
"arch_pci_requires_physdev_ops" or "arch_physdev_pci_update_required" be
better? Regarding the specific ops, only add/remove are needed, but I am
not sure how to elegantly encode this in the name. Maybe you can suggest
something better if you have something specific in mind?
--
Mykyta
On 28.04.2025 13:26, Mykyta Poturai wrote:
> On 28.04.25 12:01, Jan Beulich wrote:
>> On 28.04.2025 10:21, Mykyta Poturai wrote:
>>> On 17.03.25 17:07, Jan Beulich wrote:
>>>> 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.
>>>
>>> I've spent some more time thinking about how to better deal with this.
>>> In the end, I think your earlier suggestion about introducing a new arch
>>> specific function is the best approach, but I want to agree on the
>>> naming before sending new patches. Would "arch_requires_pci_physdev" be
>>> an appropriate name in your opinion?
>>>
>>> At the call sites it will look like this:
>>> case PHYSDEVOP_pci_device_remove: {
>>> struct physdev_pci_device dev;
>>>
>>> if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
>>> return -EOPNOTSUPP;
>>
>> There are several questions that affect naming: Is it really "requires"? Is
>> it really all PCI-related physdevops? Is the ordering of naming elements in
>> line with what we use elsewhere (arch_ first is, but perhaps either pci or
>> physdevop wants to move earlier)?
>
> I understand the issue with the ordering, will
> "arch_pci_requires_physdev_ops" or "arch_physdev_pci_update_required" be
> better? Regarding the specific ops, only add/remove are needed, but I am
> not sure how to elegantly encode this in the name. Maybe you can suggest
> something better if you have something specific in mind?
Simply arch_pci_device_physdevop()? This would also avoid the question if
it's "requires", "wants", or yet something else. (I'm not going to insist
that the verb be omitted, though. If it's included, I'd ask though that
it match in tense the "enabled" in the other predicate.) And it would cover
PHYSDEVOP_pci_device_reset as well, which sooner or later I expect you'll
discover you want/need Dom0 to issue, too.
Jan
On 17.03.25 17:07, Jan Beulich wrote:
> 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.
I am afraid I don't understand your concern. It still checks if PCI
passthrough is enabled. With just the change that ARM needs some extra
logic for Dom0 PCI to work properly. Maybe change the parameter name to
something like "for_pci_hwdom"?
>> + 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.
>
Agree, will change in the next version.
>> @@ -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?
>
This should be read as "is pci passthrough enabled for Dom0?" and if it
is we definitely need to do a PCI init.
I've also done some investigations on possible ways to remove the
Dom0/other domains distinction, but I'm afraid this is the most
reasonable way to make PCI functional on Dom0 without explicitly
enabling PCI passthrough.
SMMU is configured to trigger a fault on all transactions by default and
we can't statically map PCI devices to Dom0, so the only other way is to
put PCI in full passthrough mode, which I think is not safe enough.
And we also can't drop this patch as it was directly requested by Julien
here [1].
>> --- 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.
>
Got it.
>> --- 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
Is this a functional or naming concern? If it is about naming then can
it also be solved by renaming the parameter?
Regarding functional issues, I have assumed that only hwdom can make
physdev operations, but after checking it, this assumption seems to be
correct on x86 but wrong on Arm.
I expected there would be a check in do_arm_physdev_op() or somewhere
near it, similar to how it is done in x86, but there are none. I'm not
sure if it is intentional or by mistake, I think it needs some
clarification by Arm folks.
[1]
https://patchew.org/Xen/20230607030220.22698-1-stewart.hildebrand@amd.com/#2731b06d-4a54-f51c-2285-ea2cf1fac3ba@xen.org
--
Mykyta
On 21.03.2025 11:56, Mykyta Poturai wrote:
> On 17.03.25 17:07, Jan Beulich wrote:
>> 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.
>
> I am afraid I don't understand your concern. It still checks if PCI
> passthrough is enabled. With just the change that ARM needs some extra
> logic for Dom0 PCI to work properly.
Conceptually there's no such thing as "pass through" for Dom0. Hence the
name of the function itself isn't correctly reflecting what it's checking
for. iommu_enabled is a prereq for pass-through to be enabled, but it
doesn't imply that's necessarily the case.
> Maybe change the parameter name to
> something like "for_pci_hwdom"?
That may help below, yes. But not here.
> >>> @@ -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?
>
> This should be read as "is pci passthrough enabled for Dom0?" and if it
> is we definitely need to do a PCI init.
>
> I've also done some investigations on possible ways to remove the
> Dom0/other domains distinction, but I'm afraid this is the most
> reasonable way to make PCI functional on Dom0 without explicitly
> enabling PCI passthrough.
>
> SMMU is configured to trigger a fault on all transactions by default and
> we can't statically map PCI devices to Dom0, so the only other way is to
> put PCI in full passthrough mode, which I think is not safe enough.
> And we also can't drop this patch as it was directly requested by Julien
> here [1].
>
>>> --- 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?
>
> Is this a functional or naming concern? If it is about naming then can
> it also be solved by renaming the parameter?
The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom"
or anything alike is a good parameter name is a different question.
> Regarding functional issues, I have assumed that only hwdom can make
> physdev operations, but after checking it, this assumption seems to be
> correct on x86 but wrong on Arm.
> I expected there would be a check in do_arm_physdev_op() or somewhere
> near it, similar to how it is done in x86, but there are none. I'm not
> sure if it is intentional or by mistake, I think it needs some
> clarification by Arm folks.
Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there
either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make
use of.
Jan
On 21.03.25 15:41, Jan Beulich wrote:
> On 21.03.2025 11:56, Mykyta Poturai wrote:
>> On 17.03.25 17:07, Jan Beulich wrote:
>>> 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.
>>
>> I am afraid I don't understand your concern. It still checks if PCI
>> passthrough is enabled. With just the change that ARM needs some extra
>> logic for Dom0 PCI to work properly.
>
> Conceptually there's no such thing as "pass through" for Dom0. Hence the
> name of the function itself isn't correctly reflecting what it's checking
> for. iommu_enabled is a prereq for pass-through to be enabled, but it
> doesn't imply that's necessarily the case.
Okay, now I think I get it. Yes from that point of view it seems kind of
wrong. Maybe use a separate function then, something like "hwdom_has_vpci"?
>> Maybe change the parameter name to
>> something like "for_pci_hwdom"?
>
> That may help below, yes. But not here.
>
>>>>> @@ -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?
>>
>> This should be read as "is pci passthrough enabled for Dom0?" and if it
>> is we definitely need to do a PCI init.
>>
>> I've also done some investigations on possible ways to remove the
>> Dom0/other domains distinction, but I'm afraid this is the most
>> reasonable way to make PCI functional on Dom0 without explicitly
>> enabling PCI passthrough.
>>
>> SMMU is configured to trigger a fault on all transactions by default and
>> we can't statically map PCI devices to Dom0, so the only other way is to
>> put PCI in full passthrough mode, which I think is not safe enough.
>> And we also can't drop this patch as it was directly requested by Julien
>> here [1].
>>
>>>> --- 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?
>>
>> Is this a functional or naming concern? If it is about naming then can
>> it also be solved by renaming the parameter?
>
> The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom"
> or anything alike is a good parameter name is a different question.
>
>> Regarding functional issues, I have assumed that only hwdom can make
>> physdev operations, but after checking it, this assumption seems to be
>> correct on x86 but wrong on Arm.
>> I expected there would be a check in do_arm_physdev_op() or somewhere
>> near it, similar to how it is done in x86, but there are none. I'm not
>> sure if it is intentional or by mistake, I think it needs some
>> clarification by Arm folks.
>
> Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there
> either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make
> use of.
>
> Jan
It is one level above in hvm_physdev_op()
case PHYSDEVOP_setup_gsi:
case PHYSDEVOP_pci_mmcfg_reserved:
case PHYSDEVOP_pci_device_add:
case PHYSDEVOP_pci_device_remove:
case PHYSDEVOP_pci_device_reset:
case PHYSDEVOP_dbgp_op:
if ( !is_hardware_domain(currd) )
return -ENOSYS;
break;
--
Mykyta
On 21.03.2025 15:50, Mykyta Poturai wrote: > On 21.03.25 15:41, Jan Beulich wrote: >> On 21.03.2025 11:56, Mykyta Poturai wrote: >>> On 17.03.25 17:07, Jan Beulich wrote: >>>> On 14.03.2025 14:34, Mykyta Poturai wrote: >>>>> --- 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? >>> >>> Is this a functional or naming concern? If it is about naming then can >>> it also be solved by renaming the parameter? >> >> The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom" >> or anything alike is a good parameter name is a different question. >> >>> Regarding functional issues, I have assumed that only hwdom can make >>> physdev operations, but after checking it, this assumption seems to be >>> correct on x86 but wrong on Arm. >>> I expected there would be a check in do_arm_physdev_op() or somewhere >>> near it, similar to how it is done in x86, but there are none. I'm not >>> sure if it is intentional or by mistake, I think it needs some >>> clarification by Arm folks. >> >> Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there >> either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make >> use of. > > It is one level above in hvm_physdev_op() > > case PHYSDEVOP_setup_gsi: > case PHYSDEVOP_pci_mmcfg_reserved: > case PHYSDEVOP_pci_device_add: > case PHYSDEVOP_pci_device_remove: > case PHYSDEVOP_pci_device_reset: > case PHYSDEVOP_dbgp_op: > if ( !is_hardware_domain(currd) ) > return -ENOSYS; > break; But that's for just HVM guests, and only a functional restriction, not a security one. Jan
© 2016 - 2026 Red Hat, Inc.