Re: Re: [PATCH 0/2] PV framebuffer

[Steven Smith <sos22-xen@srcf.ucam.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2938 bytes --]

> Next iteration.  If there's anything left in the way of getting this
> merged, please let me know right away.
> 
> Changes since last time:
> 
> * Leave xen console defaulting to /dev/tty1.
> 
> * Start backend automatically.
I'm not entirely happy with the way this is done; I'd much rather the
device controller started the backend than the image controller.  I
can probably fix this up more easily than you.

> * Fix kernel thread to call try_to_freeze().
> 
> * Recreate the image sub-object in XendDomainInfo.py on resume.
Why did you do this?  I can't see that it has anything to do with the
pvfb work, and everything seems to still work when I get rid of this
part of the patch.

> To activate the framebuffer, you have to put the following lines into
> /etc/xen/DOMNAME:
> 
>     vfb = 'yes'
>     vkbd = 'yes'
> 
> and either
> 
>     sdl = 1
> 
> or
> 
>     vnc = 1
I'm not entirely happy about this configuration syntax:

-- It's redundant: you can't specify a vfb without a vkbd or
   vice-versa, or the domain won't start, so we might as well get rid
   of the vkbd="yes" business
-- It doesn't give any way of specifying parameters to the backend
-- The configuration for a single device ends up split into several
   different items.  It also depends on stuff outside the usual
   (device) sections in the SXP, which is kind of nasty.
-- It potentially conflicts with the way emulated vga is specified in
   hvm guests.  e.g. you can't specify that emulated vga should be
   exported over vnc while the pv framebuffer should be sdl.  Whether
   you'd ever want to do this is another matter, but it'd be nice if
   we didn't rule it out for reasons as trivial as a bad configuration
   file design.
-- It won't extend to supporting multiple pvfbs
-- It doesn't look much like the syntax for configuring other devices.

I'd prefer something more like this:

vfbs = [ 'type=sdl,listen=127.0.0.1' ]

Again, I can probably fix this up more easily than you, if you don't
object.

> One last thing to consider: I'm not entirely happy with source file
> names and locations.  It's now or never for renames.  Peeves:
> 
> * Frontend source usually lives in drivers/xen/<WHAT>front.  xenfb and
>   xenkbd are exceptions.  Rename to fbfront and kbdfront?  Same for
>   the .c files.  This would also fix the minor annoyance of having the
>   same file name in frontend and backend.
> 
> * Given how closely related xenfb and xenkbd are, I'm not fond of
>   having them in separate directories.  Move kbd into the fb
>   directory?
If it were my patch, I'd put everything in drivers/xen/fbfront, but it
doesn't really make a great deal of difference.

Apart from that, I'm happy with this.  If you send me a patch which
applies cleanly to xen-unstable and is signed-off-by the right people,
I'll check it in and fix the things I mentioned myself.

Steven.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel