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

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

[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() so add vmport=off to xenfv machine to correctly
turn off vmport for xen.

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

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

The 1st version of "-machine vmport=off: Allow..." patch did not
have this issue.  It was the adjusting to the QOM that introduced
it.

 hw/i386/pc.c      | 2 +-
 hw/i386/pc_piix.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..5bfece8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1737,7 +1737,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/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7bb97a4..3268c29 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -914,7 +914,7 @@ static QEMUMachine xenfv_machine = {
     .desc = "Xen Fully-virtualized PC",
     .init = pc_xen_hvm_init,
     .max_cpus = HVM_MAX_VCPUS,
-    .default_machine_opts = "accel=xen",
+    .default_machine_opts = "accel=xen,vmport=off",
     .hot_add_cpu = pc_hot_add_cpu,
 };
 #endif
-- 
1.8.4

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

[Paolo Bonzini]

On 19/11/2014 18:30, 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() so add vmport=off to xenfv machine to correctly
> turn off vmport for xen.
> 
> Drop the call to xen_enabled() in pc_machine_initfn() to reduce
> potential confusion.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
> 
> The 1st version of "-machine vmport=off: Allow..." patch did not
> have this issue.  It was the adjusting to the QOM that introduced
> it.
> 
>  hw/i386/pc.c      | 2 +-
>  hw/i386/pc_piix.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 1205db8..5bfece8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1737,7 +1737,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/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7bb97a4..3268c29 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -914,7 +914,7 @@ static QEMUMachine xenfv_machine = {
>      .desc = "Xen Fully-virtualized PC",
>      .init = pc_xen_hvm_init,
>      .max_cpus = HVM_MAX_VCPUS,
> -    .default_machine_opts = "accel=xen",
> +    .default_machine_opts = "accel=xen,vmport=off",
>      .hot_add_cpu = pc_hot_add_cpu,
>  };
>  #endif
> 

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

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

[Don Slutz]

On 11/19/14 12:35, Paolo Bonzini wrote:
>
> On 19/11/2014 18:30, 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() so add vmport=off to xenfv machine to correctly
>> turn off vmport for xen.
>>
>> Drop the call to xen_enabled() in pc_machine_initfn() to reduce
>> potential confusion.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>
>> The 1st version of "-machine vmport=off: Allow..." patch did not
>> have this issue.  It was the adjusting to the QOM that introduced
>> it.
>>
>>   hw/i386/pc.c      | 2 +-
>>   hw/i386/pc_piix.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 1205db8..5bfece8 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1737,7 +1737,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/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 7bb97a4..3268c29 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -914,7 +914,7 @@ static QEMUMachine xenfv_machine = {
>>       .desc = "Xen Fully-virtualized PC",
>>       .init = pc_xen_hvm_init,
>>       .max_cpus = HVM_MAX_VCPUS,
>> -    .default_machine_opts = "accel=xen",
>> +    .default_machine_opts = "accel=xen,vmport=off",
>>       .hot_add_cpu = pc_hot_add_cpu,
>>   };
>>   #endif
>>
> "-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. However you
bring up a good point and I will see if I can quickly handled it.

    -Don Slutz

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

[Paolo Bonzini]

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

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

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

[Don Slutz]

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.

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

[Don Slutz]

P.S. I think the title:

xen-common.c: If "-machine vmport=" is not specified turn vmport off

is better.  Will send v2 of patch out soon.

    -Don Slutz

On 11/19/14 14:08, Don Slutz wrote:
>
> 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.
>

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

[Eduardo Habkost]

On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote:
> 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:
> 
[...]
> 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;

So, now the setting have three possible states: unset, on, and off.

If we can't avoid that (which seems to be the case, as "accel" is
changed after we already set the default value for "vmport" in instance
init), maybe we should make it an enum property that accepts on/off/auto
as values, instead of faking a boolean property that is not really a
boolean?

-- 
Eduardo

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

[Don Slutz]

On 11/19/14 14:30, Eduardo Habkost wrote:
> On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote:
>> 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:
>>
> [...]
>> 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;
> So, now the setting have three possible states: unset, on, and off.
>
> If we can't avoid that (which seems to be the case, as "accel" is
> changed after we already set the default value for "vmport" in instance
> init), maybe we should make it an enum property that accepts on/off/auto
> as values, instead of faking a boolean property that is not really a
> boolean?
>

I would be happy to look into this.  I have not done anything that used
an enum property, and so it is hard to guess on how long it will take to
convert the change to this.

    -Don Slutz

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

[Eduardo Habkost]

On Wed, Nov 19, 2014 at 03:01:24PM -0500, Don Slutz wrote:
> On 11/19/14 14:30, Eduardo Habkost wrote:
> >On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote:
> >>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:
> >>
> >[...]
> >>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;
> >So, now the setting have three possible states: unset, on, and off.
> >
> >If we can't avoid that (which seems to be the case, as "accel" is
> >changed after we already set the default value for "vmport" in instance
> >init), maybe we should make it an enum property that accepts on/off/auto
> >as values, instead of faking a boolean property that is not really a
> >boolean?
> >
> 
> I would be happy to look into this.  I have not done anything that used
> an enum property, and so it is hard to guess on how long it will take to
> convert the change to this.

I think your patch may be a good intermediate solution, as long as you
add a comment noting that the return value of the "vmport" property may
be invalid before machine->init() is called, depending on the
machine-type.

Or, maybe a property without a getter would be appropriate?

-- 
Eduardo

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

[Don Slutz]

