Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Don Slutz <dslutz@verizon.com>]

On 11/19/14 19:58, Eduardo Habkost wrote:
> On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
> [...]
>> @@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
>>   
>>       pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>>   
>> +    if (xen_enabled()) {
>> +        switch (pc_machine->vmport) {
>> +        case VMPORT_MAX:
>> +        case VMPORT_AUTO:
>> +        case VMPORT_OFF:
>> +            no_vmport = true;
>> +            break;
>> +        case VMPORT_ON:
>> +            no_vmport = false;
>> +            break;
>> +        }
>> +    } else {
>> +        switch (pc_machine->vmport) {
>> +        case VMPORT_MAX:
>> +        case VMPORT_OFF:
>> +            no_vmport = true;
>> +            break;
>> +        case VMPORT_AUTO:
>> +        case VMPORT_ON:
>> +            no_vmport = false;
>> +            break;
>> +        }
>> +    }
>> +
>>       /* init basic PC hardware */
>>       pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
>> -                         !pc_machine->vmport, 0x4);
>> +                         no_vmport, 0x4);
>>   
>>       pc_nic_init(isa_bus, pci_bus);
>>   
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 598e679..4f932d6 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
>>       PcGuestInfo *guest_info;
>>       ram_addr_t lowmem;
>>       DriveInfo *hd[MAX_SATA_PORTS];
>> +    bool no_vmport;
>>   
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>>        * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>> @@ -242,9 +243,33 @@ static void pc_q35_init(MachineState *machine)
>>   
>>       pc_register_ferr_irq(gsi[13]);
>>   
>> +    if (xen_enabled()) {
>> +        switch (pc_machine->vmport) {
>> +        case VMPORT_MAX:
>> +        case VMPORT_AUTO:
>> +        case VMPORT_OFF:
>> +            no_vmport = true;
>> +            break;
>> +        case VMPORT_ON:
>> +            no_vmport = false;
>> +            break;
>> +        }
>> +    } else {
>> +        switch (pc_machine->vmport) {
>> +        case VMPORT_MAX:
>> +        case VMPORT_OFF:
>> +            no_vmport = true;
>> +            break;
>> +        case VMPORT_AUTO:
>> +        case VMPORT_ON:
>> +            no_vmport = false;
>> +            break;
>> +        }
>> +    }
> What about simplifying those 24 lines to the following 5 lines:
>
>      if (pc_machine->vmport == VMPORT_AUTO) {
>            no_vmport = xen_enabled();
>      } else {
>            no_vmport = (pc_machine->vmport == VMPORT_ON);
>      }
>

Looks much better.  Will switch to this.

>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d0926d9..f7ee3ad 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3513,3 +3513,19 @@
>>   # Since: 2.1
>>   ##
>>   { 'command': 'rtc-reset-reinjection' }
>> +
>> +##
>> +# @vmport
>> +#
>> +# An enumeration of the options on enabling of VMWare ioport emulation
>> +#
>> +# @auto: system selects the old default
>> +#
>> +# @on: provide the vmport device
>> +#
>> +# @off: do not provide the vmport device
>> +#
>> +# Since: 2.2
>> +##
>> +{ 'enum': 'vmport',
> Coding style for enum typedefs is CamelCase.
>
> Probably something more descriptive than just "VMPort" would be better.
> This enum does not describe the vmport itself, but just the on/off/auto
> setting. Maybe VMPortConfig?

That would be fine with me.

>> +  'data': [ 'auto', 'on', 'off' ] }
> I believe there may be other properties with exactly the same behavior.
> Maybe we could use a generic enum name, like "Tristate"?
>
> But we have a problem, here: the existing code accepts "on", "yes" and
> "true" for boolean values. Now it is going to accept only "on' and
> "off". This breaks compatibility.

This is new at 2.2, so I think we can adjust either way (just the 3 or add
others).

> Maybe my enum suggestion was not a very good one? I would like to hear
> opinions from others.
>
> In either case, I am sure your previous bugfix is more appropriate for
> 2.2. Changing the data type of the vmport property is much more
> intrusive than just allowing it to return an incorrect value temporarily
> between instance_init and machine->init().
>

I can post the v2 if that makes sense (as v4?)  will wait a little 
while.  If the v2
is the one in 2.2 then allowing the other options may make sense.

     -Don Slutz