From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36276) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrFwf-00013b-DW for qemu-devel@nongnu.org; Wed, 19 Nov 2014 19:50:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrFwZ-0002dj-AC for qemu-devel@nongnu.org; Wed, 19 Nov 2014 19:50:01 -0500 Received: from fldsmtpe04.verizon.com ([140.108.26.143]:27616) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrFwZ-0002df-1W for qemu-devel@nongnu.org; Wed, 19 Nov 2014 19:49:55 -0500 From: Don Slutz Message-ID: <546D3AB0.6070401@terremark.com> Date: Wed, 19 Nov 2014 19:49:52 -0500 MIME-Version: 1.0 References: <1416418257-10166-1-git-send-email-dslutz@verizon.com> <546CD4F1.4040602@redhat.com> <546CDC46.8050603@terremark.com> <546CDCAD.9030800@redhat.com> <546CEA98.40505@terremark.com> <20141119193009.GF3243@thinpad.lan.raisama.net> <546CF714.4030505@terremark.com> <20141120002452.GG3243@thinpad.lan.raisama.net> In-Reply-To: <20141120002452.GG3243@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 1/1] hw/i386/pc_piix.c: Also pass vmport=off for xenfv machine 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:24, Eduardo Habkost wrote: > On Wed, Nov 19, 2014 at 03:01:24PM -0500, Don Slutz wrote: >> On 11/19/14 14:30, Eduardo Habkost wrote: >>> On Wed, Nov 19, 2014 at 02:08:08PM -0500, Don Slutz wrote: >>>> On 11/19/14 13:08, Paolo Bonzini wrote: >>>>> On 19/11/2014 19:07, Don Slutz wrote: >>>>>>> "-M pc -machine accel=xen" should work and, if that's what you want, >>>>>>> disable the vmport device. I think this patch is wrong. >>>>>>> >>>>>>> Paolo >>>>>> Well, I also want "-M pc -machine accel=xen,vmport=on" to work. >>>>> Right. So let's start by deciding what the desired semantics are for >>>>> all six cases: -M pc/xenfv, -machine vmport=on/off/absent. >>>>> >>>>> Paolo >>>> I get 12 cases (PCMachineState *pcms = PC_MACHINE(obj)): >>>> >>>> >>>> >>>> -M pc >>>> pcms->vmport is true >>>> -M pc -machine vmport=on >>>> pcms->vmport is true >>>> -M pc -machine vmport=off >>>> pcms->vmport is false >>>> -M xenfv >>>> pcms->vmport is false >>>> -M xenfv -machine vmport=on >>>> pcms->vmport is true >>>> -M xenfv -machine vmport=off >>>> pcms->vmport is false >>>> >>>> -M pc -machine accel=xen >>>> pcms->vmport is false >>>> -M pc -machine vmport=on,accel=xen >>>> pcms->vmport is true >>>> -M pc -machine vmport=off,accel=xen >>>> pcms->vmport is false >>>> -M xenfv -machine accel=xen >>>> pcms->vmport is false >>>> -M xenfv -machine vmport=on,accel=xen >>>> pcms->vmport is true >>>> -M xenfv -machine vmport=off,accel=xen >>>> pcms->vmport is false >>>> >>>> >>>> Which look like to me to solve down to xen_init() called via >>>> xen_accel_class_init() >>>> need to see if vmport has been specified and change it if needed. >>>> >>>> Which leads me to: >>>> >>> [...] >>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >>>> index 7c3731f..5782406 100644 >>>> --- a/include/hw/i386/pc.h >>>> +++ b/include/hw/i386/pc.h >>>> @@ -38,6 +38,7 @@ struct PCMachineState { >>>> >>>> uint64_t max_ram_below_4g; >>>> bool vmport; >>>> + bool vmport_changed; >>> So, now the setting have three possible states: unset, on, and off. >>> >>> If we can't avoid that (which seems to be the case, as "accel" is >>> changed after we already set the default value for "vmport" in instance >>> init), maybe we should make it an enum property that accepts on/off/auto >>> as values, instead of faking a boolean property that is not really a >>> boolean? >>> >> I would be happy to look into this. I have not done anything that used >> an enum property, and so it is hard to guess on how long it will take to >> convert the change to this. > I think your patch may be a good intermediate solution, as long as you > add a comment noting that the return value of the "vmport" property may > be invalid before machine->init() is called, depending on the > machine-type. > > Or, maybe a property without a getter would be appropriate? > Timing is everything. The convert to enum was not too hard. v3 posted. -Don Slutz