Xen devel - Sep 2006 - [PATCH] Paravirt framebuffer support in xend [3/5]

Add support in xend and xm to know about the vnc and sdl options for PV
domains.  Launch xen-sdlfb or xen-vncfb if we''re setting up a graphics
console.  Also, if we''re going to use a graphical console in the guest,
set /<dompath>/console/use_graphics to 1.

(Note: this patch requires a tiny bit of obvious change to work with my
patch for vnclisten that I posted earlier today)

Signed-off-by: Jeremy Katz <katzj@redhat.com>


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

> Add support in xend and xm to know about the vnc and sdl options for PV
> domains.  Launch xen-sdlfb or xen-vncfb if we''re setting up a
graphics
> console.  Also, if we''re going to use a graphical console in the
guest,
> set /<dompath>/console/use_graphics to 1.
> 
> (Note: this patch requires a tiny bit of obvious change to work with my
> patch for vnclisten that I posted earlier today)
> 
> Signed-off-by: Jeremy Katz <katzj@redhat.com>
> diff -r a2a8f1ed16ea -r 2b360c6b44fa tools/python/xen/xend/image.py
> --- a/tools/python/xen/xend/image.py	Sat Sep 02 15:22:19 2006 -0400
> +++ b/tools/python/xen/xend/image.py	Sat Sep 02 15:23:32 2006 -0400
> @@ -20,8 +20,10 @@ import os, string
>  import os, string
>  import re
>  import math
> +import signal
Why?
>  
>  import xen.lowlevel.xc
> +import xen.util.auxbin
>  from xen.xend import sxp
>  from xen.xend.XendError import VmError
>  from xen.xend.XendLogging import log
> @@ -189,6 +191,68 @@ class LinuxImageHandler(ImageHandler):
>                                cmdline        = self.cmdline,
>                                ramdisk        = self.ramdisk,
>                                features       = self.vm.getFeatures())
> +
> +    def configure(self, imageConfig, deviceConfig):
Does this really belong in class LinuxImageHandler?
> +        ImageHandler.configure(self, imageConfig, deviceConfig)
> +
> +        self.pid = 0
> +        log.info("configuring linux guest")
> +
> +        # set up the graphics bits.
> +        # FIXME: this is much like what we do for HVM, should it be 
> +        # for all image types now?
> +        self.display = sxp.child_value(imageConfig,
''display'')
> +        self.xauthority = sxp.child_value(imageConfig,
''xauthority'')
> +        self.vncconsole = sxp.child_value(imageConfig,
''vncconsole'')
> +        self.vnc = sxp.child_value(imageConfig, ''vnc'')
> +        self.sdl = sxp.child_value(imageConfig, ''sdl'')
> +        if self.vnc:
> +            self.vncdisplay = sxp.child_value(imageConfig,
''vncdisplay'',
> +                                              int(self.vm.getDomid()))
> +            self.vncunused = sxp.child_value(imageConfig,
''vncunused'')
> +            self.vnclisten = sxp.child_value(imageConfig,
''vnclisten'')
> +        if self.vnc or self.sdl:
> +            log.info("setting use_graphics")
> +            self.vm.writeDom("console/use_graphics",
"1")
> +        else:
> +            self.vm.writeDom("console/use_graphics",
"0")
> +
> +    def createDeviceModel(self):
Maybe call ImageHandler.createDeviceModel?
> +        if self.pid:
> +            return
> +        # Execute device model (for us, it''s just the fb
frontend)
> +        if not self.vnc and not self.sdl:
> +            return
> +
> +        if self.vnc:
> +            args = [xen.util.auxbin.pathTo("xen-vncfb")]
> +            if self.vncunused:
> +                args += [''--unused'']
> +            elif self.vncdisplay:
> +                args += [ "--vncport", "%d" %(5900 +
self.vncdisplay,) ]
> +            if self.vnclisten:
> +                args += [ "--listen", self.vnclisten ]
> +            if self.vncconsole:
> +                args += [ "--vncviewer" ]
> +        elif self.sdl:
> +            args = [xen.util.auxbin.pathTo("xen-sdlfb")]
> +        args = args + [ "--domid", "%d" %
self.vm.getDomid(),
> +                        "--title",
self.vm.info[''name''] ]
> +        env = dict(os.environ)
> +        if self.display:
> +            env[''DISPLAY''] = self.display
> +        if self.xauthority:
> +            env[''XAUTHORITY''] = self.xauthority
> +        log.info("spawning video: %s", args)
> +        self.pid = os.spawnve(os.P_NOWAIT, args[0], args, env)
> +        log.info("device model pid: %d", self.pid)
> +
> +    def destroy(self):
> +        if not self.pid:
> +            return
> +        os.kill(self.pid, signal.SIGKILL)
> +        os.waitpid(self.pid, 0)
> +        self.pid = 0
>  
>  class PPC_LinuxImageHandler(LinuxImageHandler):
>  
> @@ -371,7 +435,6 @@ class HVMImageHandler(ImageHandler):
>  
>      def destroy(self):
>          self.unregister_shutdown_watch();
> -        import signal
Why?
>          if not self.pid:
>              return
>          os.kill(self.pid, signal.SIGKILL)
> diff -r a2a8f1ed16ea -r 2b360c6b44fa tools/python/xen/xm/create.py
> --- a/tools/python/xen/xm/create.py	Sat Sep 02 15:22:19 2006 -0400
> +++ b/tools/python/xen/xm/create.py	Sat Sep 02 15:23:32 2006 -0400
> @@ -483,6 +483,8 @@ def configure_image(vals):
>  
>      if vals.builder == ''hvm'':
>          configure_hvm(config_image, vals)
> +
> +    configure_graphics(config_image, vals)        
>          
>      return config_image
>      
> @@ -630,14 +632,21 @@ def configure_vifs(config_devs, vals):
>          map(f, d.keys())
>          config_devs.append([''device'', config_vif])
>  
> +def configure_graphics(config_image, vals):
> +    """Create the config for graphic consoles.
> +    """
> +    args = [ ''vnc'', ''vncdisplay'',
''vncconsole'', ''vncunused'',
> +             ''sdl'', ''display'',
''xauthority'' ]
> +    for a in args:
> +        if (vals.__dict__[a]):
> +            config_image.append([a, vals.__dict__[a]])
This looks very wrong.  What is it trying to do?  Why do these parameters
need to be handled differently from the ones in configure_image?
>  
>  def configure_hvm(config_image, vals):
>      """Create the config for HVM devices.
>      """
>      args = [ ''device_model'', ''pae'',
''vcpus'', ''boot'', ''fda'',
''fdb'',
>               ''localtime'', ''serial'',
''stdvga'', ''isa'',
''nographic'', ''soundhw'',
> -             ''vnc'', ''vncdisplay'',
''vncunused'', ''vncconsole'',
''sdl'', ''display'',
> -             ''acpi'', ''apic'',
''xauthority'', ''usb'',
''usbdevice'' ]
> +             ''acpi'', ''apic'',
''usb'', ''usbdevice'' ]
>      for a in args:
>          if (vals.__dict__[a]):
>              config_image.append([a, vals.__dict__[a]])
Steven.


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

