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 <address@hidden>
> 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 <address@hidden>
>
> 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.