On Mon, Nov 06, 2023 at 02:38:55 -0500, Laine Stump wrote:
> Xen only supports a single type of PCI hostdev assignment, so it is
> superfluous to have <driver name='xen'/> peppered throughout the
> config. It *is* necessary to have the driver type explicitly set in
> the hosdev object before calling into the hypervisor-agnostic "hostdev
> manager" though (otherwise the hostdev manager doesn't know whether it
> should do Xen-specific setup, or VFIO-specific setup).
>
> Historically, the Xen driver has checked for "default" driver type
> (i.e. not set in the XML), and set it to "xen', during the XML
> postparse, thus guaranteeing that it will be set by the time the
> object is sent to the hostdev manager at runtime, but also setting it
> so early that a simple round-trip of parse-format results in the XML
> always containing an explicit <driver name='xen'/>, even if that
> wasn't specified in the original XML.
Generally stuff that is written to the definition at parse time is done
so that it doesn't change in the future, thus preserving ABI of machines
created with a config where the default was not entered yet.
This commit message doesn't make an argument why the change is needed,
so I'm a bit reluctant...
> The QEMU driver *doesn't* set driver.type during postparse though;
> instead, it waits until domain startup time (or device attach time for
> hotplug), and sets the driver.type then. The result is that a
> parse-format round trip of the XML in the QEMU driver *doesn't* add in
> the <driver name='vfio'/>.
>
> This patch modifies the Xen code to behave similarly to the QEMU code
> - the PostParse just checks for a driver.type that isn't supported by
> the Xen driver, and any explicit setting to "xen" is deferred until domain
> runtime rather than during the postparse.
>
> This delayed setting of driver.type of course results in slightly
> different xml2xml parse-format results, so the unit test data is
> modified accordingly.
>
> Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> ---
> src/libxl/libxl_domain.c | 65 +++++++++++++++----
> src/libxl/libxl_driver.c | 25 ++++---
> tests/libxlxml2domconfigdata/moredevs-hvm.xml | 1 -
> tests/xlconfigdata/test-fullvirt-pci.xml | 2 -
> tests/xmconfigdata/test-pci-dev-syntax.xml | 2 -
> tests/xmconfigdata/test-pci-devs.xml | 2 -
> 6 files changed, 69 insertions(+), 28 deletions(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 22482adfa6..ecdf57e655 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -160,8 +160,15 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev,
>
> if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> - pcisrc->driver.type == VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_DEFAULT)
> - pcisrc->driver.type = VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_XEN;
> + pcisrc->driver.type != VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_DEFAULT &&
> + pcisrc->driver.type != VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_XEN) {
> +
> + /* Xen only supports "Xen" style of hostdev, of course! */
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("XEN does not support device assignment mode '%1$s'"),
> + virDeviceHostdevPCIDriverTypeToString(pcisrc->driver.type));
> + return -1;
> + }
> }
>
> if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) {
> @@ -986,18 +993,9 @@ libxlNetworkPrepareDevices(virDomainDef *def)
> if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> /* Each type='hostdev' network device must also have a
> - * corresponding entry in the hostdevs array. For netdevs
> - * that are hardcoded as type='hostdev', this is already
> - * done by the parser, but for those allocated from a
> - * network / determined at runtime, we need to do it
> - * separately.
> + * corresponding entry in the hostdevs array.
> */
> virDomainHostdevDef *hostdev = virDomainNetGetActualHostdev(net);
> - virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci;
> -
> - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> - pcisrc->driver.type = VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_XEN;
>
> if (virDomainHostdevInsert(def, hostdev) < 0)
> return -1;
> @@ -1007,6 +1005,46 @@ libxlNetworkPrepareDevices(virDomainDef *def)
> return 0;
> }
>
> +
> +static int
> +libxlHostdevPrepareDevices(virDomainDef *def)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->nhostdevs; i++) {
> + virDomainHostdevDef *hostdev = def->hostdevs[i];
> + virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci;
> +
> + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> + pcisrc->driver.type == VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_DEFAULT) {
> +
... and here I don't think it's obvious enough that the default must
never change.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
