From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55396) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrAiy-0005Ui-CE for qemu-devel@nongnu.org; Wed, 19 Nov 2014 14:15:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrAis-0005oR-7M for qemu-devel@nongnu.org; Wed, 19 Nov 2014 14:15:32 -0500 Received: from fldsmtpe04.verizon.com ([140.108.26.143]:18240) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrAis-0005oI-2G for qemu-devel@nongnu.org; Wed, 19 Nov 2014 14:15:26 -0500 From: Don Slutz Message-ID: <546CEC48.4060306@terremark.com> Date: Wed, 19 Nov 2014 14:15:20 -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> In-Reply-To: <546CEA98.40505@terremark.com> 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: Don Slutz , Paolo Bonzini , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , "Michael S. Tsirkin" , Eduardo Habkost Cc: Michael Tokarev , Anthony Liguori , Stefan Hajnoczi P.S. I think the title: xen-common.c: If "-machine vmport=" is not specified turn vmport off is better. Will send v2 of patch out soon. -Don Slutz On 11/19/14 14:08, 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: > > commit 436d056420bb209d066d2378b628d297777d9f20 > Author: Don Slutz > Date: Wed Nov 19 10:01:16 2014 -0500 > > hw/i386/pc_piix.c: Also pass vmport=off for xenfv machine > > 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(). > > Add vmport_changed to track the state of vmport so that > accel=xen can do the right thing. > > Drop the call to xen_enabled() in pc_machine_initfn() to reduce > potential confusion. > > Signed-off-by: Don Slutz > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 1205db8..b400ac8 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1723,6 +1723,7 @@ static void pc_machine_set_vmport(Object *obj, > bool value, Error **errp) > PCMachineState *pcms = PC_MACHINE(obj); > > pcms->vmport = value; > + pcms->vmport_changed = true; > } > > static void pc_machine_initfn(Object *obj) > @@ -1737,7 +1738,7 @@ 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(); > + pcms->vmport = true; > object_property_add_bool(obj, PC_MACHINE_VMPORT, > pc_machine_get_vmport, > pc_machine_set_vmport, > 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; > }; > > #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device" > diff --git a/xen-common.c b/xen-common.c > index 56359ca..bb11bb7 100644 > --- a/xen-common.c > +++ b/xen-common.c > @@ -12,6 +12,7 @@ > #include "qmp-commands.h" > #include "sysemu/char.h" > #include "sysemu/accel.h" > +#include "hw/i386/pc.h" > > //#define DEBUG_XEN > > @@ -112,6 +113,11 @@ static void xen_change_state_handler(void > *opaque, int running, > > static int xen_init(MachineState *ms) > { > + PCMachineState *pcms = PC_MACHINE(ms); > + > + if (!pcms->vmport_changed) { > + pcms->vmport = false; > + } > xen_xc = xen_xc_interface_open(0, 0, 0); > if (xen_xc == XC_HANDLER_INITIAL_VALUE) { > xen_be_printf(NULL, 0, "can't open xen interface\n"); > > > -Don Slutz > > > >>> However you bring up a good point and I will see if I can quickly >>> handled it. >