3:29 a.m.
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
diff to v1:
- Reworked patch 3/3
Michal Prívozník (3):
domain_validate: Refuse VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED
qemuDomainAttachHostPCIDevice: Handle hostevs with unassigned type of
address
qemuDomainDetachDeviceLive: Handle hostevs with unassigned type of
address
src/conf/domain_validate.c | 45 ++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_hotplug.c | 11 ++++++++++
2 files changed, 56 insertions(+)
--
2.34.1
3:29 a.m.
New subject: [PATCH v2 1/3] domain_validate: Refuse VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED
We document that <address type='unassigned'/> can be used only
for <hostdev/>-s. However, corresponding validation rule is
missing. Let's put the rule into hypervisor agnostic part of
validation process so that all drivers can benefit.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
---
src/conf/domain_validate.c | 45 ++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index e9baf1d41a..d5e74db243 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2257,10 +2257,55 @@ virDomainGraphicsDefValidate(const virDomainDef *def,
return 0;
}
+static int
+virDomainDeviceInfoValidate(const virDomainDeviceDef *dev)
+{
+ virDomainDeviceInfo *info;
+
+ if (!(info = virDomainDeviceGetInfo(dev)))
+ return 0;
+
+ switch ((virDomainDeviceAddressType) info->type) {
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
+ /* No validation for these address types yet */
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED:
+ if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("address of type '%s' is supported only for
hostdevs"),
+ virDomainDeviceAddressTypeToString(info->type));
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
+ default:
+ virReportEnumRangeError(virDomainDeviceAddressType, info->type);
+ return -1;
+ }
+
+ return 0;
+}
+
static int
virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
const virDomainDef *def)
{
+ if (virDomainDeviceInfoValidate(dev) < 0)
+ return -1;
+
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
return virDomainDiskDefValidate(def, dev->data.disk);
--
2.34.1
3:29 a.m.
New subject: [PATCH v2 2/3] qemuDomainAttachHostPCIDevice: Handle hostevs with unassigned type of address
A <hostdev/> can have <address type='unassigned'/> which means
libvirt manages the device detach from/reattach to the host but
the device is never exposed to the guest. This means that we have
to take a shortcut during hotplug, similar to the one we are
taking when constructing the command line (see
qemuBuildHostdevCommandLine()).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2040548
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f36de2385a..b998b51f5a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1668,6 +1668,12 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver,
qemuDomainFillDeviceIsolationGroup(vm->def, &dev);
}
+ if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED) {
+ /* Unassigned devices are not exposed to QEMU. Our job is done here. */
+ ret = 0;
+ goto done;
+ }
+
if (qemuDomainEnsurePCIAddress(vm, &dev) < 0)
goto error;
releaseaddr = true;
@@ -1692,6 +1698,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver,
exit_monitor:
qemuDomainObjExitMonitor(driver, vm);
+ done:
virDomainAuditHostdev(vm, hostdev, "attach", ret == 0);
if (ret < 0)
goto error;
--
2.34.1
3:29 a.m.
New subject: [PATCH v2 3/3] qemuDomainDetachDeviceLive: Handle hostevs with unassigned type of address
A <hostdev/> can have <address type='unassigned'/> which means
libvirt manages the device detach from/reattach to the host but
the device is never exposed to the guest. This means that we have
to take a shortcut during hotunplug (e.g. never ask QEMU on the
monitor to detach the device, or never wait for DEVICE_DELETED
event).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b998b51f5a..31141cf7df 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6219,6 +6219,10 @@ qemuDomainDetachDeviceLive(virDomainObj *vm,
info->addr.pci.slot, info->addr.pci.function);
return -1;
}
+ } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED) {
+ /* Unassigned devices are not exposed to QEMU, so remove the device
+ * explicitly, just like if we received DEVICE_DELETED event.*/
+ return qemuDomainRemoveDevice(driver, vm, &detach);
}
/*
--
2.34.1
6:46 a.m.
New subject: [PATCH v2 3/3] qemuDomainDetachDeviceLive: Handle hostevs with unassigned type of address
s/hostevs/hostdevs/ in the commit summary
On a Wednesday in 2022, Michal Privoznik wrote:
A <hostdev/> can have <address type='unassigned'/>
which means
libvirt manages the device detach from/reattach to the host but
the device is never exposed to the guest. This means that we have
to take a shortcut during hotunplug (e.g. never ask QEMU on the
monitor to detach the device, or never wait for DEVICE_DELETED
event).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 4 ++++
1 file changed, 4 insertions(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
7:57 a.m.
On 1/26/22 06:29, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
Back when I've added this UNASSIGNED address type I was also pursuing a PCI
multifunction
hotplug/unplug feature in Libvirt and, apparently, I didn't mind testing
hotplug/unplug
of non-multifunction unassigned hostdevs like the bug mentioned in patch 1 did.
Thanks for fixing this up.
Daniel
diff to v1:
- Reworked patch 3/3
Michal Prívozník (3):
domain_validate: Refuse VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED
qemuDomainAttachHostPCIDevice: Handle hostevs with unassigned type of
address
qemuDomainDetachDeviceLive: Handle hostevs with unassigned type of
address
src/conf/domain_validate.c | 45 ++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_hotplug.c | 11 ++++++++++
2 files changed, 56 insertions(+)
8:05 a.m.
On 1/27/22 14:57, Daniel Henrique Barboza wrote:
On 1/26/22 06:29, Michal Privoznik wrote:
> v2 of:
>
> https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
>
>
> Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
Back when I've added this UNASSIGNED address type I was also pursuing a
PCI multifunction
hotplug/unplug feature in Libvirt and, apparently, I didn't mind testing
hotplug/unplug
of non-multifunction unassigned hostdevs like the bug mentioned in patch
1 did.
Thanks for fixing this up.
yeah, this is more of a workaround than a real fix, because IIUC
unassigned PCI address makes sense for devices within one IOMMU group. I
mean, you want to passthrough one specific PCI device but because it's
in IOMMU group with another one, both have to be detached from the host.
Well, and for that we would need the hotplug/hotunplug API understand
that and attach/detach two or more devices at once. With my patches you
can still hotplug/hotunplug just a single device, but hey, at least we
don't error out!
Michal
8:16 a.m.
On 1/27/22 11:05, Michal Prívozník wrote:
On 1/27/22 14:57, Daniel Henrique Barboza wrote:
>
>
> On 1/26/22 06:29, Michal Privoznik wrote:
>> v2 of:
>>
>> https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
>>
>>
>> Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
>
>
> Back when I've added this UNASSIGNED address type I was also pursuing a
> PCI multifunction
> hotplug/unplug feature in Libvirt and, apparently, I didn't mind testing
> hotplug/unplug
> of non-multifunction unassigned hostdevs like the bug mentioned in patch
> 1 did.
>
>
> Thanks for fixing this up.
yeah, this is more of a workaround than a real fix, because IIUC
unassigned PCI address makes sense for devices within one IOMMU group. I
mean, you want to passthrough one specific PCI device but because it's
in IOMMU group with another one, both have to be detached from the host.
Yes. The context where I added this UNASSIGNED address type was to allow a whole PCI
multifunction card to be managed by Libvirt, which would detach all the functions from
the IOMMU grupo of the host and, at the same time, filter which functions would be
visible
to the guest.
I didn't foresee this usage back then.
Well, and for that we would need the hotplug/hotunplug API understand
that and attach/detach two or more devices at once. With my patches you
can still hotplug/hotunplug just a single device, but hey, at least we
don't error out!
You could also just forbid hotplug of UNASSIGNED non-multifunction hostdevs.
I can't think of an actual use of detaching a single hostdev from the host and doing
nothing
with it. If the idea is to refrain the host from using the hostdev the user can just
nodedev-detach it.
Matter of fact, I'd say that the 'UNASSIGNED' address type makes little sense
for
non-multifuction devices altogether.
Thanks,
Daniel
Michal
1031
days inactive
1032
days old
7 comments
4 participants
participants (4)
-
Daniel Henrique Barboza -
Ján Tomko -
Michal Privoznik -
Michal Prívozník