[Qemu-devel] [BUGFIX][PATCH for 2.2 v6 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Qemu-devel] [BUGFIX][PATCH for 2.2 v6 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Don Slutz]

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.  Move check of xen_enabled() back to pc_init1().

Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Eric Blake <eblake@redhat.com>
---
v6:

  Added the sentence: "Move check of xen_enabled() back to pc_init1()."
  to the commit message.

  I considered folding pc_machine_set_vmport and
  pc_machine_get_vmport into 1 routine.  Desided that for 2.2 I
  would keep the extra safety of passing a copy of vmport in the get
  routine.  I do not think is is needed but without a comment
  about this I took the simpler change.

  Eduardo Habkost:
    Use visit_type_OnOffAuto not visit_type_enum.
      Done -- Needed to add #include "qapi-visit.h"
    Adjust usage of visit_type_OnOffAuto to avoid undefined
    values.
      Done
    Change the property from "str" to "OnOffAuto"
      Done.

v5:
  Eduardo Habkost:
    What about changing pc_machine->vmport here instead of using a
    no_vmport variable, so the actual vmport configuration may be
    queried by anybody later using the QOM property? It would even
    make the code shorter.
      Done.
  Eric Blake:
    I've only reviewed the qapi/common.json and qemu-options.hx
    files for QMP interface (and will leave the rest of the patch to
    others), but I'm okay with the changes to those files. I guess
    that means no R-b, since I didn't do a full review, so here's a
    weaker acked-by.

v4:
  Michael S. Tsirkin, Eric Blake, Eduardo Habkost:
    Rename vmport to OnOffAuto and move to qapi/common.json
  Eduardo Habkost:
    Simpler convert of enum to no_vmport.
  Michael S. Tsirkin:
    Add assert for ON_OFF_AUTO_MAX.

 hw/i386/pc.c         | 22 +++++++++++++---------
 hw/i386/pc_piix.c    |  7 ++++++-
 hw/i386/pc_q35.c     |  7 ++++++-
 include/hw/i386/pc.h |  2 +-
 qapi/common.json     | 15 +++++++++++++++
 qemu-options.hx      |  8 +++++---
 vl.c                 |  2 +-
 7 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..10c1fa5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -61,6 +61,7 @@
 #include "hw/mem/pc-dimm.h"
 #include "trace.h"
 #include "qapi/visitor.h"
+#include "qapi-visit.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -1711,18 +1712,21 @@ 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);
+    OnOffAuto vmport = pcms->vmport;
 
-    return pcms->vmport;
+    visit_type_OnOffAuto(v, &vmport, 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);
 
-    pcms->vmport = value;
+    visit_type_OnOffAuto(v, &pcms->vmport, name, errp);
 }
 
 static void pc_machine_initfn(Object *obj)
@@ -1737,11 +1741,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 = ON_OFF_AUTO_AUTO;
+    object_property_add(obj, PC_MACHINE_VMPORT, "OnOffAuto",
+                        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..fffaab7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -234,9 +234,14 @@ static void pc_init1(MachineState *machine,
 
     pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
 
+    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
+    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
+        pc_machine->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
+    }
+
     /* init basic PC hardware */
     pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
-                         !pc_machine->vmport, 0x4);
+                         (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
 
     pc_nic_init(isa_bus, pci_bus);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..88cee93 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -242,9 +242,14 @@ static void pc_q35_init(MachineState *machine)
 
     pc_register_ferr_irq(gsi[13]);
 
+    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
+    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
+        pc_machine->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
+    }
+
     /* init basic PC hardware */
     pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
-                         !pc_machine->vmport, 0xff0104);
+                         (pc_machine->vmport != ON_OFF_AUTO_ON), 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..7f7d2d4 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;
+    OnOffAuto vmport;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
diff --git a/qapi/common.json b/qapi/common.json
index 4e9a21f..63ef3b4 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -87,3 +87,18 @@
 ##
 { 'command': 'query-commands', 'returns': ['CommandInfo'] }
 
+##
+# @OnOffAuto
+#
+# An enumeration of three options: on, off, and auto
+#
+# @auto: QEMU selects the value between on and off
+#
+# @on: Enabled
+#
+# @off: Disabled
+#
+# Since: 2.2
+##
+{ 'enum': 'OnOffAuto',
+  '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.
-@item vmport=on|off
-Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default)
+@item 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

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v6 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Eduardo Habkost]

On Fri, Nov 21, 2014 at 11:18:52AM -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.  Move check of xen_enabled() back to pc_init1().
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> Acked-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks for your patience with the many requests for changes.

-- 
Eduardo

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v6 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Paolo Bonzini]

