From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42699) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrViO-0008QH-VP for qemu-devel@nongnu.org; Thu, 20 Nov 2014 12:40:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrViI-00062C-Rq for qemu-devel@nongnu.org; Thu, 20 Nov 2014 12:40:20 -0500 Received: from fldsmtpe04.verizon.com ([140.108.26.143]:42681) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrViI-00061y-NL for qemu-devel@nongnu.org; Thu, 20 Nov 2014 12:40:14 -0500 From: Don Slutz Message-ID: <546E277B.1020109@terremark.com> Date: Thu, 20 Nov 2014 12:40:11 -0500 MIME-Version: 1.0 References: <1416443890-20263-1-git-send-email-dslutz@verizon.com> <20141120084414.GA3142@redhat.com> In-Reply-To: <20141120084414.GA3142@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Don Slutz Cc: Eduardo Habkost , Michael Tokarev , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Stefan Hajnoczi , Paolo Bonzini , lcapitulino@redhat.com, mdroth@linux.vnet.ibm.com, Anthony Liguori 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 > 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