<div dir="ltr">Hi Cole,<div> </div><div>Thanks for the comments. <div class="gmail_extra"> <div class="gmail_quote">On Fri, Nov 22, 2013 at 1:54 AM, Cole Robinson <<a href="mailto:crobinso@redhat.com" target="_blank">crobinso@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 11/21/2013 03:06 PM, Shivaprasad G Bhat wrote: > From: Shivaprasad G Bhat <<a href="mailto:shivaprasadbhat@gmail.com">shivaprasadbhat@gmail.com</a>> > > The bus type IDE being enum Zero, the bus type on pseries system appears as IDE for all the -hda/-cdrom and for disk drives with if="none" type. Pseries platform needs this to appear as SCSI instead of IDE. The ide being not supported, the explicit requests for ide devices will return an error. > > Signed-off-by: Shivaprasad G Bhat <<a href="mailto:sbhat@linux.vnet.ibm.com">sbhat@linux.vnet.ibm.com</a>> > --- > src/qemu/qemu_command.c | 66 +++++++++++++++++++- > tests/qemuargv2xmltest.c | 1 > .../qemuxml2argv-pseries-disk.args | 5 ++ > .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 40 ++++++++++++ > 4 files changed, 108 insertions(+), 4 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 8dc7e43..21b5108 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -10032,6 +10032,7 @@ error: > static virDomainDiskDefPtr > qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, > const char *val, > + virDomainDefPtr dom, > int nvirtiodisk, > bool old_style_ceph_args) > { > @@ -10055,7 +10056,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, > if (VIR_ALLOC(def) < 0) > goto cleanup; > > - def->bus = VIR_DOMAIN_DISK_BUS_IDE; > + if (((dom->os.arch == VIR_ARCH_PPC64) && > + dom->os.machine && STREQ(dom->os.machine, "pseries"))) > + def->bus = VIR_DOMAIN_DISK_BUS_SCSI; > + else > + def->bus = VIR_DOMAIN_DISK_BUS_IDE; > def->device = VIR_DOMAIN_DISK_DEVICE_DISK; > def->type = VIR_DOMAIN_DISK_TYPE_FILE; > > @@ -10140,9 +10145,15 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, > def->type = VIR_DOMAIN_DISK_TYPE_FILE; > } > } else if (STREQ(keywords[i], "if")) { > - if (STREQ(values[i], "ide")) > + if (STREQ(values[i], "ide")) { > def->bus = VIR_DOMAIN_DISK_BUS_IDE; > - else if (STREQ(values[i], "scsi")) > + if (((dom->os.arch == VIR_ARCH_PPC64) && > + dom->os.machine && STREQ(dom->os.machine, "pseries"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("pseries systems do not support ide devices '%s'"), val); > + goto error; > + } > + } else if (STREQ(values[i], "scsi")) > def->bus = VIR_DOMAIN_DISK_BUS_SCSI; > else if (STREQ(values[i], "virtio")) > def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO; > @@ -11368,6 +11379,9 @@ qemuParseCommandLine(virCapsPtr qemuCaps, > disk->type = VIR_DOMAIN_DISK_TYPE_FILE; > if (STREQ(arg, "-cdrom")) { > disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; > + if (((def->os.arch == VIR_ARCH_PPC64) && > + def->os.machine && STREQ(def->os.machine, "pseries"))) > + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; > if (VIR_STRDUP(disk->dst, "hdc") < 0) > goto error; > disk->readonly = true; > @@ -11381,6 +11395,9 @@ qemuParseCommandLine(virCapsPtr qemuCaps, > disk->bus = VIR_DOMAIN_DISK_BUS_IDE; > else > disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; > + if (((def->os.arch == VIR_ARCH_PPC64) && > + def->os.machine && STREQ(def->os.machine, "pseries"))) > + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; > } > if (VIR_STRDUP(disk->dst, arg + 1) < 0) > goto error; > @@ -11672,7 +11689,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, > } > } else if (STREQ(arg, "-drive")) { > WANT_VALUE(); > - if (!(disk = qemuParseCommandLineDisk(xmlopt, val, > + if (!(disk = qemuParseCommandLineDisk(xmlopt, val, def, > nvirtiodisk, > ceph_args != NULL))) > goto error; > @@ -11858,6 +11875,47 @@ qemuParseCommandLine(virCapsPtr qemuCaps, > } > } else if (STREQ(arg, "-S")) { > /* ignore, always added by libvirt */ > + } else if (STREQ(arg, "-device")) { > + /* something we can't fully parse. Check if supported and > + * add it to the qemu namespace > + * cmdline/environment advanced options and hope for the best > + */ > + bool unsupported = false; > + WANT_VALUE() > + > + if ((def->os.arch == VIR_ARCH_PPC64) && > + def->os.machine && STREQ(def->os.machine, "pseries")) { > + int nkws; > + char **kws; > + char **vals; > + size_t j; > + > + if (!qemuParseKeywords(val, &kws, &vals, &nkws, 1)) { > + for (j = 0; j < nkws; j++) { > + if (STREQLEN(kws[j], "ide", 3) || > + (STREQ(kws[j], "if") && vals[j] && > + STREQ(vals[j], "ide"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("pseries systems do not support ide devices '%s'"), val); > + unsupported = true; > + break; > + } > + } > + for (j = 0; j < nkws; j++) { > + VIR_FREE(kws[j]); > + VIR_FREE(vals[j]); > + } > + VIR_FREE(kws); > + VIR_FREE(vals); > + } > + } > + > + if (unsupported || VIR_REALLOC_N(cmd->args, cmd->num_args+2) < 0) > + goto error; > + if (VIR_STRDUP(cmd->args[cmd->num_args++], arg) < 0) > + goto error; > + if (VIR_STRDUP(cmd->args[cmd->num_args++], val) < 0) > + goto error; </div></div>This -device handling chunk should at least be a separate patch and have a test case that exercises it. But I don't know if that's how we want to handle -device conversions, just turning them into qemu commandline passthrough bits, but actually learning to parse -device would be a pretty massive undertaking.</blockquote><div> </div><div>I have removed the chunk from v4 and i'll address it separately as suggested. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > } else { > /* something we can't yet parse. Add it to the qemu namespace > * cmdline/environment advanced options and hope for the best > diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c > index 6dd8bb0..0bf4c37 100644 > --- a/tests/qemuargv2xmltest.c > +++ b/tests/qemuargv2xmltest.c > @@ -249,6 +249,7 @@ mymain(void) > DO_TEST("hyperv"); > > DO_TEST("pseries-nvram"); > + DO_TEST("pseries-disk"); > > DO_TEST("nosharepages"); > > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.args > new file mode 100644 > index 0000000..5fc0938 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.args > @@ -0,0 +1,5 @@ > +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu-system-ppc64 \ > +-S -M pseries -m 512 -smp 1 \ > +-no-acpi -boot c -usb \ > +-boot c -hda /dev/HostVG/QEMUGuest1 -cdrom /root/boot.iso </div>Does -hda actually create a scsi disk on pseries? Interesting. </blockquote><div> </div><div>Yes. It does. :) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml > new file mode 100644 > index 0000000..dbbd6aa > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml > @@ -0,0 +1,40 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>87eedafe-eedc-4336-8130-ed9fe5dc90c8</uuid> > + <memory unit='KiB'>524288</memory> > + <currentMemory unit='KiB'>524288</currentMemory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='ppc64' machine='pseries'>hvm</type> > + <boot dev='hd'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>destroy</on_crash> > + <devices> > + <emulator>/usr/bin/qemu-system-ppc64</emulator> > + <disk type='block' device='disk'> > + <driver name='qemu' type='raw'/> > + <source dev='/dev/HostVG/QEMUGuest1'/> > + <target dev='hda' bus='scsi'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <disk type='file' device='cdrom'> > + <driver name='qemu' type='raw'/> > + <source file='/root/boot.iso'/> > + <target dev='hdc' bus='scsi'/> > + <readonly/> > + <address type='drive' controller='0' bus='0' target='0' unit='2'/> > + </disk> > + <controller type='usb' index='0'/> > + <controller type='scsi' index='0'/> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <graphics type='sdl'/> > + <video> > + <model type='cirrus' vram='9216' heads='1'/> > + </video> > + <memballoon model='virtio'/> > + </devices> > +</domain> > </div></div>Does this XML actually work if you 'virsh define' it? I'd think libvirt would complain about target='hda' with bus='scsi' but I didn't confirm. </blockquote><div> </div><div>It works. I see that selinux option is not added by default to the xml. Otherwise the xml works taking hda to scsiI. </div> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - Cole <div class="HOEnZb"><div class="h5"> -- libvir-list mailing list <a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a> </div></div></blockquote></div> </div></div></div>