On Mon, 2006-09-04 at 10:02 +0100, Steven Smith wrote:
> > diff -r a2a8f1ed16ea -r 2b360c6b44fa tools/python/xen/xend/image.py
> > --- a/tools/python/xen/xend/image.py	Sat Sep 02 15:22:19 2006 -0400
> > +++ b/tools/python/xen/xend/image.py	Sat Sep 02 15:23:32 2006 -0400
> > @@ -20,8 +20,10 @@ import os, string
> >  import os, string
> >  import re
> >  import math
> > +import signal
> Why?
Because it''s used to kill a process and doing a lazy import of things
like this is a good way to drive a man crazy ;-)
> >  
> >  import xen.lowlevel.xc
> > +import xen.util.auxbin
> >  from xen.xend import sxp
> >  from xen.xend.XendError import VmError
> >  from xen.xend.XendLogging import log
> > @@ -189,6 +191,68 @@ class LinuxImageHandler(ImageHandler):
> >                                cmdline        = self.cmdline,
> >                                ramdisk        = self.ramdisk,
> >                                features       = self.vm.getFeatures())
> > +
> > +    def configure(self, imageConfig, deviceConfig):
> Does this really belong in class LinuxImageHandler?
Right now, it''s only implemented for Linux -- with a proof of concept
for elsewhere, I could see move it to being generic instead.  But right
now, it''s Linux specific
> > +    def createDeviceModel(self):
> Maybe call ImageHandler.createDeviceModel?
The HVM one doesn''t -- perhaps both should although currently the
comment in the superclass is such that it''s not going to define
anything
> > @@ -371,7 +435,6 @@ class HVMImageHandler(ImageHandler):
> >  
> >      def destroy(self):
> >          self.unregister_shutdown_watch();
> > -        import signal
> Why?
Because we import it once at the top instead of scattering imports all
over in methods
> > +def configure_graphics(config_image, vals):
> > +    """Create the config for graphic consoles.
> > +    """
> > +    args = [ ''vnc'', ''vncdisplay'',
''vncconsole'', ''vncunused'',
> > +             ''sdl'', ''display'',
''xauthority'' ]
> > +    for a in args:
> > +        if (vals.__dict__[a]):
> > +            config_image.append([a, vals.__dict__[a]])
> This looks very wrong.  What is it trying to do?  Why do these parameters
> need to be handled differently from the ones in configure_image?
It''s making it so that we have one place to modify the list of graphics
related arguments instead of keeping one copy in configure_image and one
copy in configure_hvm.  Now, they can both call configure_graphics and
it''s easier to keep things in sync

