Hi Cole,
Thanks for the comments.
On Fri, Nov 22, 2013 at 1:54 AM, Cole Robinson <crobi...@redhat.com> wrote:
> On 11/21/2013 03:06 PM, Shivaprasad G Bhat wrote:
> > From: Shivaprasad G Bhat <shivaprasadb...@gmail.com>
> >
> > 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 <sb...@linux.vnet.ibm.com>
> > ---
> > 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;
>
> 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.
I have removed the chunk from v4 and i'll address it separately as
suggested.
> > } 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
>
> Does -hda actually create a scsi disk on pseries? Interesting.
>
Yes. It does. :)
> > 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>
> >
>
> 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.
>
It works. I see that selinux option is not added by default to the xml.
Otherwise the xml works taking hda to scsiI.
> - Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
