Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 1/1] hw/i386/pc_piix.c: Also pass vmport=off for xenfv machine

[Don Slutz <dslutz@verizon.com>]

On 11/19/14 13:08, Paolo Bonzini wrote:
>
> On 19/11/2014 19:07, Don Slutz wrote:
>>> "-M pc -machine accel=xen" should work and, if that's what you want,
>>> disable the vmport device.  I think this patch is wrong.
>>>
>>> Paolo
>> Well, I also want "-M pc -machine accel=xen,vmport=on" to work.
> Right.  So let's start by deciding what the desired semantics are for
> all six cases: -M pc/xenfv, -machine vmport=on/off/absent.
>
> Paolo

I get 12 cases (PCMachineState *pcms = PC_MACHINE(obj)):



-M pc
     pcms->vmport is true
-M pc -machine vmport=on
     pcms->vmport is true
-M pc -machine vmport=off
     pcms->vmport is false
-M xenfv
     pcms->vmport is false
-M xenfv -machine vmport=on
     pcms->vmport is true
-M xenfv -machine vmport=off
     pcms->vmport is false

-M pc -machine accel=xen
     pcms->vmport is false
-M pc -machine vmport=on,accel=xen
     pcms->vmport is true
-M pc -machine vmport=off,accel=xen
     pcms->vmport is false
-M xenfv -machine accel=xen
     pcms->vmport is false
-M xenfv -machine vmport=on,accel=xen
     pcms->vmport is true
-M xenfv -machine vmport=off,accel=xen
     pcms->vmport is false


Which look like to me to solve down to xen_init() called via 
xen_accel_class_init()
need to see if vmport has been specified and change it if needed.

Which leads me to:

commit 436d056420bb209d066d2378b628d297777d9f20
Author: Don Slutz <dslutz@verizon.com>
Date:   Wed Nov 19 10:01:16 2014 -0500

     hw/i386/pc_piix.c: Also pass vmport=off for xenfv machine

     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().

     Add vmport_changed to track the state of vmport so that
     accel=xen can do the right thing.

     Drop the call to xen_enabled() in pc_machine_initfn() to reduce
     potential confusion.

     Signed-off-by: Don Slutz <dslutz@verizon.com>

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..b400ac8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1723,6 +1723,7 @@ static void pc_machine_set_vmport(Object *obj, 
bool value, Error **errp)
      PCMachineState *pcms = PC_MACHINE(obj);

      pcms->vmport = value;
+    pcms->vmport_changed = true;
  }

  static void pc_machine_initfn(Object *obj)
@@ -1737,7 +1738,7 @@ 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();
+    pcms->vmport = true;
      object_property_add_bool(obj, PC_MACHINE_VMPORT,
                               pc_machine_get_vmport,
                               pc_machine_set_vmport,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7c3731f..5782406 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -38,6 +38,7 @@ struct PCMachineState {

      uint64_t max_ram_below_4g;
      bool vmport;
+    bool vmport_changed;
  };

  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
diff --git a/xen-common.c b/xen-common.c
index 56359ca..bb11bb7 100644
--- a/xen-common.c
+++ b/xen-common.c
@@ -12,6 +12,7 @@
  #include "qmp-commands.h"
  #include "sysemu/char.h"
  #include "sysemu/accel.h"
+#include "hw/i386/pc.h"

  //#define DEBUG_XEN

@@ -112,6 +113,11 @@ static void xen_change_state_handler(void *opaque, 
int running,

  static int xen_init(MachineState *ms)
  {
+    PCMachineState *pcms = PC_MACHINE(ms);
+
+    if (!pcms->vmport_changed) {
+        pcms->vmport = false;
+    }
      xen_xc = xen_xc_interface_open(0, 0, 0);
      if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
          xen_be_printf(NULL, 0, "can't open xen interface\n");


    -Don Slutz



>> However you bring up a good point and I will see if I can quickly handled it.