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