Jeremy


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

> On Mon, 2006-09-04 at 10:02 +0100, Steven Smith wrote:
> > > diff -r a2a8f1ed16ea -r 2b360c6b44fa
tools/python/xen/xend/image.py
> > > --- a/tools/python/xen/xend/image.py	Sat Sep 02 15:22:19 2006
-0400
> > > +++ b/tools/python/xen/xend/image.py	Sat Sep 02 15:23:32 2006
-0400
> > > @@ -20,8 +20,10 @@ import os, string
> > >  import os, string
> > >  import re
> > >  import math
> > > +import signal
> > Why?
> Because it''s used to kill a process and doing a lazy import of
things
> like this is a good way to drive a man crazy ;-)
I''d drop this from this patch, since it''s not really required
or
particularly useful.

Don''t let that stop you from doing a separate cleanup patch,
though. :)
> 
> > >  
> > >  import xen.lowlevel.xc
> > > +import xen.util.auxbin
> > >  from xen.xend import sxp
> > >  from xen.xend.XendError import VmError
> > >  from xen.xend.XendLogging import log
> > > @@ -189,6 +191,68 @@ class LinuxImageHandler(ImageHandler):
> > >                                cmdline        = self.cmdline,
> > >                                ramdisk        = self.ramdisk,
> > >                                features       =
self.vm.getFeatures())
> > > +
> > > +    def configure(self, imageConfig, deviceConfig):
> > Does this really belong in class LinuxImageHandler?
> Right now, it''s only implemented for Linux -- with a proof of
concept
> for elsewhere, I could see move it to being generic instead.  But right
> now, it''s Linux specific
The other PV devices have their own Controller classes
(BlkifController, NetifController, etc.).  Why is the framebuffer
special?
> > > +    def createDeviceModel(self):
> > Maybe call ImageHandler.createDeviceModel?
> The HVM one doesn''t -- perhaps both should although currently the
> comment in the superclass is such that it''s not going to define
anything
I think that''s a bug in the HVM version, personally.  I''ll
have a look
at it later.
> > > @@ -371,7 +435,6 @@ class HVMImageHandler(ImageHandler):
> > >  
> > >      def destroy(self):
> > >          self.unregister_shutdown_watch();
> > > -        import signal
> > Why?
> Because we import it once at the top instead of scattering imports all
> over in methods
Again, this really belongs in a separate patch.
> > > +def configure_graphics(config_image, vals):
> > > +    """Create the config for graphic consoles.
> > > +    """
> > > +    args = [ ''vnc'',
''vncdisplay'', ''vncconsole'',
''vncunused'',
> > > +             ''sdl'',
''display'', ''xauthority'' ]
> > > +    for a in args:
> > > +        if (vals.__dict__[a]):
> > > +            config_image.append([a, vals.__dict__[a]])
> > This looks very wrong.  What is it trying to do?  Why do these
parameters
> > need to be handled differently from the ones in configure_image?
> It''s making it so that we have one place to modify the list of
graphics
> related arguments instead of keeping one copy in configure_image and one
> copy in configure_hvm.  Now, they can both call configure_graphics and
> it''s easier to keep things in sync
Your argument would have more force if they actually did both call
configure_graphics.

