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

On 11/20/2015 03:20 PM, Guido Günther wrote:

--- src/qemu/qemu_command.c | 24 ++++++++++++++---------- src/qemu/qemu_domain.c | 6 ++++++ src/qemu/qemu_domain.h | 1 + 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6b7f09..e467f9c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1054,11 +1054,12 @@ 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 or G3Beige, the first (and currently - * only) IDE controller is an integrated controller hardcoded - * with id "ide" + /* for any machine based on I440FX, G3Beige or Sun4u, the + * first (and currently only) IDE controller is an integrated + * controller hardcoded with id "ide" */ if ((qemuDomainMachineIsI440FX(domainDef) || + qemuDomainMachineIsSun4u(domainDef) || qemuDomainMachineIsG3Beige(domainDef)) && controller->idx == 0) return VIR_STRDUP(controller->info.alias, "ide"); @@ -4916,12 +4917,14 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, break;

case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - /* Since we currently only support the integrated IDE controller - * 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. + /* Since we currently only support the integrated IDE + * controller on 440fx, G3Beige and Sun4u, 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)) + if (qemuDomainMachineIsG3Beige(domainDef) || + qemuDomainMachineIsSun4u(domainDef)) break;

Same comment for this one as for Patch 1/3. (otherwise ACK).

if (qemuDomainMachineIsI440FX(domainDef)) @@ -9905,10 +9908,11 @@ qemuBuildCommandLine(virConnectPtr conn, cont->idx == 0 && qemuDomainMachineIsQ35(def)) continue;

- /* first IDE controller on i440fx and g3beige machines - * is implicit */ + /* first IDE controller on i440fx, G3Beige and Sun4u + * machines is implicit */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && (qemuDomainMachineIsI440FX(def) || + qemuDomainMachineIsSun4u(def) || qemuDomainMachineIsG3Beige(def))) continue;

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bf1f0f1..8eb81c5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3731,6 +3731,12 @@ qemuDomainMachineIsG3Beige(const virDomainDef *def) }

+bool +qemuDomainMachineIsSun4u(const virDomainDef *def) +{ + return STREQ(def->os.machine, "sun4u"); +} +

Do we really need a function for this, as it's only used once? (no strong opinion either way).

/** * qemuDomainUpdateCurrentMemorySize: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1d2245b..d987c24 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -480,6 +480,7 @@ bool qemuDomainMachineIsI440FX(const virDomainDef *def); bool qemuDomainMachineNeedsFDC(const virDomainDef *def); bool qemuDomainMachineIsS390CCW(const virDomainDef *def); bool qemuDomainMachineIsG3Beige(const virDomainDef *def); +bool qemuDomainMachineIsSun4u(const virDomainDef *def);

int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, virDomainObjPtr vm);

On 11/20/2015 03:20 PM, Guido Günther wrote:

so handle it like I440FX --- src/qemu/qemu_command.c | 13 ++++++++----- src/qemu/qemu_domain.c | 8 ++++++++ src/qemu/qemu_domain.h | 1 + 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e467f9c..589aff5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1054,12 +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, G3Beige or Sun4u, the + /* for any machine based on I440FX, G3Beige, Sun4u or Malta, the * first (and currently only) IDE controller is an integrated * controller hardcoded with id "ide" */ if ((qemuDomainMachineIsI440FX(domainDef) || qemuDomainMachineIsSun4u(domainDef) || + qemuDomainMachineIsMalta(domainDef) || qemuDomainMachineIsG3Beige(domainDef)) && controller->idx == 0) return VIR_STRDUP(controller->info.alias, "ide"); @@ -4918,12 +4919,13 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,

case VIR_DOMAIN_CONTROLLER_TYPE_IDE: /* Since we currently only support the integrated IDE - * controller on 440fx, G3Beige and Sun4u, if we ever get to - * here, it's because some other machinetype had an IDE + * controller on 440fx, G3Beige, Sun4u and Malta, 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) || + qemuDomainMachineIsMalta(domainDef) || qemuDomainMachineIsSun4u(domainDef)) break;

... and of course the same comment here. Thanks again!

@@ -9908,11 +9910,12 @@ qemuBuildCommandLine(virConnectPtr conn, cont->idx == 0 && qemuDomainMachineIsQ35(def)) continue;

- /* first IDE controller on i440fx, G3Beige and Sun4u - * machines is implicit */ + /* first IDE controller on i440fx, G3Beige, Sun4u and + * Malta machines is implicit */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && (qemuDomainMachineIsI440FX(def) || qemuDomainMachineIsSun4u(def) || + qemuDomainMachineIsMalta(def) || qemuDomainMachineIsG3Beige(def))) continue;

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8eb81c5..3ca5fbd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3737,6 +3737,14 @@ qemuDomainMachineIsSun4u(const virDomainDef *def) return STREQ(def->os.machine, "sun4u"); }

+ +bool +qemuDomainMachineIsMalta(const virDomainDef *def) +{ + return STREQ(def->os.machine, "malta"); +} + + /** * qemuDomainUpdateCurrentMemorySize: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d987c24..af04d62 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -481,6 +481,7 @@ bool qemuDomainMachineNeedsFDC(const virDomainDef *def); bool qemuDomainMachineIsS390CCW(const virDomainDef *def); bool qemuDomainMachineIsG3Beige(const virDomainDef *def); bool qemuDomainMachineIsSun4u(const virDomainDef *def); +bool qemuDomainMachineIsMalta(const virDomainDef *def);

int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, virDomainObjPtr vm);