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