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@redhat.com> --- 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.