On Wed, Jul 17, 2019 at 17:05:08 +0200, Michal Privoznik wrote:
> On 7/16/19 3:00 PM, Peter Krempa wrote:
> > On Thu, Jul 11, 2019 at 17:53:58 +0200, Michal Privoznik wrote:
> > > To simplify implementation, some restrictions are added. For
> > > instance, an NVMe disk can't go to any bus but virtio and has to
> > > be type of 'disk' and can't have startupPolicy set.
> > >
> > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > > ---
> > > src/conf/domain_conf.c | 129 +++++++++++++++++++++++++
> > > src/libvirt_private.syms | 1 +
> > > src/qemu/qemu_block.c | 1 +
> > > src/qemu/qemu_command.c | 1 +
> > > src/qemu/qemu_driver.c | 4 +
> > > src/qemu/qemu_migration.c | 1 +
> > > src/util/virstoragefile.c | 59 +++++++++++
> > > src/util/virstoragefile.h | 15 +++
> > > src/xenconfig/xen_xl.c | 1 +
> > > tests/qemuxml2argvdata/disk-nvme.xml | 12 ++-
> > > tests/qemuxml2xmloutdata/disk-nvme.xml | 1 +
> > > tests/qemuxml2xmltest.c | 1 +
> > > 12 files changed, 224 insertions(+), 2 deletions(-)
> > > create mode 120000 tests/qemuxml2xmloutdata/disk-nvme.xml
> > >
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index 3323c9a5b1..73f5e1fa0f 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> >
> > [...]
> >
> > > @@ -5938,6 +5943,38 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
> > > return -1;
> > > }
> > > + if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
> > > + /* These might not be valid for all hypervisors, but be
> > > + * strict now and possibly refine in the future. */
> > > + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("Unsupported disk type '%s' for NVMe disk"),
> > > + virDomainDiskDeviceTypeToString(disk->device));
> > > + return -1;
> > > + }
> > > +
> > > + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("Unsupported bus '%s' for NVMe disk"),
> > > + virDomainDiskBusTypeToString(disk->bus));
> > > + return -1;
> > > + }
> > > +
> > > + if (disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_DEFAULT &&
> > > + disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_MANDATORY) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("Unsupported startup policy '%s' for NVMe disk"),
> > > + virDomainStartupPolicyTypeToString(disk->startupPolicy));
> > > + return -1;
> > > + }
> > > +
> > > + if (disk->src->shared) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > + _("Unsupported <shareable/> for NVMe disk"));
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > return 0;
> > > }
> >
> > As noted in the other thread, this really should be extracted, placed in
> > the validation callback rather than post parse and must iterate the
> > backing chain if you want this to keep working with -blockdev.
>
> I'm not sure I understand what you mean. This function is called
> virDomainDiskDefValidate() and therefore it is validation callback rather
> than post parse callback. Where do you want me to put these checks?
Sorry I thought it was in post-parse. This is okay.
>
> >
> > > @@ -9184,6 +9221,76 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> > > }
> > > +static int
> > > +virDomainDiskSourceNVMeParse(xmlNodePtr node,
> > > + xmlXPathContextPtr ctxt,
> > > + virStorageSourcePtr src)
> > > +{
> > > + VIR_AUTOPTR(virStorageSourceNVMeDef) nvme = NULL;
> > > + VIR_AUTOFREE(char *) type = NULL;
> > > + VIR_AUTOFREE(char *) namespace = NULL;
> > > + VIR_AUTOFREE(char *) managed = NULL;
> > > + xmlNodePtr address;
> > > +
> > > + if (VIR_ALLOC(nvme) < 0)
> > > + return -1;
> > > +
> > > + if (!(type = virXMLPropString(node, "type"))) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("missing 'type' attribute to disk source"));
> > > + return -1;
> > > + }
> > > +
> > > + if (STRNEQ(type, "pci")) {
> > > + virReportError(VIR_ERR_XML_ERROR,
> > > + _("unsupported source type '%s'"),
> > > + type);
> > > + return -1;
> > > + }
> > > +
> > > + if (!(namespace = virXMLPropString(node, "namespace"))) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("missing 'namespace' attribute to disk source"));
> > > + return -1;
> > > + }
> > > +
> > > + if (virStrToLong_ul(namespace, NULL, 10, &nvme->namespace) < 0) {
> > > + virReportError(VIR_ERR_XML_ERROR,
> > > + _("malformed namespace '%s'"),
> > > + namespace);
> > > + return -1;
> > > + }
> > > +
> > > + /* NVMe namespaces start from 1 */
> > > + if (nvme->namespace == 0) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("NVMe namespace can't be zero"));
> > > + return -1;
> > > + }
> > > +
> > > + if ((managed = virXMLPropString(node, "managed"))) {
> > > + if ((nvme->managed = virTristateBoolTypeFromString(managed)) <= 0) {
> > > + virReportError(VIR_ERR_XML_ERROR,
> > > + _("malformed managed value '%s'"),
> > > + managed);
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + if (!(address = virXPathNode("./address", ctxt))) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("NVMe disk source is missing address"));
> > > + return -1;
> > > + }
> >
> > I'm displeased that this is yet another function adding validation in
> > the parser. You don't make the status quo any worse though so this is
> > not a request to change it.
>
> What validation do you mean? namespace != 0 check? Well, that stems straight
> from NVMe specs, so it is completely independent of any driver.
> Or do you mean this !address check? Well, it's needed later in the parsing.
Yes, but that stuff still does not have to be intermixed in the parser
code. It's a pre-existing mess though.
>
> >
> > > +
> > > + if (virPCIDeviceAddressParseXML(address, &nvme->pciAddr) < 0)
> > > + return -1;
> > > +
> > > + VIR_STEAL_PTR(src->nvme, nvme);
> > > + return 0;
> > > +}
> > > +
> > > +
> > > static int
> > > virDomainDiskSourcePRParse(xmlNodePtr node,
> > > xmlXPathContextPtr ctxt,
> >
> > [...]
> >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 2436f5051b..87adccab3d 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> >
> > Missing change to 'qemuMigrationSrcIsSafe' to reject VMs containing NVMe
> > disks not having shared source.
>
> A NVMe disk can't be shared. It's even explicitly denied in the validation
> callback. The reason is that there is no way to share a single PCI device
> with multiple domains.
I meant that the NVMe disk is NOT available on the destination of the
migration. That means that 'qemuMigrationSrcIsSafe' must reject it as
not having a shared storage (note that "shared" here has a different
conotation as <shareable/>, just look at he named function).
>
> But this is a good point. I'll probably put it into a different commt
> though. Even though I'm changing some parts of qemu driver here it's only
> because of the way we handle switch() with enums.
>
> >
> > Plus that function should probably check the full backing chain rather
> > than the top element only.
>
> Pre-existing, but I can try to fix it. Okay.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
