From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrTGD-0005qZ-Ck for qemu-devel@nongnu.org; Thu, 20 Nov 2014 10:03:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrTG7-0002GK-9M for qemu-devel@nongnu.org; Thu, 20 Nov 2014 10:03:05 -0500 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:48781) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrTG7-0002G3-3O for qemu-devel@nongnu.org; Thu, 20 Nov 2014 10:02:59 -0500 From: Don Slutz Message-ID: <546E029F.5000106@terremark.com> Date: Thu, 20 Nov 2014 10:02:55 -0500 MIME-Version: 1.0 References: <1416443890-20263-1-git-send-email-dslutz@verizon.com> <20141120005824.GH3243@thinpad.lan.raisama.net> In-Reply-To: <20141120005824.GH3243@thinpad.lan.raisama.net> 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: Eduardo Habkost , Don Slutz Cc: Anthony Liguori , "Michael S. Tsirkin" , Michael Tokarev , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Stefan Hajnoczi , Paolo Bonzini 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