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 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.

    -Don Slutz