[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fi
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen |
Date: |
Thu, 20 Nov 2014 10:44:14 +0200 |
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 <address@hidden>
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.
> + 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.
> /* 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.
> + '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.
> address@hidden vmport=on|off
> -Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default)
> address@hidden 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
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen, Don Slutz, 2014/11/20
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen, Eric Blake, 2014/11/19
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen,
Michael S. Tsirkin <=