Steven.


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

> > > > +    def createDeviceModel(self):
> > > Maybe call ImageHandler.createDeviceModel?
> > The HVM one doesn''t -- perhaps both should although currently
the
> > comment in the superclass is such that it''s not going to
define anything
> I think that''s a bug in the HVM version, personally. 
I''ll have a look
> at it later.
Scratch that, you were right the first time.  Sorry for the noise.

Steven.


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

On Wed, 2006-09-06 at 10:17 +0100, Steven Smith wrote:
> > On Mon, 2006-09-04 at 10:02 +0100, Steven Smith wrote:
> > > > diff -r a2a8f1ed16ea -r 2b360c6b44fa
tools/python/xen/xend/image.py
> > > > --- a/tools/python/xen/xend/image.py	Sat Sep 02 15:22:19
2006 -0400
> > > > +++ b/tools/python/xen/xend/image.py	Sat Sep 02 15:23:32
2006 -0400
> > > > @@ -20,8 +20,10 @@ import os, string
> > > >  import os, string
> > > >  import re
> > > >  import math
> > > > +import signal
> > > Why?
> > Because it''s used to kill a process and doing a lazy import
of things
> > like this is a good way to drive a man crazy ;-)
> I''d drop this from this patch, since it''s not really
required or
> particularly useful.
> 
> Don''t let that stop you from doing a separate cleanup patch,
> though. :)
Hint taken and sent ;-)
> > > >  import xen.lowlevel.xc
> > > > +import xen.util.auxbin
> > > >  from xen.xend import sxp
> > > >  from xen.xend.XendError import VmError
> > > >  from xen.xend.XendLogging import log
> > > > @@ -189,6 +191,68 @@ class LinuxImageHandler(ImageHandler):
> > > >                                cmdline        =
self.cmdline,
> > > >                                ramdisk        =
self.ramdisk,
> > > >                                features       =
self.vm.getFeatures())
> > > > +
> > > > +    def configure(self, imageConfig, deviceConfig):
> > > Does this really belong in class LinuxImageHandler?
> > Right now, it''s only implemented for Linux -- with a proof of
concept
> > for elsewhere, I could see move it to being generic instead.  But
right
> > now, it''s Linux specific
> The other PV devices have their own Controller classes
> (BlkifController, NetifController, etc.).  Why is the framebuffer
> special?
Because I was able to "liberally use" a lot of the hvm code.  Also,
for
consistency with the hvm code, the framebuffer bits show up as part of
the image section of the sexpr rather than device.  Since one of the big
goals here (at least, from my point of view) is making full and para
virt domains more consistent, I''d like to keep to that.  

If you feel strongly, I can look at reworking it
> > > > +    def createDeviceModel(self):
> > > Maybe call ImageHandler.createDeviceModel?
> > The HVM one doesn''t -- perhaps both should although currently
the
> > comment in the superclass is such that it''s not going to
define anything
> I think that''s a bug in the HVM version, personally. 
I''ll have a look
> at it later.
There are whole diatribes written on when and whether you should call
superclass methods -- if the hvm one changes, I''ll change for PV ;-)

Jeremy


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

