Add support for an "iommufd" virDomainIOMMUDef member that will be
translated to a qemu "-object iommufd" command line argument. This
iommufd struct has an "id" member to specify the iommufd object id
that can be associated with a passthrough device, and an "fd"
member to specify the fd for externally opening the /dev/iommu and
VFIO cdev by a management layer.
Signed-off-by: Nathan Chen <nathanc@nvidia.com>
---
src/conf/domain_conf.c | 108 +++++++++++++++++++++++++++++-
src/conf/domain_conf.h | 8 +++
src/conf/domain_validate.c | 44 +++++++++++-
src/conf/schemas/domaincommon.rng | 14 ++++
src/conf/virconftypes.h | 2 +
src/qemu/qemu_command.c | 13 ++++
6 files changed, 186 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c8d481eb5c..9330c47b7a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2814,6 +2814,9 @@ virDomainIOMMUDefFree(virDomainIOMMUDef *iommu)
if (!iommu)
return;
+ if (iommu->iommufd)
+ virDomainIommufdDefFree(iommu->iommufd);
+
virDomainDeviceInfoClear(&iommu->info);
g_free(iommu);
}
@@ -14292,6 +14295,54 @@ virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt,
}
+void virDomainIommufdDefFree(virDomainIommufdDef *def)
+{
+ if (!def)
+ return;
+
+ g_free(def->id);
+
+ g_free(def->fd);
+
+ g_free(def);
+}
+
+
+static virDomainIommufdDef *
+virDomainIommufdParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt)
+{
+ virDomainIommufdDef *def;
+ g_autofree char *iommufdId = NULL;
+ g_autofree char *iommufdFd = NULL;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt)
+
+ ctxt->node = node;
+
+ def = g_new0(virDomainIommufdDef, 1);
+
+ if (!(iommufdId = virXPathString("string(./id)", ctxt)))
+ goto error;
+
+ def->id = g_strdup(iommufdId);
+
+ iommufdFd = virXPathString("string(./fd)", ctxt);
+
+ def->fd = g_strdup(iommufdFd);
+
+ if (!def->id)
+ goto error;
+
+ if (iommufdFd && !def->fd)
+ goto error;
+
+ return def;
+ error:
+ virDomainIommufdDefFree(def);
+ return NULL;
+}
+
+
static virDomainIOMMUDef *
virDomainIOMMUDefParseXML(virDomainXMLOption *xmlopt,
xmlNodePtr node,
@@ -14299,7 +14350,7 @@ virDomainIOMMUDefParseXML(virDomainXMLOption *xmlopt,
unsigned int flags)
{
VIR_XPATH_NODE_AUTORESTORE(ctxt)
- xmlNodePtr driver;
+ xmlNodePtr driver, iommufd;
g_autoptr(virDomainIOMMUDef) iommu = NULL;
ctxt->node = node;
@@ -14336,6 +14387,11 @@ virDomainIOMMUDefParseXML(virDomainXMLOption *xmlopt,
return NULL;
}
+ if ((iommufd = virXPathNode("./iommufd", ctxt))) {
+ if (!(iommu->iommufd = virDomainIommufdParseXML(iommufd, ctxt)))
+ return NULL;
+ }
+
if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt,
&iommu->info, flags) < 0)
return NULL;
@@ -16384,6 +16440,25 @@ virDomainIOMMUDefEquals(const virDomainIOMMUDef *a,
a->dma_translation != b->dma_translation)
return false;
+ if (a->iommufd && b->iommufd) {
+ if (a->iommufd->id && b->iommufd->id) {
+ if (STRNEQ(a->iommufd->id, b->iommufd->id))
+ return false;
+ } else if ((a->iommufd->id && !b->iommufd->id) ||
+ (!a->iommufd->id && b->iommufd->id)) {
+ return false;
+ }
+ if (a->iommufd->fd && b->iommufd->fd) {
+ if (STRNEQ(a->iommufd->fd, b->iommufd->fd))
+ return false;
+ } else if ((a->iommufd->fd && !b->iommufd->fd) ||
+ (!a->iommufd->fd && b->iommufd->fd)) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+
switch (a->info.type) {
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
if (a->info.addr.pci.domain != b->info.addr.pci.domain ||
@@ -22078,6 +22153,27 @@ virDomainIOMMUDefCheckABIStability(virDomainIOMMUDef *src,
virTristateSwitchTypeToString(src->dma_translation));
return false;
}
+ if (src->iommufd && dst->iommufd) {
+ if (STRNEQ(src->iommufd->id, dst->iommufd->id)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target domain IOMMU device iommufd id '%1$s' does not match source '%2$s'"),
+ dst->iommufd->id,
+ src->iommufd->id);
+ return false;
+ }
+ if (STRNEQ(src->iommufd->fd, dst->iommufd->fd)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target domain IOMMU device iommufd fd '%1$s' does not match source '%2$s'"),
+ dst->iommufd->fd,
+ src->iommufd->fd);
+ return false;
+ }
+ } else if ((src->iommufd && !dst->iommufd) ||
+ (!src->iommufd && dst->iommufd)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Target domain IOMMU device iommufd does not match source"));
+ return false;
+ }
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
}
@@ -28310,6 +28406,16 @@ virDomainIOMMUDefFormat(virBuffer *buf,
virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL);
+ if (iommu->iommufd) {
+ virBufferAddLit(&childBuf, "<iommufd>\n");
+ virBufferAdjustIndent(&childBuf, 2);
+ virBufferAsprintf(&childBuf, "<id>%s</id>\n", iommu->iommufd->id);
+ if (iommu->iommufd->fd)
+ virBufferAsprintf(&childBuf, "<fd>%s</fd>\n", iommu->iommufd->fd);
+ virBufferAdjustIndent(&childBuf, -2);
+ virBufferAddLit(&childBuf, "</iommufd>\n");
+ }
+
virDomainDeviceInfoFormat(&childBuf, &iommu->info, 0);
virBufferAsprintf(&attrBuf, " model='%s'",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 75568ff50a..ce447e9823 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3010,6 +3010,11 @@ typedef enum {
VIR_DOMAIN_IOMMU_MODEL_LAST
} virDomainIOMMUModel;
+struct _virDomainIommufdDef {
+ char *id;
+ char *fd;
+};
+
struct _virDomainIOMMUDef {
virDomainIOMMUModel model;
virTristateSwitch intremap;
@@ -3017,6 +3022,8 @@ struct _virDomainIOMMUDef {
virTristateSwitch eim;
virTristateSwitch iotlb;
unsigned int aw_bits;
+ virDomainIommufdDef *iommufd;
+
virDomainDeviceInfo info;
virTristateSwitch dma_translation;
};
@@ -3747,6 +3754,7 @@ virDomainVideoDef *virDomainVideoDefNew(virDomainXMLOption *xmlopt);
void virDomainVideoDefFree(virDomainVideoDef *def);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVideoDef, virDomainVideoDefFree);
void virDomainVideoDefClear(virDomainVideoDef *def);
+void virDomainIommufdDefFree(virDomainIommufdDef *def);
virDomainHostdevDef *virDomainHostdevDefNew(void);
void virDomainHostdevDefFree(virDomainHostdevDef *def);
void virDomainHubDefFree(virDomainHubDef *def);
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 4861b1c002..88649ba0b9 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -1838,9 +1838,9 @@ virDomainDefCputuneValidate(const virDomainDef *def)
static int
virDomainDefIOMMUValidate(const virDomainDef *def)
{
- size_t i;
+ size_t i, j;
- if (!def->iommu)
+ if (!def->iommu || def->niommus == 0)
return 0;
for (i = 0; i < def->niommus; i++) {
@@ -1851,6 +1851,39 @@ virDomainDefIOMMUValidate(const virDomainDef *def)
_("IOMMU model smmuv3Dev must be specified for multiple IOMMU definitions"));
}
+ for (j = i + 1; j < def->niommus; j++) {
+ if (virDomainIOMMUDefEquals(iommu,
+ def->iommu[j])) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("IOMMU already exists in the domain configuration"));
+ return -1;
+ }
+
+ if (iommu->iommufd && def->iommu[j]->iommufd) {
+ if (iommu->iommufd->id && def->iommu[j]->iommufd->id) {
+ if (STRNEQ(iommu->iommufd->id, def->iommu[j]->iommufd->id)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("iommufd ID must be the same for multiple IOMMUs"));
+ return -1;
+ }
+ } else {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("iommufd ID must be specified"));
+ }
+ if (iommu->iommufd->fd && def->iommu[j]->iommufd->fd &&
+ STRNEQ(iommu->iommufd->fd, def->iommu[j]->iommufd->fd)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("iommufd FD must be the same for multiple IOMMUs"));
+ return -1;
+ }
+ } else if ((iommu->iommufd && !def->iommu[j]->iommufd) ||
+ (!iommu->iommufd && def->iommu[j]->iommufd)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("The same iommufd configuration must be specified for multiple IOMMUs"));
+ return -1;
+ }
+ }
+
if (iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -3115,6 +3148,13 @@ virDomainIOMMUDefValidate(const virDomainIOMMUDef *iommu)
break;
}
+ if (iommu->iommufd) {
+ if (!iommu->iommufd->id) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("iommufd must have an associated id"));
+ return -1;
+ }
+ }
return 0;
}
diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
index 8d1768b24f..41db338c71 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -6231,6 +6231,20 @@
<optional>
<ref name="acpi"/>
</optional>
+ <optional>
+ <element name="iommufd">
+ <interleave>
+ <element name="id">
+ <text/>
+ </element>
+ <optional>
+ <element name="fd">
+ <text/>
+ </element>
+ </optional>
+ </interleave>
+ </element>
+ </optional>
<optional>
<ref name="address"/>
</optional>
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index c70437bc05..47392154a4 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -142,6 +142,8 @@ typedef struct _virDomainHubDef virDomainHubDef;
typedef struct _virDomainHugePage virDomainHugePage;
+typedef struct _virDomainIommufdDef virDomainIommufdDef;
+
typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9deafefaad..d683d0eef7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6166,6 +6166,19 @@ qemuBuildIOMMUCommandLine(virCommand *cmd,
if (!def->iommu || def->niommus <= 0)
return 0;
+ if (def->iommu[0]->iommufd) {
+ if (qemuMonitorCreateObjectProps(&props, "iommufd",
+ def->iommu[0]->iommufd->id,
+ "S:fd", def->iommu[0]->iommufd->fd,
+ NULL) < 0)
+ return -1;
+
+ if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
+ return -1;
+ }
+
+ props = NULL;
+
for (i = 0; i < def->niommus; i++) {
virDomainIOMMUDef *iommu = def->iommu[i];
switch (iommu->model) {
--
2.43.0On Thu, May 15, 2025 at 01:36:40PM -0700, Nathan Chen via Devel wrote:
> Add support for an "iommufd" virDomainIOMMUDef member that will be
> translated to a qemu "-object iommufd" command line argument. This
> iommufd struct has an "id" member to specify the iommufd object id
> that can be associated with a passthrough device, and an "fd"
> member to specify the fd for externally opening the /dev/iommu and
> VFIO cdev by a management layer.
>
> @@ -28310,6 +28406,16 @@ virDomainIOMMUDefFormat(virBuffer *buf,
>
> virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL);
>
> + if (iommu->iommufd) {
> + virBufferAddLit(&childBuf, "<iommufd>\n");
> + virBufferAdjustIndent(&childBuf, 2);
> + virBufferAsprintf(&childBuf, "<id>%s</id>\n", iommu->iommufd->id);
> + if (iommu->iommufd->fd)
> + virBufferAsprintf(&childBuf, "<fd>%s</fd>\n", iommu->iommufd->fd);
I'm not convinced we should be exposing the QEMU "id" in the XML.
It is an identifier libvirt uses internally for configuring
QEMU, but I don't see a reason why the end user needs to control
its name. I'm similarly not sure we should expose an FD to
users eitherm, just use that internally when talking to QEMU.
IOW, why aren't we just having "iommu='yes'" as an attribute
on the <iommu> device, avoiding all the logic below that
tries to force all devices to use the same configuration.
> + virBufferAdjustIndent(&childBuf, -2);
> + virBufferAddLit(&childBuf, "</iommufd>\n");
> + }
> +
> virDomainDeviceInfoFormat(&childBuf, &iommu->info, 0);
>
> virBufferAsprintf(&attrBuf, " model='%s'",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 75568ff50a..ce447e9823 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3010,6 +3010,11 @@ typedef enum {
> VIR_DOMAIN_IOMMU_MODEL_LAST
> } virDomainIOMMUModel;
>
> +struct _virDomainIommufdDef {
> + char *id;
> + char *fd;
> +};
> +
> struct _virDomainIOMMUDef {
> virDomainIOMMUModel model;
> virTristateSwitch intremap;
> @@ -3017,6 +3022,8 @@ struct _virDomainIOMMUDef {
> virTristateSwitch eim;
> virTristateSwitch iotlb;
> unsigned int aw_bits;
> + virDomainIommufdDef *iommufd;
> +
> virDomainDeviceInfo info;
> virTristateSwitch dma_translation;
> };
> @@ -3747,6 +3754,7 @@ virDomainVideoDef *virDomainVideoDefNew(virDomainXMLOption *xmlopt);
> void virDomainVideoDefFree(virDomainVideoDef *def);
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVideoDef, virDomainVideoDefFree);
> void virDomainVideoDefClear(virDomainVideoDef *def);
> +void virDomainIommufdDefFree(virDomainIommufdDef *def);
> virDomainHostdevDef *virDomainHostdevDefNew(void);
> void virDomainHostdevDefFree(virDomainHostdevDef *def);
> void virDomainHubDefFree(virDomainHubDef *def);
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 4861b1c002..88649ba0b9 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -1838,9 +1838,9 @@ virDomainDefCputuneValidate(const virDomainDef *def)
> static int
> virDomainDefIOMMUValidate(const virDomainDef *def)
> {
> - size_t i;
> + size_t i, j;
>
> - if (!def->iommu)
> + if (!def->iommu || def->niommus == 0)
> return 0;
>
> for (i = 0; i < def->niommus; i++) {
> @@ -1851,6 +1851,39 @@ virDomainDefIOMMUValidate(const virDomainDef *def)
> _("IOMMU model smmuv3Dev must be specified for multiple IOMMU definitions"));
> }
>
> + for (j = i + 1; j < def->niommus; j++) {
> + if (virDomainIOMMUDefEquals(iommu,
> + def->iommu[j])) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("IOMMU already exists in the domain configuration"));
> + return -1;
> + }
> +
> + if (iommu->iommufd && def->iommu[j]->iommufd) {
> + if (iommu->iommufd->id && def->iommu[j]->iommufd->id) {
> + if (STRNEQ(iommu->iommufd->id, def->iommu[j]->iommufd->id)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("iommufd ID must be the same for multiple IOMMUs"));
> + return -1;
> + }
> + } else {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("iommufd ID must be specified"));
> + }
> + if (iommu->iommufd->fd && def->iommu[j]->iommufd->fd &&
> + STRNEQ(iommu->iommufd->fd, def->iommu[j]->iommufd->fd)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("iommufd FD must be the same for multiple IOMMUs"));
> + return -1;
> + }
> + } else if ((iommu->iommufd && !def->iommu[j]->iommufd) ||
> + (!iommu->iommufd && def->iommu[j]->iommufd)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("The same iommufd configuration must be specified for multiple IOMMUs"));
> + return -1;
> + }
> + }
> +
> if (iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
> def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -3115,6 +3148,13 @@ virDomainIOMMUDefValidate(const virDomainIOMMUDef *iommu)
> break;
> }
>
> + if (iommu->iommufd) {
> + if (!iommu->iommufd->id) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("iommufd must have an associated id"));
> + return -1;
> + }
> + }
> return 0;
> }
>
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index 8d1768b24f..41db338c71 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -6231,6 +6231,20 @@
> <optional>
> <ref name="acpi"/>
> </optional>
> + <optional>
> + <element name="iommufd">
> + <interleave>
> + <element name="id">
> + <text/>
> + </element>
> + <optional>
> + <element name="fd">
> + <text/>
> + </element>
> + </optional>
> + </interleave>
> + </element>
> + </optional>
> <optional>
> <ref name="address"/>
> </optional>
> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
> index c70437bc05..47392154a4 100644
> --- a/src/conf/virconftypes.h
> +++ b/src/conf/virconftypes.h
> @@ -142,6 +142,8 @@ typedef struct _virDomainHubDef virDomainHubDef;
>
> typedef struct _virDomainHugePage virDomainHugePage;
>
> +typedef struct _virDomainIommufdDef virDomainIommufdDef;
> +
> typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
>
> typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9deafefaad..d683d0eef7 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6166,6 +6166,19 @@ qemuBuildIOMMUCommandLine(virCommand *cmd,
> if (!def->iommu || def->niommus <= 0)
> return 0;
>
> + if (def->iommu[0]->iommufd) {
> + if (qemuMonitorCreateObjectProps(&props, "iommufd",
> + def->iommu[0]->iommufd->id,
> + "S:fd", def->iommu[0]->iommufd->fd,
> + NULL) < 0)
> + return -1;
> +
> + if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
> + return -1;
> + }
> +
> + props = NULL;
> +
> for (i = 0; i < def->niommus; i++) {
> virDomainIOMMUDef *iommu = def->iommu[i];
> switch (iommu->model) {
> --
> 2.43.0
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 5/20/2025 6:01 AM, Daniel P. Berrangé wrote:
>> Add support for an "iommufd" virDomainIOMMUDef member that will be
>> translated to a qemu "-object iommufd" command line argument. This
>> iommufd struct has an "id" member to specify the iommufd object id
>> that can be associated with a passthrough device, and an "fd"
>> member to specify the fd for externally opening the /dev/iommu and
>> VFIO cdev by a management layer.
>>
>> @@ -28310,6 +28406,16 @@ virDomainIOMMUDefFormat(virBuffer *buf,
>>
>> virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL);
>>
>> + if (iommu->iommufd) {
>> + virBufferAddLit(&childBuf, "<iommufd>\n");
>> + virBufferAdjustIndent(&childBuf, 2);
>> + virBufferAsprintf(&childBuf, "<id>%s</id>\n", iommu->iommufd->id);
>> + if (iommu->iommufd->fd)
>> + virBufferAsprintf(&childBuf, "<fd>%s</fd>\n", iommu->iommufd->fd);
> I'm not convinced we should be exposing the QEMU "id" in the XML.
> It is an identifier libvirt uses internally for configuring
> QEMU, but I don't see a reason why the end user needs to control
> its name. I'm similarly not sure we should expose an FD to
> users eitherm, just use that internally when talking to QEMU.
>
> IOW, why aren't we just having "iommu='yes'" as an attribute
> on the <iommu> device, avoiding all the logic below that
> tries to force all devices to use the same configuration.
That makes sense - I will simply have an attribute like "iommufd='yes'"
as an attribute in the next revision instead of having users specify the
iommufd id themselves.
As for exposing an FD to users, this is meant to mirror the qemu
implementation where users can specify the FD string [0], so I think we
should keep this part as-is.
[0] https://www.qemu.org/docs/master/devel/vfio-iommufd.html
Thanks,
Nathan
On Tue, May 27, 2025 at 06:51:50PM -0700, Nathan Chen wrote:
>
>
> On 5/20/2025 6:01 AM, Daniel P. Berrangé wrote:
> > > Add support for an "iommufd" virDomainIOMMUDef member that will be
> > > translated to a qemu "-object iommufd" command line argument. This
> > > iommufd struct has an "id" member to specify the iommufd object id
> > > that can be associated with a passthrough device, and an "fd"
> > > member to specify the fd for externally opening the /dev/iommu and
> > > VFIO cdev by a management layer.
> > >
> > > @@ -28310,6 +28406,16 @@ virDomainIOMMUDefFormat(virBuffer *buf,
> > > virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL);
> > > + if (iommu->iommufd) {
> > > + virBufferAddLit(&childBuf, "<iommufd>\n");
> > > + virBufferAdjustIndent(&childBuf, 2);
> > > + virBufferAsprintf(&childBuf, "<id>%s</id>\n", iommu->iommufd->id);
> > > + if (iommu->iommufd->fd)
> > > + virBufferAsprintf(&childBuf, "<fd>%s</fd>\n", iommu->iommufd->fd);
> > I'm not convinced we should be exposing the QEMU "id" in the XML.
> > It is an identifier libvirt uses internally for configuring
> > QEMU, but I don't see a reason why the end user needs to control
> > its name. I'm similarly not sure we should expose an FD to
> > users eitherm, just use that internally when talking to QEMU.
> >
> > IOW, why aren't we just having "iommu='yes'" as an attribute
> > on the <iommu> device, avoiding all the logic below that
> > tries to force all devices to use the same configuration.
>
> That makes sense - I will simply have an attribute like "iommufd='yes'" as
> an attribute in the next revision instead of having users specify the
> iommufd id themselves.
>
> As for exposing an FD to users, this is meant to mirror the qemu
> implementation where users can specify the FD string [0], so I think we
> should keep this part as-is.
The FD passing functionality exposed by QEMU is targetted at mgmt
apps like libvirt, which apply sandboxing to QEMU, and thus need
to pass in pre-opened FDs. Most of the time, this is not something
that libvirt will expose to its own API/XML users, as FDs are a
very low level concept that is backend implementation specific.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 5/30/2025 6:47 AM, Daniel P. Berrangé wrote:
>>
>> On 5/20/2025 6:01 AM, Daniel P. Berrangé wrote:
>>>> Add support for an "iommufd" virDomainIOMMUDef member that will be
>>>> translated to a qemu "-object iommufd" command line argument. This
>>>> iommufd struct has an "id" member to specify the iommufd object id
>>>> that can be associated with a passthrough device, and an "fd"
>>>> member to specify the fd for externally opening the /dev/iommu and
>>>> VFIO cdev by a management layer.
>>>>
>>>> @@ -28310,6 +28406,16 @@ virDomainIOMMUDefFormat(virBuffer *buf,
>>>> virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL);
>>>> + if (iommu->iommufd) {
>>>> + virBufferAddLit(&childBuf, "<iommufd>\n");
>>>> + virBufferAdjustIndent(&childBuf, 2);
>>>> + virBufferAsprintf(&childBuf, "<id>%s</id>\n", iommu->iommufd->id);
>>>> + if (iommu->iommufd->fd)
>>>> + virBufferAsprintf(&childBuf, "<fd>%s</fd>\n", iommu->iommufd->fd);
>>> I'm not convinced we should be exposing the QEMU "id" in the XML.
>>> It is an identifier libvirt uses internally for configuring
>>> QEMU, but I don't see a reason why the end user needs to control
>>> its name. I'm similarly not sure we should expose an FD to
>>> users eitherm, just use that internally when talking to QEMU.
>>>
>>> IOW, why aren't we just having "iommu='yes'" as an attribute
>>> on the <iommu> device, avoiding all the logic below that
>>> tries to force all devices to use the same configuration.
>> That makes sense - I will simply have an attribute like "iommufd='yes'" as
>> an attribute in the next revision instead of having users specify the
>> iommufd id themselves.
>>
>> As for exposing an FD to users, this is meant to mirror the qemu
>> implementation where users can specify the FD string [0], so I think we
>> should keep this part as-is.
> The FD passing functionality exposed by QEMU is targetted at mgmt
> apps like libvirt, which apply sandboxing to QEMU, and thus need
> to pass in pre-opened FDs. Most of the time, this is not something
> that libvirt will expose to its own API/XML users, as FDs are a
> very low level concept that is backend implementation specific.
I see, I will look into implementing logic to pre-open iommufd FDs from
libvirt, and keep this un-exposed to the API/XML users. Pre-opening
these FDs may come into play in resolving the permissions issues I was
encountering in the cover letter as well.
Thanks,
Nathan
© 2016 - 2025 Red Hat, Inc.