[libvirt] [PATCHv2 1/2] Optimize machine option to set more options with it.
Li Zhang
zhlcindy at gmail.com
Thu Mar 28 03:08:01 UTC 2013
On 2013年03月27日 23:08, Martin Kletzander wrote:
> On 03/15/2013 10:19 AM, Li Zhang wrote:
>> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>
>> Currently, -machine option is used only when dump-guest-core is set.
>>
>> To use options defined in machine option for newer version of QEMU,
>> it needs to use -machine xxx, and to be compatible with older version
>> -M, this patch addes QEMU_CAPS_MACHINE_OPT capability for newer version,
>> say 1.2.0.
>>
>> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>> ---
>> v2 -> v1:
>> * Split the patch to 2 parts suggested by Daniel P.Berrange
>> * Rename QEMU_CAPS_MACH_OPT to QEMU_CAPS_MACHINE_OPT
>> * Remove version 1.1 assertion for QEMU_CAPS_MACHINE_OPT
>>
>> src/qemu/qemu_capabilities.c | 6 +++++-
>> src/qemu/qemu_capabilities.h | 1 +
>> src/qemu/qemu_command.c | 30 +++++++++++++++++++-----------
>> tests/qemuxml2argvtest.c | 6 +++---
>> 4 files changed, 28 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 519d2c5..778e825 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -210,7 +210,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>
>> "rng-random", /* 130 */
>> "rng-egd",
>> - "virtio-ccw"
>> + "virtio-ccw",
>> + "machine-opt"
>> );
>>
>> struct _virQEMUCaps {
>> @@ -2442,6 +2443,9 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>>
>> virQEMUCapsInitQMPBasic(qemuCaps);
>>
>> + /* machine option is supported for newer version */
>> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_OPT);
>> +
> But this is also supported when we don't probe by QMP.
Ah, okay, it should be set when using help string.
>
>> if (!(archstr = qemuMonitorGetTargetArch(mon)))
>> goto cleanup;
>>
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index da06e27..66df556 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -172,6 +172,7 @@ enum virQEMUCapsFlags {
>> virtio rng */
>> QEMU_CAPS_OBJECT_RNG_EGD = 131, /* EGD protocol daemon for rng */
>> QEMU_CAPS_VIRTIO_CCW = 132, /* -device virtio-*-ccw */
>> + QEMU_CAPS_MACHINE_OPT = 133, /* -machine xxxx*/
>>
>> QEMU_CAPS_LAST, /* this must always be the last item */
>> };
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index dc49d44..c39faf0 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4941,6 +4941,8 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>> const virDomainDefPtr def,
>> virQEMUCapsPtr qemuCaps)
>> {
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> /* This should *never* be NULL, since we always provide
>> * a machine in the capabilities data for QEMU. So this
>> * check is just here as a safety in case the unexpected
>> @@ -4948,27 +4950,33 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>> if (!def->os.machine)
>> return 0;
>>
>> - if (!def->mem.dump_core) {
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_OPT)) {
> This was intentionally done the way you see. I tried to avoid using
> '-machine' for various other qemu implementations (I couldn't find
> anywhere that '-machine' is guaranteed on all sorts of implementations).
> This is why the following comment is in place, which you obsoleted, but
> haven't removed, so it doesn't make much sense now.
I see. You are right, the comment should be removed.
>
> Also this won't work with implementations where we don't probe by QMP,
> but '-machine dump-core=xx' is supported.
Currently, it assumes that 1.2.0 supports QMP and -machine.
As you mentioned, I will add this capability when using help string.
Is it okay to support -machine with 1.1.0 onwards ?
>
>> /* if no parameter to the machine type is needed, we still use
>> * '-M' to keep the most of the compatibility with older versions.
>> */
>> virCommandAddArgList(cmd, "-M", def->os.machine, NULL);
>> } else {
>> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - "%s", _("dump-guest-core is not available "
>> - " with this QEMU binary"));
>> - return -1;
>> - }
>>
>> /* However, in case there is a parameter to be added, we need to
>> * use the "-machine" parameter because qemu is not parsing the
>> * "-M" correctly */
>> +
>> virCommandAddArg(cmd, "-machine");
>> - virCommandAddArgFormat(cmd,
>> - "%s,dump-guest-core=%s",
>> - def->os.machine,
>> - virDomainMemDumpTypeToString(def->mem.dump_core));
>> + virBufferAsprintf(&buf, "%s", def->os.machine);
>> +
>> + if (def->mem.dump_core) {
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
> There is also on pre-existing bug in my code which I see now. When
> probing by QMP, we don't set QEMU_CAPS_DUMP_GUEST_CORE flag and hence
> report this as unsupported even when the qemu knows this option :(
So it needs to add this capability when using QMP.
>
> However, I believe we have to do some more investigation and rely on
> version-specific capabilities only as a last resort. But because there
> is no way to get information about ',usb=off' and more of these are
> already in the code and coming, we will probably need to switch to
> '-machine' anyway.
Yes, you are right.
> Replying to your [2/2] here as well, with what kind of configuration do
> you experience such problems? I see no problems with upstream git QEMU
> (reporting itself as version 1.4.50), nor 1.4.0.
Ah, this is a problem on PowerPC.
To compare with PowerPC, X86 doesn't create USB controller in
machine->init().
X86 machine init code in QEMU is as the following:
if (pci_enabled && usb_enabled(false)) {
pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
}
But PowerPC init code in QEMU is as the following:
if (usb_enabled(spapr->has_graphics)) {
pci_create_simple(phb->bus, -1, "pci-ohci");
if (spapr->has_graphics) {
usbdevice_create("keyboard");
usbdevice_create("mouse");
}
}
When spapr->has_graphics is true, it will create one USB controller
implicitly.
But in libvirt, it also creates one USB controller implicitly if no USB
controller specified.
So this will cause errors.
I think you can also get errors by hacking X86 QEMU code with
usb_enabled(true)
If with ,usb=off, it will get false with (usb_enabled(spapr->has_graphics)).
Then libvirt can use its own controller created implicitly.
Thanks.
>
> Thanks,
> Martin
More information about the libvir-list
mailing list