On 11/19/14 19:24, Eduardo Habkost wrote:
> On Wed, Nov 19, 2014 at 03:01:24PM -0500, Don Slutz wrote:
>> On 11/19/14 14:30, Eduardo Habkost wrote:
>>> On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote:
>>>> 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:
>>>>
>>> [...]
>>>> 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;
>>> So, now the setting have three possible states: unset, on, and off.
>>>
>>> If we can't avoid that (which seems to be the case, as "accel" is
>>> changed after we already set the default value for "vmport" in instance
>>> init), maybe we should make it an enum property that accepts on/off/auto
>>> as values, instead of faking a boolean property that is not really a
>>> boolean?
>>>
>> I would be happy to look into this.  I have not done anything that used
>> an enum property, and so it is hard to guess on how long it will take to
>> convert the change to this.
> I think your patch may be a good intermediate solution, as long as you
> add a comment noting that the return value of the "vmport" property may
> be invalid before machine->init() is called, depending on the
> machine-type.
>
> Or, maybe a property without a getter would be appropriate?
>

Timing is everything.  The convert to enum was not too hard.  v3 posted.

    -Don Slutz

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

[Paolo Bonzini]

On 19/11/2014 20:08, Don Slutz wrote:
> 
> -M pc -machine accel=xen
>     pcms->vmport is false

I think this should be true.  Any reason why not?

Paolo

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

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

[Don Slutz]

On 11/20/14 01:02, Paolo Bonzini wrote:
>
> On 19/11/2014 20:08, Don Slutz wrote:
>> -M pc -machine accel=xen
>>      pcms->vmport is false
> I think this should be true.  Any reason why not?
>
> Paolo

Yes, QEMU will crash if xen is enabled and the guest tries to access the 
VMware port.

(more on different thread).

    -Don Slutz

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

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

[Eduardo Habkost]

On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote:
> 
> 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)):

We have more cases, if we consider "-M pc-2.1" too.

With this first patch (the one changing default_machine_opts), I expect
to get the following results:

-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 pc-2.1
    pcms->vmport is true
-M pc-2.1 -machine vmport=on
    pcms->vmport is true
    (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
-M pc-2.1 -machine vmport=off
    pcms->vmport is false
    (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
-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 true
-M pc -machine vmport=on,accel=xen
    pcms->vmport is true
-M pc -machine vmport=off,accel=xen
    pcms->vmport is false
-M pc-2.1 -machine accel=xen
    pcms->vmport is true **
-M pc-2.1 -machine vmport=on,accel=xen
    pcms->vmport is true
    (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
-M pc-2.1 -machine vmport=off,accel=xen
    pcms->vmport is false
    (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
-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

I believe there's no consensus yet about the one marked with "**" above. It
boils to the question: do we need to keep guest ABI stability when using
"-M pc-2.1 -machine accel=xen"?

-- 
Eduardo

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

[Don Slutz]

On 11/20/14 10:13, Eduardo Habkost wrote:
> On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote:
>> 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)):
> We have more cases, if we consider "-M pc-2.1" too.
>
> With this first patch (the one changing default_machine_opts), I expect
> to get the following results:
>
> -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 pc-2.1
>      pcms->vmport is true
> -M pc-2.1 -machine vmport=on
>      pcms->vmport is true
>      (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
> -M pc-2.1 -machine vmport=off
>      pcms->vmport is false
>      (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
> -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 true
> -M pc -machine vmport=on,accel=xen
>      pcms->vmport is true
> -M pc -machine vmport=off,accel=xen
>      pcms->vmport is false
> -M pc-2.1 -machine accel=xen
>      pcms->vmport is true **
> -M pc-2.1 -machine vmport=on,accel=xen
>      pcms->vmport is true
>      (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
> -M pc-2.1 -machine vmport=off,accel=xen
>      pcms->vmport is false
>      (but it doesn't matter, QEMU 2.1 didn't have the vmport option)
> -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
>
> I believe there's no consensus yet about the one marked with "**" above. It
> boils to the question: do we need to keep guest ABI stability when using
> "-M pc-2.1 -machine accel=xen"?
>

My answer is that for "**" vmport needs to be off.  It looks like my 
response:

 > Subject: Re: [BUGFIX][PATCH for 2.2 1/1] hw/i386/pc_piix.c: Also pass 
vmport=off for xenfv machine
 > Date: Thu, 20 Nov 2014 10:09:31 -0500
 > From: Don Slutz <dslutz@terremark.com>
 > To: Paolo Bonzini <pbonzini@redhat.com>, Don Slutz 
<dslutz@verizon.com>, qemu-devel <qemu-devel@nongnu.org>, Eduardo 
Habkost <ehabkost@redhat.com>
 >
 > On 11/20/14 01:02, Paolo Bonzini wrote:
 > >
 > > On 19/11/2014 20:08, Don Slutz wrote:
 > >> -M pc -machine accel=xen
 > >>      pcms->vmport is false
 > > I think this should be true.  Any reason why not?
 > >
 > > Paolo
 >
 > Yes, QEMU will crash if xen is enabled and the guest tries to access the
 > VMware port.
 >
 > (more on different thread).
 >
 >    -Don Slutz


Was not seen before this.
     -Don Slutz

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

[Eduardo Habkost]

On Thu, Nov 20, 2014 at 10:24:15AM -0500, Don Slutz wrote:
[...]
> >-M pc-2.1 -machine accel=xen
> >     pcms->vmport is true **
[...]
> >
> >I believe there's no consensus yet about the one marked with "**" above. It
> >boils to the question: do we need to keep guest ABI stability when using
> >"-M pc-2.1 -machine accel=xen"?
> >
> 
> My answer is that for "**" vmport needs to be off.  It looks like my
> response:
> 
[...]
> > Yes, QEMU will crash if xen is enabled and the guest tries to access the
> > VMware port.
[...]
> 
> Was not seen before this.

Yes, your answer arrived seconds after I sent my message. Ignore what I
said above.  :)

-- 
Eduardo