qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fi


From: Don Slutz
Subject: Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
Date: Thu, 20 Nov 2014 12:40:11 -0500
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0

On 11/20/14 03:44, Michael S. Tsirkin wrote:
On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4

or

c/s b154537ad07598377ebf98252fb7d2aff127983b

moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().

xen_enabled() does not return the correct value in
pc_machine_initfn().

Changed vmport from a bool to an enum.  Added the value "auto" to do
the old way.

Signed-off-by: Don Slutz <address@hidden>
This looks fine to me. A couple of minor comments below.
Also this changes qapi schema file, let's get ack from maintainers -
my understanding is that just adding a definition there won't
affect any users, correct?


---
  hw/i386/pc.c         | 23 ++++++++++++++---------
  hw/i386/pc_piix.c    | 27 ++++++++++++++++++++++++++-
  hw/i386/pc_q35.c     | 27 ++++++++++++++++++++++++++-
  include/hw/i386/pc.h |  2 +-
  qapi-schema.json     | 16 ++++++++++++++++
  qemu-options.hx      |  8 +++++---
  vl.c                 |  2 +-
  7 files changed, 89 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..2f68e4d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object 
*obj, Visitor *v,
      pcms->max_ram_below_4g = value;
  }
-static bool pc_machine_get_vmport(Object *obj, Error **errp)
+static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
  {
      PCMachineState *pcms = PC_MACHINE(obj);
+    int vmport = pcms->vmport;
- return pcms->vmport;
+    visit_type_enum(v, &vmport, vmport_lookup, NULL, name, errp);
  }
-static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
+static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
  {
      PCMachineState *pcms = PC_MACHINE(obj);
+    int vmport;
- pcms->vmport = value;
+    visit_type_enum(v, &vmport, vmport_lookup, NULL, name, errp);
+    pcms->vmport = vmport;
  }
static void pc_machine_initfn(Object *obj)
@@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
                          pc_machine_get_max_ram_below_4g,
                          pc_machine_set_max_ram_below_4g,
                          NULL, NULL, NULL);
-    pcms->vmport = !xen_enabled();
-    object_property_add_bool(obj, PC_MACHINE_VMPORT,
-                             pc_machine_get_vmport,
-                             pc_machine_set_vmport,
-                             NULL);
+    pcms->vmport = VMPORT_AUTO;
+    object_property_add(obj, PC_MACHINE_VMPORT, "str",
+                        pc_machine_get_vmport,
+                        pc_machine_set_vmport,
+                        NULL, NULL, NULL);
  }
static void pc_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7bb97a4..81a7b83 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -101,6 +101,7 @@ static void pc_init1(MachineState *machine,
      FWCfgState *fw_cfg = NULL;
      PcGuestInfo *guest_info;
      ram_addr_t lowmem;
+    bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
       * If it doesn't, we need to split it in chunks below and above 4G.
@@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
Probably should be assert(pc_machine->vmport != VMPORT_MAX) -
we never get any values except on/off/auto.

Ok, added the assert.

+    if (xen_enabled()) {
+        switch (pc_machine->vmport) {
+        case VMPORT_MAX:
+        case VMPORT_AUTO:
+        case VMPORT_OFF:
+            no_vmport = true;
+            break;
+        case VMPORT_ON:
+            no_vmport = false;
+            break;
+        }
+    } else {
+        switch (pc_machine->vmport) {
+        case VMPORT_MAX:
+        case VMPORT_OFF:
+            no_vmport = true;
+            break;
+        case VMPORT_AUTO:
+        case VMPORT_ON:
+            no_vmport = false;
+            break;
+        }
+    }
+
Can't above be moved to a function in pc.c, and be reused between piix
and q35? It's big enough to avoid open-coding, IMHO.


I feel that the what Eduardo Habkost provided:

+    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
+    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
+        no_vmport = xen_enabled();
+    } else {
+        no_vmport = (pc_machine->vmport == ON_OFF_AUTO_ON);
+    }

is short enough to not need it's own function. But I can do this if still needed.

      /* init basic PC hardware */
      pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
