[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 1/1][RESEND] Optimize machine option to set more options with it.



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 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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]