> > > > Why?
> > > Because it''s used to kill a process and doing a lazy
import of things
> > > like this is a good way to drive a man crazy ;-)
> > I''d drop this from this patch, since it''s not really
required or
> > particularly useful.
> > 
> > Don''t let that stop you from doing a separate cleanup patch,
> > though. :)
> Hint taken and sent ;-)
Thanks.
> > > > >  import xen.lowlevel.xc
> > > > > +import xen.util.auxbin
> > > > >  from xen.xend import sxp
> > > > >  from xen.xend.XendError import VmError
> > > > >  from xen.xend.XendLogging import log
> > > > > @@ -189,6 +191,68 @@ class
LinuxImageHandler(ImageHandler):
> > > > >                                cmdline        =
self.cmdline,
> > > > >                                ramdisk        =
self.ramdisk,
> > > > >                                features       =
self.vm.getFeatures())
> > > > > +
> > > > > +    def configure(self, imageConfig, deviceConfig):
> > > > Does this really belong in class LinuxImageHandler?
> > > Right now, it''s only implemented for Linux -- with a
proof of concept
> > > for elsewhere, I could see move it to being generic instead.  But
right
> > > now, it''s Linux specific
> > The other PV devices have their own Controller classes
> > (BlkifController, NetifController, etc.).  Why is the framebuffer
> > special?
> Because I was able to "liberally use" a lot of the hvm code. 
Also, for
> consistency with the hvm code, the framebuffer bits show up as part of
> the image section of the sexpr rather than device.
I''d be happier if the framebuffer stuff was in the device section of
the sexp, because it is a device, and there''s no reason in principle
why a domain couldn''t have more than one of them.
> Since one of the big goals here (at least, from my point of view) is
> making full and para virt domains more consistent, I''d like to
keep
> to that.
This is a good reason, but I think that the display configuration was
put in image in the HVM sexp for reasons of expediency rather than
correctness.  It''d be good not to copy that if we don''t have
to.

Even better would be to move the HVM configuration information to the
right place, but that''s probably a job for another time.
> If you feel strongly, I can look at reworking it
How much of a pain is this going to be?

Steven.


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

On Sat, Sep 02, 2006 at 03:58:37PM -0400, Jeremy Katz
wrote:
> Add support in xend and xm to know about the vnc and sdl options for PV
> domains.  Launch xen-sdlfb or xen-vncfb if we''re setting up a
graphics
> console.  Also, if we''re going to use a graphical console in the
guest,
> set /<dompath>/console/use_graphics to 1.
I''ve found a bit of a problematic interaction between the SEXPR for VNC
and bootloaders. The VNC related bit of the SEXPR is within the image
part, eg 

  (image 
    (linux 
       (kernel ''/var/lib/xen/vmlinuz.RSa3-O'')
       (ramdisk ''/var/lib/xen/initrd.ndZEf8'')
       (vnc 1)
       (vncdisplay 9)
    )
  )

If I''m using a bootloader though, the SEXPR I send to XenD does not
even
contain a ''(image (linux ...))'' block so there is no place to
specify the
VNC options. The hack of trying to supply a (image) section within a
kernel doesn''t work:

  (bootloader ''/usr/bin/pygrub'')
  (image
    (linux 
       (vnc 1)
       (vncdisplay 9)
    )
  )


Because if XenD finds a ''image'' node in the SEXPR it ignores
the bootloader
settings & assumes there is a (kernel) bit. Now we could hack it so that
XenD can handle a (image) bit without a kernel specified, but really this
is far from optimal. 

IMHO the VNC console options have no business being anywhere near the (image)
bit of the SEXPR since they''re nothing todo with kernel images
whatsoever.
Of course I understand why the PV framebuffer puts them here - its just
copying existing HVM style, but that doesn''t make it a good idea. I
think
we''d be better off having the framebuffer related stuff as a
fully-fledged
part of the (device) SEXPR block.

So this raises two questions, do we want to re-arrange the SEXPR to have
VNC opts in a sensible part, or is there a way we can make existing scheme
work nicely with bootloaders.


NB, the reason that ''xm'' can create domains with the
framebuffer active,
and using the bootloader is because xm runs ''pygrub'' client
side& then
munges the SEXPR before sending it to XenD. The problem occurrs only if
you are running pygrub server-side (ie letting XenD run it) because then
there is no way to specify any console options due to missing (image)
block.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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