Re: [libvirt PATCH v2 10/15] xen: explicitly set hostdev driver.type at runtime, not in postparse

On 11/27/23 10:12 AM, Peter Krempa wrote: > 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. I don't think there was any intentional "lock in the default setting in the config to preserve ABI" in this case - it all seems to just be convenience/serendipity. From what I can see in the original code adding "PCI passthrough" to the libxl driver, they were just filling in the driver name in the xml because 1) it was there, and 2) they had just split out some of the code from qemu into "common code" to be used by both libxl and QEMU, needed a way to specify which driver was calling into this common code, saw the driver name as a convenient way to do it, and noticed that lots of other values that weren't set by the user were being set in the postparse, so that's where they set it in their code. (BTW, there was code in the original commit adding PCI passthrough to libxl (commit 6225cb3df5a5732868719462f0a602ef11f7fa03) that logged an error if the driver name was anything other than empty (in which case it was set to "xen") or "xen". That code is still there today (see libxlNodeDeviceDetachFlags), so the Xen driver definiteoy only supports the one type of PCI device assignment.) > > This commit message doesn't make an argument why the change is needed, > so I'm a bit reluctant... Well... the less use of the old usage of <driver name> there is, the better, and preventing it from being written into every single new config file with a <hostdev> means less use. Also this change makes the libxl driver more consistent with the behavior of the qemu driver (which has never set an explicit default in the postparse - it waits until the devices are being initialized for domain startup/hotplug before it sets the driver type (and it also always sets it to the same value). I suppose I could omit this patch, but I think it's just perpetuating unintentional code that is inconsistent with the other driver (qemu) which serves to make it more difficult for a newcomer to understand what's going on with the <driver name/type> and what is the proper way to handle it in the case of some new hypervisor driver that decides to support <hostdev>. > > > > 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(a)redhat.com&gt; > > --- > > 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. But is that the case? (that the default must never change, I mean) It's only going to make a difference if libxl adds support for some other type of PCI device assignment *and* there is something in that new type of device assignment that changes the guest ABI in an incompatible way. (I'm not certain, but I'm guessing that Xen doesn't live migration of domains with an assigned PCI device, so likely any guest would need to be fully shut down to move from the existing type of device assignment to some hypothetical new type that may exist at some time in the future. At the time that VFIO device assignment first became available, there was no <driver name='kvm'/> in any existing config, and no existing management app had a setting for the type of device assignment. This turned out to be a very *good* thing, because it meant that everyone began using VFIO device assignment by default immediately even on existing configs (and the newly-introduced <driver name='kvm|vfio'/> was only used if someone found a problem in VFIO and needed to temporarily switch back to KVM; fortunately I don't think anyone ever did). If we had instead required an explicit change to config for anyone to switch to using VFIO device assignment instead of legacy KVM device assignment (in addition to also requiring the management applications to add a knob to turn it on) it surely would have taken *years* longer for VFIO to be fully accepted/adopted, and it would have been a correspondingly longer time before the legacy KVM device assignment code could be deprecated and then finally removed from the kernel and qemu. Anyway, I don't see an advantage to explicitly writing into the config the value of a knob that only has one valid setting. If, at some point in the future, it turns out that libxl adds some new type of device assignment, and that new type of device assignement is incompatible with the current device assignment in such a way that they don't want users to automatically begin using it, then at that time it can just be explicitly stated that "not having the driver type set in the config means that the type is "legacy xen". But maybe that *won't* be needed, and it will be totally okay for everyone to just automatically start using this hypothetical "new type of device assignment for libxl"; why make the decision now that we shouldn't allow that? So have I convinced you, or should I drop it?