On 12/1/23 21:56, Stefano Stabellini wrote:
> On Fri, 1 Dec 2023, Roger Pau Monné wrote:
>> On Mon, Nov 13, 2023 at 05:21:13PM -0500, Stewart Hildebrand wrote:
>>> @@ -1618,6 +1630,14 @@ int iommu_do_pci_domctl(
>>> bus = PCI_BUS(machine_sbdf);
>>> devfn = PCI_DEVFN(machine_sbdf);
>>>
>>> + if ( needs_vpci(d) && !has_vpci(d) )
>>> + {
>>> + printk(XENLOG_G_WARNING "Cannot assign %pp to %pd: vPCI
>>> support not enabled\n",
>>> + &PCI_SBDF(seg, bus, devfn), d);
>>> + ret = -EPERM;
>>> + break;
>>
>> I think this is likely too restrictive going forward. The current
>> approach is indeed to enable vPCI on a per-domain basis because that's
>> how PVH dom0 uses it, due to being unable to use ioreq servers.
>>
>> If we start to expose vPCI suport to guests the interface should be on
>> a per-device basis, so that vPCI could be enabled for some devices,
>> while others could still be handled by ioreq servers.
>>
>> We might want to add a new flag to xen_domctl_assign_device (used by
>> XEN_DOMCTL_assign_device) in order to signal whether the device will
>> use vPCI.
>
> Actually I don't think this is a good idea. I am all for flexibility but
> supporting multiple different configurations comes at an extra cost for
> both maintainers and contributors. I think we should try to reduce the
> amount of configurations we support rather than increasing them
> (especially on x86 where we have PV, PVH, HVM).
>
> I don't think we should enable IOREQ servers to handle PCI passthrough
> for PVH guests and/or guests with vPCI. If the domain has vPCI, PCI
> Passthrough can be handled by vPCI just fine. I think this should be a
> good anti-feature to have (a goal to explicitly not add this feature) to
> reduce complexity. Unless you see a specific usecase to add support for
> it?
Just to preemptively clarify: there is a use case for passthrough (vPCI) and
emulated virtio-pci devices (ioreq). However, the XEN_DOMCTL_assign_device
hypercall, where this check is added, is only used for real physical hardware
devices as far as I can tell. So I agree, I can't see a use case for passing
through some physical devices with vPCI and some physical devices with
qemu/ioreq.
With that said, the plumbing for a new flag does not appear to be particularly
complex. It may actually be simpler than introducing a per-arch needs_vpci(d)
heuristic.
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 96cb4da0794e..2c38088a4772 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1113,6 +1113,7 @@ typedef struct pci_add_state {
libxl_device_pci pci;
libxl_domid pci_domid;
int retries;
+ bool vpci;
} pci_add_state;
static void pci_add_qemu_trad_watch_state_cb(libxl__egc *egc,
@@ -1176,6 +1177,10 @@ static void do_pci_add(libxl__egc *egc,
}
}
+ if (type == LIBXL_DOMAIN_TYPE_PVH /* includes Arm guests */) {
+ pas->vpci = true;
+ }
+
rc = 0;
out:
@@ -1418,7 +1423,8 @@ static void pci_add_dm_done(libxl__egc *egc,
unsigned long long start, end, flags, size;
int irq, i;
int r;
- uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
+ uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED |
+ (pas->vpci ? XEN_DOMCTL_DEV_USES_VPCI : 0);
uint32_t domainid = domid;
bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 2203725a2aa6..7786da1cf1e6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1630,7 +1630,7 @@ int iommu_do_pci_domctl(
bus = PCI_BUS(machine_sbdf);
devfn = PCI_DEVFN(machine_sbdf);
- if ( needs_vpci(d) && !has_vpci(d) )
+ if ( (flags & XEN_DOMCTL_DEV_USES_VPCI) && !has_vpci(d) )
{
printk(XENLOG_G_WARNING "Cannot assign %pp to %pd: vPCI support
not enabled\n",
&PCI_SBDF(seg, bus, devfn), d);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 8b3ea62cae06..5735d47219bc 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -527,6 +527,7 @@ struct xen_domctl_assign_device {
uint32_t dev; /* XEN_DOMCTL_DEV_* */
uint32_t flags;
#define XEN_DOMCTL_DEV_RDM_RELAXED 1 /* assign only */
+#define XEN_DOMCTL_DEV_USES_VPCI (1 << 1)
union {
struct {
uint32_t machine_sbdf; /* machine PCI ID of assigned device */
