Re: [Xen-devel] [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-16 at 12:42 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] xl: create VFB for PV guest when VNC is 
> specified"):
> > On Mon, 2013-12-16 at 12:17 +0000, Ian Jackson wrote:
> > > This should be xrealloc, and wrapped to 80 columns.  But see below.
> > 
> > This also suggests that we might be trying to support both the
> > "toplevel" vnc options and the "vfb = []" style at the same time.
> > 
> > IMHO the vfb option should take precedence -- i.e. we should ignore the
> > toplevel options if it is present.
> 
> Oh, yes.  Good point.
> 
> > The alternative would be some sort of union of the toplevel options and
> > the first vfb given, but that sounds a bit mad...
> > 
> > I suppose there is also the question of what xend did here.
> 
> Perhaps Konrad can let us know...
> 
> > > There's a lot of this kind of boilerplate.  I'm tempted to suggest
> > > making a macro to do this.  Searching for "devid =" found 4 call sites
> > > if that line is included in the macro; searching for "realloc" found
> > > me 6 call sites if it isn't.  And that's before your two additional
> > > ones.  Ian C, what do you think ?
> > 
> > Some helpers for dealing with allocating and resizing/appending to libxl
> > Array types might be a useful addition to the library itself, i.e. idl
> > generated? Otherwise an xl macro might indeed be handy.
> > 
> > I'm not sure what the 4 vs. 6 "if that line is/isn't included in" is
> > referring too though.
> 
> There are four instances of this kind of boilerplate which contain
>             SPONGE->devid = d_config->num_SPONGEs;
> and six that lack it.

Oh right, yes.

> > > This duplicates the HVM VNC option parsing.  It should be factored out
> > > into a subroutine.
> > 
> > Ack.
> > 
> > That makes me wonder if the top level options shouldn't be populating
> > &b_info->u.pv.vnc.* (and if that shouldn't have been a non-keyed field)
> > while vfb = fills in d_config->vfbs. That is a bigger change on the
> > libxl side though and maybe doesn't make quite as much sense as with
> > the .
> 
> As with the "." ?

Erm... I think I was about to say something which was nonsense and only
partially aborted...

What I *should* have said is that HVM has an emulated VGA card which is
what &b_info->u.hvm.vnc is configuring and which is separate to any PVFB
given to the guest. So it doesn't make sense to have b_info->u.pv.vnc
because there is no VGA.

(Incidentally, I expect even xend only meant for the toplevel vnc
options to configure the VGA and the leakage into PV guest's pvfb[0] is
just an unfortunate art we need to deal with)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel