Hi Cole,

Thanks for the comments.


On Fri, Nov 22, 2013 at 1:54 AM, Cole Robinson <crobinso@redhat.com> wrote:
On 11/21/2013 03:06 PM, Shivaprasad G Bhat wrote:
> From: Shivaprasad G Bhat <shivaprasadbhat@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 <sbhat@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