From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH] libxl: default to xenconsoled for pv guests,
 even if qemu is running
Date: Wed, 25 Apr 2012 11:49:30 +0100
Message-ID: <alpine.DEB.2.00.1204251147470.26786@kaball-desktop>
References: <c8486295429011e9a220.1335348912@cosworth.uk.xensource.com>
	<alpine.DEB.2.00.1204251128260.26786@kaball-desktop>
	<1335350442.28015.29.camel@zakaz.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1335350442.28015.29.camel@zakaz.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Wed, 25 Apr 2012, Ian Campbell wrote:
> On Wed, 2012-04-25 at 11:36 +0100, Stefano Stabellini wrote:
> > On Wed, 25 Apr 2012, Ian Campbell wrote:
> > > # HG changeset patch
> > > # User Ian Campbell <ian.campbell@citrix.com>
> > > # Date 1335348624 -3600
> > > # Node ID c8486295429011e9a220db1b6ed9f34ba690e729
> > > # Parent  6f740f2f6e3e080e4bba9df59c826947885f6bd7
> > > libxl: default to xenconsoled for pv guests, even if qemu is running.
> > > 
> > > Currently we prefer to use qemu for the disk backend if we are starting qemu
> > > anyway (e.g. to service a disk).
> > > 
> > > Unfortunately qemu doesn't log the console, which xenconsoled can do via
> > > XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it seems like
> > > there is no particular reason to prefer qemu just because it happens to be
> > > running.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > QEMU's console backend supports multiple PV consoles while xenconsoled
> > does not.
> > I think you should just change the default only in case a single PV
> > console is configured.
> > 
> > 
> > > I'm not sure if this is 4.2 material, perhaps too late to be making this sort
> > > of change?
> > > 
> > > diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_create.c
> > > --- a/tools/libxl/libxl_create.c	Wed Apr 25 11:05:05 2012 +0100
> > > +++ b/tools/libxl/libxl_create.c	Wed Apr 25 11:10:24 2012 +0100
> > > @@ -682,8 +682,7 @@ static int do_domain_create(libxl__gc *g
> > >                  d_config->num_vfbs, d_config->vfbs,
> > >                  d_config->num_disks, &d_config->disks[0]);
> > >  
> > > -        if (need_qemu)
> > > -             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> > > +        console.consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> > >  
> > >          libxl__device_console_add(gc, domid, &console, &state);
> > >          libxl__device_console_dispose(&console);
> > 
> > I would change the if into:
> > 
> > if (need_qemu and nr_console > 1)
> > 
> > even though I am aware that at the moment nr_console is always 1.
> 
> There isn't actually a nr_console here, just a hardcoded 1... I could
> add a nr_console just for this but perhaps it makes more sense for
> libxl__need_xenpv_qemu to enforce this by updating consback for all
> consoles iff there are > 1 of them and then returning need_qemu as
> appropriate?
> 
> I'm a little unhappy with having need_qemu have side-effects like this,
> but it seems better than splitting the logic into two places...

I would be OK with that, maybe it is worth adding a comment on
libxl__need_xenpv_qemu to document the behavior.