[libvirt] [PATCHv2 1/1][RESEND] Optimize machine option to set more options with it.
Li Zhang
zhlcindy at gmail.com
Fri Mar 15 03:14:45 UTC 2013
On 2013年03月14日 19:11, Daniel P. Berrange wrote:
> On Thu, Mar 14, 2013 at 02:54:20PM +0800, Li Zhang wrote:
>> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>
>> Currently, -machine option is used only when dump-guest-core is used.
>>
>> To use options defined in machine option for new version of QEMU,
>> it needs to use -machine xxx, and to be compatible with older version
>> -M, this patch addes QEMU_CAPS_MACH_OPT capability, and assumes
>> -machine is used for QEMU v1.0 onwards.
>>
>> To avoid the collision for creating USB controllers when using USB
>> option and -device xxxx, it needs to set usb=off in machine option.
>> QEMU_CAPS_USB_OPT capability is added, and it will be for QEMU
>> v1.3.0-rc0 onwards which supports USB option.
> I'd rather see this patch split into two parts. One part to introduce
> the use of -machine, and a second patch to deal with the usb=off part.
OK, I will split it.
>
> That said I'm not sure why we need to set usb=off at all - we already run
> qemu with -nodefconfig -nodefaults which should be blocking the default
> USB controller.
Here is the reason.
In QEMU,
vl.c defines one usb_enable() which is to set USB option.
Actually, this USB option is one of global machine options.
-nodefaults doesn't set this option.
bool usb_enabled(bool default_usb)
{
QemuOpts *mach_opts;
mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
if (mach_opts) {
return qemu_opt_get_bool(mach_opts, "usb", default_usb);
}
return default_usb;
}
In machine->init(),
For sPAPR, it will create USB controller according to this option.
if (usb_enabled(spapr->has_graphics)) {
pci_create_simple(phb->bus, -1, "pci-ohci");
if (spapr->has_graphics) {
usbdevice_create("keyboard");
usbdevice_create("mouse");
}
}
This code says that, it will create one USB controller if VGA is enabled.
With this code, libvirt also create another USB controller by -device xxxx.
Then QEMU will report one error.
Actually, if with legacy USB (-usb), this error won't occur.
Because -usb is just to set USB option as on(usb=on).
As another patch of mine, I also want to use legacy USB as default
which will be created int machine->init() when there is no specific
controller is defined in XML.
>
>> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>> ---
>> src/qemu/qemu_capabilities.c | 10 ++++++++++
>> src/qemu/qemu_capabilities.h | 2 ++
>> src/qemu/qemu_command.c | 36 +++++++++++++++++++++++++-----------
>> tests/qemuxml2argvtest.c | 4 ++--
>> 4 files changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 51fc9dc..79eb83f 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -205,6 +205,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>> "usb-serial", /* 125 */
>> "usb-net",
>> "add-fd",
>> + "mach-opt",
>> + "usb-opt",
>>
>> );
>>
>> @@ -2416,6 +2418,14 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>>
>> virQEMUCapsInitQMPBasic(qemuCaps);
>>
>> + /* Assuming to use machine option v1.0 onwards*/
>> + if (qemuCaps->version >= 1000000)
>> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACH_OPT);
>> +
>> + /* USB option is supported v1.3.0-rc0 onwards */
>> + if (qemuCaps->version >= 1002090)
>> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_USB_OPT);
>> +
>> if (!(archstr = qemuMonitorGetTargetArch(mon)))
>> goto cleanup;
> If we get to this aprt of virQEMUCapsInitQMP() code path, then we already
> know we have a QEMU >= 1.2.0, so this first version check is not required.
> Just set the QEMU_CAPS_MACH_OPT in virQEMUCapsInitQMPBasic.
Oh, maybe I should set this version as 1.3.0.
The official release of 1.2.0 doesn't support this option AFAIK.
>
> For the USB property just use version 1.3.0 - don't try to do stuff
> based on development snapshot version numbers.
OK, I will correct it.
Thanks. :)
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index e69d558..06aaa68 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -166,6 +166,8 @@ enum virQEMUCapsFlags {
>> QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */
>> QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
>> QEMU_CAPS_ADD_FD = 127, /* -add-fd */
>> + QEMU_CAPS_MACH_OPT = 128, /* -machine xxxx*/
> We don't need to abbreviate - use QEMU_CAPS_MACHINE_OPT
OK, I will correct it.
>
>> + QEMU_CAPS_USB_OPT = 129, /* -machine xxxx,usb=off*/
> I'd prefer this second one to be QEMU_CAPS_MACHINE_USB_OPT
OK, I will correct.
Thanks a lot for your review. :)
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 6c28123..e7dde21 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4614,6 +4614,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
>> @@ -4621,27 +4623,39 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>> if (!def->os.machine)
>> return 0;
>>
>> - if (!def->mem.dump_core) {
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACH_OPT)) {
>> /* 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);
>> +
>> + /* To avoid the collision of creating USB controllers when calling
>> + * machine->init in QEMU, it needs to set usb=off
>> + */
>> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_OPT))
>> + virBufferAsprintf(&buf, ",usb=off");
>> +
>> + if (def->mem.dump_core) {
>> + 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;
>> + }
>> +
>> + virBufferAsprintf(&buf, ",dump-guest-core=%s",
>> + virDomainMemDumpTypeToString(def->mem.dump_core));
>> + }
>> +
>> + virCommandAddArg(cmd, virBufferContentAndReset(&buf));
>> }
>>
> Daniel
More information about the libvir-list
mailing list