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