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 10:02:55 -0500
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0

On 11/19/14 19:58, Eduardo Habkost wrote:
On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
[...]
@@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL); + 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, 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;
+        }
+    }
What about simplifying those 24 lines to the following 5 lines:

     if (pc_machine->vmport == VMPORT_AUTO) {
           no_vmport = xen_enabled();
     } else {
           no_vmport = (pc_machine->vmport == VMPORT_ON);
     }


Looks much better.  Will switch to this.

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',
Coding style for enum typedefs is CamelCase.

Probably something more descriptive than just "VMPort" would be better.
This enum does not describe the vmport itself, but just the on/off/auto
setting. Maybe VMPortConfig?

That would be fine with me.

+  'data': [ 'auto', 'on', 'off' ] }
I believe there may be other properties with exactly the same behavior.
Maybe we could use a generic enum name, like "Tristate"?

But we have a problem, here: the existing code accepts "on", "yes" and
"true" for boolean values. Now it is going to accept only "on' and
"off". This breaks compatibility.

This is new at 2.2, so I think we can adjust either way (just the 3 or add
others).

Maybe my enum suggestion was not a very good one? I would like to hear
opinions from others.

In either case, I am sure your previous bugfix is more appropriate for
2.2. Changing the data type of the vmport property is much more
intrusive than just allowing it to return an incorrect value temporarily
between instance_init and machine->init().


I can post the v2 if that makes sense (as v4?) will wait a little while. If the v2
is the one in 2.2 then allowing the other options may make sense.

    -Don Slutz




reply via email to

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