On Sun, Mar 26, 2017 at 02:16:10PM -0400, Laine Stump wrote:
> On 03/22/2017 11:27 AM, Erik Skultety wrote:
> > A mediated device will be identified by a UUID (with 'model' now being
> > a mandatory <hostdev> attribute to represent the mediated device API) of
> > the user pre-created mediated device. The data necessary to identify a
> > mediated device can be easily extended in the future, e.g. when
> > auto-creation of mediated devices should be enabled. We also need to
>
> s/should be/is/
>
> (although... it really goes without saying that the data needed can be
> easily extended. After all, you wrote it, right? :-P)
>
> > make sure that if user explicitly provides a guest address
> > for a mdev device, the address type will be matching the device API
> > supported on that specific mediated device and error out with an
> > incorrect XML message.
>
>
> Please include a sample of the xml for an mdev <hostdev> in the commit log.
>
[...]
> > + goto error;
> > + }
> > + } else {
> > + if (!model) {
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("Missing 'model' attribute in mediated device's "
> > + "<hostdev> element"));
>
> Do you actually required it in the original config? I had thought it
> should be optional, and filled in from the device-api if it's missing
> (hmm, although I guess the problem is that we don't have access to the
> device-api at the time of definition, so maybe not :-(. Still, maybe we
> can divine a default based on the machinetype - for x86 vfio-pci is an
> extremely safe bet, for example).
>
> Anyway, that could be a lengthy discussion, so not worth holding
> everything else up for it - we can always loosen the restrictions and
> supply a default later. (it *would* be nice to be able to completely
> specify the config without needing to use "vfio" anywhere...)
Right, I'd wait until we're supposed to be able to do more than manage pre-created
devices by the user, because the 'default' would need significant adjustments,
otherwise you could end up throwing an error about a wrong model type, even
though the user specified 'default' thinking that libvirt will probably know
better, but unfortunately in the meantime someone introduced vfio-pci2
(I think we talked about this as well...) so our 'safe' assumption
about vfio-pci on the x86 platform would no longer be correct and would lead to
an error. Thus, what we probably will want to do is leave the model as default
(verbatim) and stall the resolution of the default type to some reasonable type
until we absolutely need it, which is just before calling
virHostdevPrepareDevices, and then just simply look into sysfs and assign the
value we find over there, rather than trying to guess.
Erik
>
> (end of comments)
>
> ACK with the commit log changed, and the one error log made more specific.
>
>
> > + goto error;
> > + }
> > +
> > + if ((mdevsrc->model = virMediatedDeviceModelTypeFromString(model)) < 0) {
> > + virReportError(VIR_ERR_XML_ERROR,
> > + _("unknown hostdev model '%s'"),
> > + model);
> > + goto error;
> > + }
> > + }
> > +
> > switch (def->source.subsys.type) {
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> > if (virDomainHostdevSubsysPCIDefParseXML(sourcenode, def, flags) < 0)
> > @@ -6525,6 +6605,10 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> > if (virDomainHostdevSubsysSCSIVHostDefParseXML(sourcenode, def) < 0)
> > goto error;
> > break;
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > + if (virDomainHostdevSubsysMediatedDevDefParseXML(def, ctxt) < 0)
> > + goto error;
> > + break;
> >
> > default:
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > @@ -6539,6 +6623,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> > VIR_FREE(sgio);
> > VIR_FREE(rawio);
> > VIR_FREE(backendStr);
> > + VIR_FREE(model);
> > return ret;
> > }
> >
> > @@ -13384,6 +13469,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt,
> > }
> > break;
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > break;
> > }
> > @@ -14318,6 +14404,7 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
> > return 1;
> > else
> > return 0;
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > return 0;
> > }
> > @@ -21319,6 +21406,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
> > virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci;
> > virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi;
> > virDomainHostdevSubsysSCSIVHostPtr hostsrc = &def->source.subsys.u.scsi_host;
> > + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev;
> > virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
> > virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
> >
> > @@ -21423,6 +21511,10 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
> > break;
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
> > break;
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > + virBufferAsprintf(buf, "<address uuid='%s'/>\n",
> > + mdevsrc->uuidstr);
> > + break;
> > default:
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("unexpected hostdev type %d"),
> > @@ -23318,6 +23410,7 @@ virDomainHostdevDefFormat(virBufferPtr buf,
> > {
> > const char *mode = virDomainHostdevModeTypeToString(def->mode);
> > virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi;
> > + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev;
> > const char *type;
> >
> > if (!mode) {
> > @@ -23367,6 +23460,10 @@ virDomainHostdevDefFormat(virBufferPtr buf,
> > virBufferAsprintf(buf, " rawio='%s'",
> > virTristateBoolTypeToString(scsisrc->rawio));
> > }
> > +
> > + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV)
> > + virBufferAsprintf(buf, " model='%s'",
> > + virMediatedDeviceModelTypeToString(mdevsrc->model));
> > }
> > virBufferAddLit(buf, ">\n");
> > virBufferAdjustIndent(buf, 2);
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 8e6d874178..47eaacef3d 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -295,6 +295,7 @@ typedef enum {
> > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI,
> > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
> > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST,
> > + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV,
> >
> > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST
> > } virDomainHostdevSubsysType;
> > @@ -369,6 +370,13 @@ struct _virDomainHostdevSubsysSCSI {
> > } u;
> > };
> >
> > +typedef struct _virDomainHostdevSubsysMediatedDev virDomainHostdevSubsysMediatedDev;
> > +typedef virDomainHostdevSubsysMediatedDev *virDomainHostdevSubsysMediatedDevPtr;
> > +struct _virDomainHostdevSubsysMediatedDev {
> > + int model; /* enum virMediatedDeviceModelType */
> > + char uuidstr[VIR_UUID_STRING_BUFLEN]; /* mediated device's uuid string */
> > +};
> > +
> > typedef enum {
> > VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_NONE,
> > VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST,
> > @@ -394,6 +402,7 @@ struct _virDomainHostdevSubsys {
> > virDomainHostdevSubsysPCI pci;
> > virDomainHostdevSubsysSCSI scsi;
> > virDomainHostdevSubsysSCSIVHost scsi_host;
> > + virDomainHostdevSubsysMediatedDev mdev;
> > } u;
> > };
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index c239a06767..c0f060b0a3 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -7102,6 +7102,7 @@ qemuDomainGetHostdevPath(virDomainDefPtr def,
> > break;
> > }
> >
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > break;
> > }
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index ddcbc5e259..66c469e55e 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -3907,6 +3907,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
> > qemuDomainRemoveSCSIVHostDevice(driver, vm, hostdev);
> > break;
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > + break;
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > break;
> > }
> > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> > index 0d3e891a71..f5b72e1c2d 100644
> > --- a/src/security/security_apparmor.c
> > +++ b/src/security/security_apparmor.c
> > @@ -901,6 +901,9 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
> > break;
> > }
> >
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > + break;
> > +
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > ret = 0;
> > break;
> > diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> > index d6a2daf747..4e968f29c0 100644
> > --- a/src/security/security_dac.c
> > +++ b/src/security/security_dac.c
> > @@ -964,6 +964,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
> > break;
> > }
> >
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > ret = 0;
> > break;
> > @@ -1119,6 +1120,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
> > break;
> > }
> >
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > ret = 0;
> > break;
> > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> > index a6bcf9e01f..7b3276dc34 100644
> > --- a/src/security/security_selinux.c
> > +++ b/src/security/security_selinux.c
> > @@ -1838,6 +1838,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
> > break;
> > }
> >
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > ret = 0;
> > break;
> > @@ -2065,6 +2066,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
> > break;
> > }
> >
> > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> > ret = 0;
> > break;
> > diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml
> > index 6a676253c3..82a92322e5 100644
> > --- a/tests/domaincapsschemadata/full.xml
> > +++ b/tests/domaincapsschemadata/full.xml
> > @@ -89,6 +89,7 @@
> > <value>pci</value>
> > <value>scsi</value>
> > <value>scsi_host</value>
> > + <value>mdev</value>
> > </enum>
> > <enum name='capsType'>
> > <value>storage</value>
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
