| Message ID | 20231030235240.106998-5-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Kconfig for PCI passthrough on ARM | expand |
On 31.10.2023 00:52, Stewart Hildebrand wrote: > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl( > bus = PCI_BUS(machine_sbdf); > devfn = PCI_DEVFN(machine_sbdf); > > + if ( IS_ENABLED(CONFIG_ARM) && > + !is_hardware_domain(d) && > + !is_system_domain(d) && > + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) ) I don't think you need the explicit ARM check; that's redundant with checking !HAS_VPCI_GUEST_SUPPORT. It's also not really clear why you need to check for the system domain here. > + { > + printk(XENLOG_G_WARNING "Cannot assign %pp to %pd: vPCI support not enabled\n", > + &PCI_SBDF(seg, bus, devfn), d); ret = -EPERM; (or some other suitable error indicator) Jan > + break; > + } > + > pcidevs_lock(); > ret = device_assigned(seg, bus, devfn); > if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
Hi, On 31/10/2023 11:03, Jan Beulich wrote: > On 31.10.2023 00:52, Stewart Hildebrand wrote: >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl( >> bus = PCI_BUS(machine_sbdf); >> devfn = PCI_DEVFN(machine_sbdf); >> >> + if ( IS_ENABLED(CONFIG_ARM) && >> + !is_hardware_domain(d) && >> + !is_system_domain(d) && >> + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) ) > > I don't think you need the explicit ARM check; that's redundant with > checking !HAS_VPCI_GUEST_SUPPORT. It's also not really clear why you > need to check for the system domain here. I might be missing but I wouldn't expect the domain to have vPCI enabled if CONFIG_HAVE_VPCI_GUEST_SUPPORT=n. So why can't this simply be: if ( !has_vcpi(d) ) { ... } Cheers,
On 10/31/23 09:17, Julien Grall wrote: > Hi, > > On 31/10/2023 11:03, Jan Beulich wrote: >> On 31.10.2023 00:52, Stewart Hildebrand wrote: >>> --- a/xen/drivers/passthrough/pci.c >>> +++ b/xen/drivers/passthrough/pci.c >>> @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl( >>> bus = PCI_BUS(machine_sbdf); >>> devfn = PCI_DEVFN(machine_sbdf); >>> + if ( IS_ENABLED(CONFIG_ARM) && >>> + !is_hardware_domain(d) && >>> + !is_system_domain(d) && >>> + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) ) >> >> I don't think you need the explicit ARM check; that's redundant with >> checking !HAS_VPCI_GUEST_SUPPORT. Currently that is true. However, this is allowing for the possibility that we eventually may want to enable PCI passthrough for PVH domU using vPCI (e.g. hyperlaunch, or eliminating qemu backend), in which case we may want to enable CONFIG_HAS_VPCI_GUEST_SUPPORT=y on x86. >> It's also not really clear why you >> need to check for the system domain here. xl pci-assignable-add will assign the device to domIO, which doesn't have vPCI, but it is still a valid assignment. Perhaps an in code comment would be helpful for clarity? > > I might be missing but I wouldn't expect the domain to have vPCI enabled if CONFIG_HAVE_VPCI_GUEST_SUPPORT=n. So why can't this simply be: > > if ( !has_vcpi(d) ) > { > ... > } Right, the CONFIG_HAVE_VPCI_GUEST_SUPPORT check here is not strictly needed because this case is already caught by the other half of this patch in xen/arch/arm/vpci.c. This simplifies it to: if ( IS_ENABLED(CONFIG_ARM) && !is_hardware_domain(d) && !is_system_domain(d) /* !domIO */ && !has_vpci(d) ) On x86, unless I misunderstood something, I think it's valid to assign PCI devices to a domU without has_vpci(). BTW, it's valid for has_vpci() to be true and CONFIG_HAVE_VPCI_GUEST_SUPPORT=n for dom0.
On 31.10.2023 15:15, Stewart Hildebrand wrote: > On 10/31/23 09:17, Julien Grall wrote: >> On 31/10/2023 11:03, Jan Beulich wrote: >>> On 31.10.2023 00:52, Stewart Hildebrand wrote: >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl( >>>> bus = PCI_BUS(machine_sbdf); >>>> devfn = PCI_DEVFN(machine_sbdf); >>>> + if ( IS_ENABLED(CONFIG_ARM) && >>>> + !is_hardware_domain(d) && >>>> + !is_system_domain(d) && >>>> + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) ) >>> >>> I don't think you need the explicit ARM check; that's redundant with >>> checking !HAS_VPCI_GUEST_SUPPORT. > > Currently that is true. However, this is allowing for the possibility that we eventually may want to enable PCI passthrough for PVH domU using vPCI (e.g. hyperlaunch, or eliminating qemu backend), in which case we may want to enable CONFIG_HAS_VPCI_GUEST_SUPPORT=y on x86. That's precisely why I'd like to see the ARM check go away here. >>> It's also not really clear why you >>> need to check for the system domain here. > > xl pci-assignable-add will assign the device to domIO, which doesn't have vPCI, but it is still a valid assignment. Perhaps an in code comment would be helpful for clarity? And/or specifically check for DomIO, not any system domain. Jan
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 5ff68e5d5979..3845b238a33f 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 default n help This option enables PCI device passthrough diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 3bc4bb55082a..61e0edcedea9 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -2,6 +2,7 @@ /* * xen/arch/arm/vpci.c */ +#include <xen/lib.h> #include <xen/sched.h> #include <xen/vpci.h> @@ -90,8 +91,15 @@ int domain_vpci_init(struct domain *d) return ret; } else + { + if ( !IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) + { + gdprintk(XENLOG_ERR, "vPCI requested but guest support not enabled\n"); + return -EINVAL; + } register_mmio_handler(d, &vpci_mmio_handler, GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); + } return 0; } diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 04d00c7c37df..bbdc926eda2c 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl( bus = PCI_BUS(machine_sbdf); devfn = PCI_DEVFN(machine_sbdf); + if ( IS_ENABLED(CONFIG_ARM) && + !is_hardware_domain(d) && + !is_system_domain(d) && + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) ) + { + printk(XENLOG_G_WARNING "Cannot assign %pp to %pd: vPCI support not enabled\n", + &PCI_SBDF(seg, bus, devfn), d); + break; + } + pcidevs_lock(); ret = device_assigned(seg, bus, devfn); if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
Select HAS_VPCI_GUEST_SUPPORT in Kconfig for enabling vPCI support for domUs. Add checks to fail guest creation if the configuration is invalid. 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. I'll coordinate with Volodymyr to include this in the vPCI series or resend afterwards. Meanwhile, I'll include it here until the Kconfig and xen_arch_domainconfig prerequisites have been committed. v3->v4: * refuse to create domain if configuration is invalid * split toolstack change into separate patch v2->v3: * set pci flags in toolstack v1->v2: * new patch [1] https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg02361.html --- xen/arch/arm/Kconfig | 1 + xen/arch/arm/vpci.c | 8 ++++++++ xen/drivers/passthrough/pci.c | 10 ++++++++++ 3 files changed, 19 insertions(+)