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

[Don Slutz <dslutz@verizon.com>]

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