On Fri, Jul 24, 2015 at 03:30:14PM -0400, Kothapally Madhu Pavan wrote:

PowerPC pseries based VMs do not support a floppy disk controller. This prohibits libvirt from adding floppy disk for a PowerPC pseries VM.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 19 +++++++++++++++---- src/qemu/qemu_capabilities.c | 15 +++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 73ac537..b9f35b4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6540,7 +6540,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virHashTablePtr bootHash, virSecurityLabelDefPtr* vmSeclabels, int nvmSeclabels, - unsigned int flags) + unsigned int flags, + virArch arch, + const char *machine) { virDomainDiskDefPtr def; xmlNodePtr sourceNode = NULL; @@ -7165,6 +7167,12 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } else { if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(arch) && STRPREFIX(machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + } def->bus = VIR_DOMAIN_DISK_BUS_FDC; } else if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { if (STRPREFIX(target, "hd")) @@ -12375,7 +12383,8 @@ virDomainDeviceDefParse(const char *xmlStr, if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, NULL, def->seclabels, def->nseclabels, - flags))) + flags, def->os.arch, + def->os.machine)))

The architecture and machine type do not affect parsing of the disk. If we really want to forbid even parsing a domain with a floppy, this would better be done somewhere in the virDomainDefPostParse functions. But I think we should happily parse this domain and only report an error on startup, so all the domain_conf.c changes can be dropped.

goto error; break; case VIR_DOMAIN_DEVICE_LEASE: @@ -12519,7 +12528,8 @@ virDomainDiskDefSourceParse(const char *xmlStr, if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, NULL, def->seclabels, def->nseclabels, - flags))) + flags, def->os.arch, + def->os.machine))) goto cleanup;

ret = disk->src; @@ -15539,7 +15549,8 @@ virDomainDefParseXML(xmlDocPtr xml, bootHash, def->seclabels, def->nseclabels, - flags); + flags, def->os.arch, + def->os.machine); if (!disk) goto error;

The rest looks good. Jan

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d8cb32d..e304473 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3919,25 +3919,32 @@ virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps,

static int virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, + const char *machine, virDomainCapsDeviceDiskPtr disk) { disk->device.supported = true; /* QEMU supports all of these */ VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_DISK, - VIR_DOMAIN_DISK_DEVICE_CDROM, - VIR_DOMAIN_DISK_DEVICE_FLOPPY); + VIR_DOMAIN_DISK_DEVICE_CDROM); + + /* PowerPC pseries based VMs do not support floppy device */ + if (!(ARCH_IS_PPC64(qemuCaps->arch) && STRPREFIX(machine, "pseries"))) + VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_FLOPPY);

if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_LUN);

VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_IDE, - VIR_DOMAIN_DISK_BUS_FDC, VIR_DOMAIN_DISK_BUS_SCSI, VIR_DOMAIN_DISK_BUS_VIRTIO, /* VIR_DOMAIN_DISK_BUS_SD */);

+ /* PowerPC pseries based VMs do not support floppy device */ + if (!(ARCH_IS_PPC64(qemuCaps->arch) && STRPREFIX(machine, "pseries"))) + VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_FDC); + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_USB); return 0; @@ -4008,7 +4015,7 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,

if (virQEMUCapsFillDomainOSCaps(qemuCaps, os, loader, nloader) < 0 || - virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, disk) < 0 || + virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, domCaps->machine, disk) < 0 || virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0) return -1; return 0;

On Fri, Jul 24, 2015 at 03:30:49PM -0400, Kothapally Madhu Pavan wrote:

PowerPC pseries based VMs do not support a floppy disk controller. This prohibits libvirt from creating qemu command with floppy device.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--

Extending the qemuxml2argvtest with a test case that fails on such config would be nice.

1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 42906a8..93f84e2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9486,6 +9486,12 @@ qemuBuildCommandLine(virConnectPtr conn, boot[i] = 'd'; break; case VIR_DOMAIN_BOOT_FLOPPY: + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + }

There is no need to error out earlier for machines that do not use deviceboot. This error can be dropped, we will hit the next one.

boot[i] = 'a'; break; case VIR_DOMAIN_BOOT_DISK: @@ -9769,6 +9775,12 @@ qemuBuildCommandLine(virConnectPtr conn, bootCD = 0; break; case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + }

This is more appropriate place for the error message. Personally, I would move it above the switch() and let the switch only deal with boot indexes.

bootindex = bootFloppy; bootFloppy = 0; break; @@ -9812,6 +9824,12 @@ qemuBuildCommandLine(virConnectPtr conn,

if (withDeviceArg) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + }

This is dead code, the condition will never be true here, we already matched the error above.

if (virAsprintf(&optstr, "drive%c=drive-%s", disk->info.addr.drive.unit ? 'B' : 'A', disk->info.alias) < 0) @@ -9854,6 +9872,12 @@ qemuBuildCommandLine(virConnectPtr conn, /* Newer Q35 machine types require an explicit FDC controller */ virBufferTrim(&fdc_opts, ",", -1); if ((fdc_opts_str = virBufferContentAndReset(&fdc_opts))) { + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + }

Same here.

virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "isa-fdc,%s", fdc_opts_str); VIR_FREE(fdc_opts_str); @@ -9918,10 +9942,17 @@ qemuBuildCommandLine(virConnectPtr conn, _("cannot create virtual FAT disks in read-write mode")); goto error; } - if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + } fmt = "fat:floppy:%s"; - else + } else { fmt = "fat:%s"; + }

This code path can only be taken for QEMUs not having QEMU_CAPS_DRIVE, i.e. older than at least 7 years. I do not think touching this part of code is worth it.

if (virAsprintf(&file, fmt, disk->src) < 0) goto error;

@@ -11674,6 +11705,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, def->device = VIR_DOMAIN_DISK_DEVICE_CDROM; def->src->readonly = true; } else if (STREQ(values[i], "floppy")) { + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(dom->os.arch) && STRPREFIX(dom->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + }

This function is used to parse the command line of a running QEMU. If QEMU rejects the floppy drive on the command line, such a machine would not be running. If it quietly ignores it, we should do that too to get a matching config.

def->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; } } else if (STREQ(keywords[i], "format")) { @@ -12908,6 +12945,12 @@ qemuParseCommandLine(virCapsPtr qemuCaps, disk->src->readonly = true; } else { if (STRPREFIX(arg, "-fd")) { + /* PowerPC pseries based VMs do not support floppy device */ + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); + goto error; + }

Same here. Jan