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

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

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,

-- 
Julien Grall