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/20/14 03:44, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
>> c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4
>>
>> or
>>
>> c/s b154537ad07598377ebf98252fb7d2aff127983b
>>
>> moved the testing of xen_enabled() from pc_init1() to
>> pc_machine_initfn().
>>
>> xen_enabled() does not return the correct value in
>> pc_machine_initfn().
>>
>> Changed vmport from a bool to an enum.  Added the value "auto" to do
>> the old way.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> This looks fine to me. A couple of minor comments below.
> Also this changes qapi schema file, let's get ack from maintainers -
> my understanding is that just adding a definition there won't
> affect any users, correct?
>
>
>> ---
>>   hw/i386/pc.c         | 23 ++++++++++++++---------
>>   hw/i386/pc_piix.c    | 27 ++++++++++++++++++++++++++-
>>   hw/i386/pc_q35.c     | 27 ++++++++++++++++++++++++++-
>>   include/hw/i386/pc.h |  2 +-
>>   qapi-schema.json     | 16 ++++++++++++++++
>>   qemu-options.hx      |  8 +++++---
>>   vl.c                 |  2 +-
>>   7 files changed, 89 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 1205db8..2f68e4d 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
>>       pcms->max_ram_below_4g = value;
>>   }
>>   
>> -static bool pc_machine_get_vmport(Object *obj, Error **errp)
>> +static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
>> +                                  const char *name, Error **errp)
>>   {
>>       PCMachineState *pcms = PC_MACHINE(obj);
>> +    int vmport = pcms->vmport;
>>   
>> -    return pcms->vmport;
>> +    visit_type_enum(v, &vmport, vmport_lookup, NULL, name, errp);
>>   }
>>   
>> -static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
>> +static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
>> +                                  const char *name, Error **errp)
>>   {
>>       PCMachineState *pcms = PC_MACHINE(obj);
>> +    int vmport;
>>   
>> -    pcms->vmport = value;
>> +    visit_type_enum(v, &vmport, vmport_lookup, NULL, name, errp);
>> +    pcms->vmport = vmport;
>>   }
>>   
>>   static void pc_machine_initfn(Object *obj)
>> @@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
>>                           pc_machine_get_max_ram_below_4g,
>>                           pc_machine_set_max_ram_below_4g,
>>                           NULL, NULL, NULL);
>> -    pcms->vmport = !xen_enabled();
>> -    object_property_add_bool(obj, PC_MACHINE_VMPORT,
>> -                             pc_machine_get_vmport,
>> -                             pc_machine_set_vmport,
>> -                             NULL);
>> +    pcms->vmport = VMPORT_AUTO;
>> +    object_property_add(obj, PC_MACHINE_VMPORT, "str",
>> +                        pc_machine_get_vmport,
>> +                        pc_machine_set_vmport,
>> +                        NULL, NULL, NULL);
>>   }
>>   
>>   static void pc_machine_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 7bb97a4..81a7b83 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -101,6 +101,7 @@ static void pc_init1(MachineState *machine,
>>       FWCfgState *fw_cfg = NULL;
>>       PcGuestInfo *guest_info;
>>       ram_addr_t lowmem;
>> +    bool no_vmport;
>>   
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
>>        * If it doesn't, we need to split it in chunks below and above 4G.
>> @@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
>>   
>>       pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>>   
> Probably should be assert(pc_machine->vmport != VMPORT_MAX) -
> we never get any values except on/off/auto.

Ok, added the assert.

>> +    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;
>> +        }
>> +    }
>> +
> Can't above be moved to a function in pc.c, and be reused between piix
> and q35? It's big enough to avoid open-coding, IMHO.
>

I feel that the what Eduardo Habkost provided:

+    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
+    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
+        no_vmport = xen_enabled();
+    } else {
+        no_vmport = (pc_machine->vmport == ON_OFF_AUTO_ON);
+    }

is short enough to not need it's own function.  But I can do this if 
still needed.

>>       /* 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;
>> +        }
>> +    }
>> +
>>       /* init basic PC hardware */
>>       pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
>> -                         !pc_machine->vmport, 0xff0104);
>> +                         no_vmport, 0xff0104);
>>   
>>       /* connect pm stuff to lpc */
>>       ich9_lpc_pm_init(lpc);
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 7c3731f..d7d8f30 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -37,7 +37,7 @@ struct PCMachineState {
>>       ISADevice *rtc;
>>   
>>       uint64_t max_ram_below_4g;
>> -    bool vmport;
>> +    vmport vmport;
>>   };
>>   
>>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>> 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',
>
> vmport as type name violates our coding style.
> Should be VMPort.
>
> But in fact, this is a generic OnOffAuto type, isn't it?
> Maybe it should be named like this, and go into qapi/common.json?
> I think it might be a good idea but it's not a must.
>

This looks like the name I will go with.

     -Don Slutz

>> +  'data': [ 'auto', 'on', 'off' ] }
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index da9851d..64af16d 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>       "                property accel=accel1[:accel2[:...]] selects accelerator\n"
>>       "                supported accelerators are kvm, xen, tcg (default: tcg)\n"
>>       "                kernel_irqchip=on|off controls accelerated irqchip support\n"
>> -    "                vmport=on|off controls emulation of vmport (default: on)\n"
>> +    "                vmport=on|off|auto controls emulation of vmport (default: auto)\n"
>>       "                kvm_shadow_mem=size of KVM shadow MMU\n"
>>       "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>>       "                mem-merge=on|off controls memory merge support (default: on)\n"
>> @@ -52,8 +52,10 @@ than one accelerator specified, the next one is used if the previous one fails
>>   to initialize.
>>   @item kernel_irqchip=on|off
>>   Enables in-kernel irqchip support for the chosen accelerator when available.
>> -@item vmport=on|off
>> -Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default)
>> +@item vmport=on|off|auto
>> +Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
>> +value based on accel. For accel=xen the default is off otherwise the default
>> +is on.
>>   @item kvm_shadow_mem=size
>>   Defines the size of the KVM shadow MMU.
>>   @item dump-guest-core=on|off
>> diff --git a/vl.c b/vl.c
>> index f4a6e5e..eb89d62 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -381,7 +381,7 @@ static QemuOptsList qemu_machine_opts = {
>>               .help = "maximum ram below the 4G boundary (32bit boundary)",
>>           }, {
>>               .name = PC_MACHINE_VMPORT,
>> -            .type = QEMU_OPT_BOOL,
>> +            .type = QEMU_OPT_STRING,
>>               .help = "Enable vmport (pc & q35)",
>>           },{
>>               .name = "iommu",
>> -- 
>> 1.8.4