On 21/11/2014 17:18, 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.  Move check of xen_enabled() back to pc_init1().
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> Acked-by: Eric Blake <eblake@redhat.com>
> ---
> v6:
> 
>   Added the sentence: "Move check of xen_enabled() back to pc_init1()."
>   to the commit message.
> 
>   I considered folding pc_machine_set_vmport and
>   pc_machine_get_vmport into 1 routine.  Desided that for 2.2 I
>   would keep the extra safety of passing a copy of vmport in the get
>   routine.  I do not think is is needed but without a comment
>   about this I took the simpler change.
> 
>   Eduardo Habkost:
>     Use visit_type_OnOffAuto not visit_type_enum.
>       Done -- Needed to add #include "qapi-visit.h"
>     Adjust usage of visit_type_OnOffAuto to avoid undefined
>     values.
>       Done
>     Change the property from "str" to "OnOffAuto"
>       Done.
> 
> v5:
>   Eduardo Habkost:
>     What about changing pc_machine->vmport here instead of using a
>     no_vmport variable, so the actual vmport configuration may be
>     queried by anybody later using the QOM property? It would even
>     make the code shorter.
>       Done.
>   Eric Blake:
>     I've only reviewed the qapi/common.json and qemu-options.hx
>     files for QMP interface (and will leave the rest of the patch to
>     others), but I'm okay with the changes to those files. I guess
>     that means no R-b, since I didn't do a full review, so here's a
>     weaker acked-by.
> 
> v4:
>   Michael S. Tsirkin, Eric Blake, Eduardo Habkost:
>     Rename vmport to OnOffAuto and move to qapi/common.json
>   Eduardo Habkost:
>     Simpler convert of enum to no_vmport.
>   Michael S. Tsirkin:
>     Add assert for ON_OFF_AUTO_MAX.
> 
>  hw/i386/pc.c         | 22 +++++++++++++---------
>  hw/i386/pc_piix.c    |  7 ++++++-
>  hw/i386/pc_q35.c     |  7 ++++++-
>  include/hw/i386/pc.h |  2 +-
>  qapi/common.json     | 15 +++++++++++++++
>  qemu-options.hx      |  8 +++++---
>  vl.c                 |  2 +-
>  7 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 1205db8..10c1fa5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -61,6 +61,7 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "trace.h"
>  #include "qapi/visitor.h"
> +#include "qapi-visit.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -1711,18 +1712,21 @@ 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);
> +    OnOffAuto vmport = pcms->vmport;
>  
> -    return pcms->vmport;
> +    visit_type_OnOffAuto(v, &vmport, 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);
>  
> -    pcms->vmport = value;
> +    visit_type_OnOffAuto(v, &pcms->vmport, name, errp);
>  }
>  
>  static void pc_machine_initfn(Object *obj)
> @@ -1737,11 +1741,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 = ON_OFF_AUTO_AUTO;
> +    object_property_add(obj, PC_MACHINE_VMPORT, "OnOffAuto",
> +                        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..fffaab7 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -234,9 +234,14 @@ static void pc_init1(MachineState *machine,
>  
>      pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>  
> +    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
> +    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
> +        pc_machine->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
> +    }
> +
>      /* init basic PC hardware */
>      pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> -                         !pc_machine->vmport, 0x4);
> +                         (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
>  
>      pc_nic_init(isa_bus, pci_bus);
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 598e679..88cee93 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -242,9 +242,14 @@ static void pc_q35_init(MachineState *machine)
>  
>      pc_register_ferr_irq(gsi[13]);
>  
> +    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
> +    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
> +        pc_machine->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
> +    }
> +
>      /* init basic PC hardware */
>      pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> -                         !pc_machine->vmport, 0xff0104);
> +                         (pc_machine->vmport != ON_OFF_AUTO_ON), 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..7f7d2d4 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;
> +    OnOffAuto vmport;
>  };
>  
>  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> diff --git a/qapi/common.json b/qapi/common.json
> index 4e9a21f..63ef3b4 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -87,3 +87,18 @@
>  ##
>  { 'command': 'query-commands', 'returns': ['CommandInfo'] }
>  
> +##
> +# @OnOffAuto
> +#
> +# An enumeration of three options: on, off, and auto
> +#
> +# @auto: QEMU selects the value between on and off
> +#
> +# @on: Enabled
> +#
> +# @off: Disabled
> +#
> +# Since: 2.2
> +##
> +{ 'enum': 'OnOffAuto',
> +  '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.
> -@item vmport=on|off
> -Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default)
> +@item 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",
> 

Thanks, applied.

Paolo

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v6 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Fabio Fantoni]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

> On Fri, Nov 21, 2014 at 11:18:52AM -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.  Move check of xen_enabled() back to pc_init1()./
> >/  /
> >/  Signed-off-by: Don Slutz <address@hidden>/
> >/  Acked-by: Eric Blake <address@hidden>/
>
> Reviewed-by: Eduardo Habkost <address@hidden>
>
> Thanks for your patience with the many requests for changes.
>
> -- 
> Eduardo

This patch need more testing before applying it?
If yes I'll do a fast test ASAP.

Thanks for any reply.


[-- Attachment #2: Type: text/html, Size: 1245 bytes --]