-                         !pc_machine->vmport, 0x4);
+                         no_vmport, 0x4);
pc_nic_init(isa_bus, pci_bus); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..4f932d6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
      PcGuestInfo *guest_info;
      ram_addr_t lowmem;
      DriveInfo *hd[MAX_SATA_PORTS];
+    bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
       * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -242,9 +243,33 @@ static void pc_q35_init(MachineState *machine)
pc_register_ferr_irq(gsi[13]); + if (xen_enabled()) {
+        switch (pc_machine->vmport) {
+        case VMPORT_MAX:
+        case VMPORT_AUTO:
+        case VMPORT_OFF:
+            no_vmport = true;
+            break;
+        case VMPORT_ON:
+            no_vmport = false;
+            break;
+        }
+    } else {
+        switch (pc_machine->vmport) {
+        case VMPORT_MAX:
+        case VMPORT_OFF:
+            no_vmport = true;
+            break;
+        case VMPORT_AUTO:
+        case VMPORT_ON:
+            no_vmport = false;
+            break;
+        }
+    }
+
      /* init basic PC hardware */
      pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
-                         !pc_machine->vmport, 0xff0104);
+                         no_vmport, 0xff0104);
/* connect pm stuff to lpc */
      ich9_lpc_pm_init(lpc);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7c3731f..d7d8f30 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -37,7 +37,7 @@ struct PCMachineState {
      ISADevice *rtc;
uint64_t max_ram_below_4g;
-    bool vmport;
+    vmport vmport;
  };
#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
diff --git a/qapi-schema.json b/qapi-schema.json
index d0926d9..f7ee3ad 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3513,3 +3513,19 @@
  # Since: 2.1
  ##
  { 'command': 'rtc-reset-reinjection' }
+
+##
+# @vmport
+#
+# An enumeration of the options on enabling of VMWare ioport emulation
+#
+# @auto: system selects the old default
+#
+# @on: provide the vmport device
+#
+# @off: do not provide the vmport device
+#
+# Since: 2.2
+##
+{ 'enum': 'vmport',

vmport as type name violates our coding style.
Should be VMPort.

But in fact, this is a generic OnOffAuto type, isn't it?
Maybe it should be named like this, and go into qapi/common.json?
I think it might be a good idea but it's not a must.


This looks like the name I will go with.

    -Don Slutz

+  'data': [ 'auto', 'on', 'off' ] }
diff --git a/qemu-options.hx b/qemu-options.hx
index da9851d..64af16d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
      "                property accel=accel1[:accel2[:...]] selects 
accelerator\n"
      "                supported accelerators are kvm, xen, tcg (default: 
tcg)\n"
      "                kernel_irqchip=on|off controls accelerated irqchip 
support\n"
-    "                vmport=on|off controls emulation of vmport (default: 
on)\n"
+    "                vmport=on|off|auto controls emulation of vmport (default: 
auto)\n"
      "                kvm_shadow_mem=size of KVM shadow MMU\n"
      "                dump-guest-core=on|off include guest memory in a core dump 
(default=on)\n"
      "                mem-merge=on|off controls memory merge support (default: 
on)\n"
@@ -52,8 +52,10 @@ than one accelerator specified, the next one is used if the 
previous one fails
  to initialize.
  @item kernel_irqchip=on|off
  Enables in-kernel irqchip support for the chosen accelerator when available.
address@hidden vmport=on|off
-Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default)
address@hidden vmport=on|off|auto
+Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
+value based on accel. For accel=xen the default is off otherwise the default
+is on.
  @item kvm_shadow_mem=size
  Defines the size of the KVM shadow MMU.
  @item dump-guest-core=on|off
diff --git a/vl.c b/vl.c
index f4a6e5e..eb89d62 100644
--- a/vl.c
+++ b/vl.c
@@ -381,7 +381,7 @@ static QemuOptsList qemu_machine_opts = {
              .help = "maximum ram below the 4G boundary (32bit boundary)",
          }, {
              .name = PC_MACHINE_VMPORT,
-            .type = QEMU_OPT_BOOL,
+            .type = QEMU_OPT_STRING,
              .help = "Enable vmport (pc & q35)",
          },{
              .name = "iommu",
--
1.8.4




reply via email to

[Prev in Thread] Current Thread [Next in Thread]