[libvirt] [PATCH 1/3] qemu: PPCs G3Beige has a default IDE controller

[Guido Günther]

Hi Laine,
On Fri, Nov 20, 2015 at 06:55:09PM -0500, Laine Stump wrote:
> On 11/20/2015 03:20 PM, Guido Günther wrote:
> >so handle it like I440FX
> >---
> >  src/qemu/qemu_command.c | 17 ++++++++++++-----
> >  src/qemu/qemu_domain.c  |  7 +++++++
> >  src/qemu/qemu_domain.h  |  1 +
> >  3 files changed, 20 insertions(+), 5 deletions(-)
> >
> >diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >index ef5ef93..d6b7f09 100644
> >--- a/src/qemu/qemu_command.c
> >+++ b/src/qemu/qemu_command.c
> >@@ -1054,11 +1054,13 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef,
> >           */
> >          return virAsprintf(&controller->info.alias, "pci.%d", controller->idx);
> >      } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
> >-        /* for any machine based on I440FX, the first (and currently
> >+        /* for any machine based on I440FX or G3Beige, the first (and currently
> >           * only) IDE controller is an integrated controller hardcoded
> >           * with id "ide"
> >           */
> >-        if (qemuDomainMachineIsI440FX(domainDef) && controller->idx == 0)
> >+        if ((qemuDomainMachineIsI440FX(domainDef) ||
> >+             qemuDomainMachineIsG3Beige(domainDef)) &&
> >+            controller->idx == 0)
> >              return VIR_STRDUP(controller->info.alias, "ide");
> >      } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) {
> >          /* for any Q35 machine, the first SATA controller is the
> >@@ -4915,10 +4917,13 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
> >      case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> >          /* Since we currently only support the integrated IDE controller
> >-         * on 440fx, if we ever get to here, it's because some other
> >+         * on 440fx and G3Beige, if we ever get to here, it's because some other
> >           * machinetype had an IDE controller specified, or a 440fx had
> >           * multiple ide controllers.
> >           */
> >+        if (qemuDomainMachineIsG3Beige(domainDef))
> >+                break;
> >+
> 
> This would cause us to ignore the fact that the config has a secondary IDE
> controller defined, and we don't support adding a 2nd IDE controller to the
> qemu commandline. I think it should instead report the following error
> message just like i440FX (unless the g3beige has an implicit secondary IDE
> and it is properly named).

Given that we can handle all machine types the same so instead having to
add machine types at three locations I added a single function to handle
this in the upcoming patch. Hope that's o.k., if not I'm happy to split
this up again.

> 
> Other than that, ACK (and thanks for taking the time to look for other
> machines that have builtin IDE controllers and writing the patches! I got a
> private mail about the breakage and asked the question about which machines
> have builtint IDE on qemu-devel, but hadn't had any more time to follow up).

All three were mentioned in a Debian bug so I did not too much
research. I had a quik look at the other machine types and didn't spot
any IDE in it though.


> >          if (qemuDomainMachineIsI440FX(domainDef))
> >              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >                             _("Only a single IDE controller is unsupported "
> 
> Ugh. I should have said "supported" there rather than "unsupported". Can you
> fix that while you're touching things?

Will do, thanks for review!
 -- Guido