> diff -rupN -x '*.orig' linux-2.6.16.29-xen/drivers/xen/console/console.c
> linux-2.6.16.29-xen-vfb/drivers/xen/console/console.c
> --- linux-2.6.16.29-xen/drivers/xen/console/console.c 2006-09-29
> 16:11:51.000000000 +0200
> +++ linux-2.6.16.29-xen-vfb/drivers/xen/console/console.c 2006-11-10
> 09:43:37.000000000 +0100
> @@ -195,12 +210,23 @@ static int __init xen_console_init(void)
> } else {
> if (!xen_start_info->console.domU.evtchn)
> goto out;
> - if (xc_mode == XC_DEFAULT)
> - xc_mode = XC_TTY;
> + if (xc_mode == XC_DEFAULT) {
> +#ifdef CONFIG_XEN_FRAMEBUFFER
> + xc_mode = XC_XVC;
> +#else
> + xc_mode = XC_TTY;
> +#endif
> + }
This hunk is changing the default behaviour of xencons in domU from
registering as tty{0...} to registering as xvc{0...}, yes? That's
going to confuse a lot of people with existing domUs which have gettys
etc. set up on /dev/ttyX but not /dev/xvcX.
Or am I confused again?
> kcons_info.write = kcons_write;
> }
>
> switch (xc_mode) {
>
> diff -rupN -x '*.orig' linux-2.6.16.29-xen/drivers/xen/Kconfig
> linux-2.6.16.29-xen-vfb/drivers/xen/Kconfig
> --- linux-2.6.16.29-xen/drivers/xen/Kconfig 2006-08-16 09:20:03.000000000
> +0200
> +++ linux-2.6.16.29-xen-vfb/drivers/xen/Kconfig 2006-11-10
> 09:43:37.000000000 +0100
> @@ -172,3 +172,19 @@ config XEN_NETDEV_FRONTEND
> dedicated device-driver domain, or your master control domain
> (domain 0), then you almost certainly want to say Y here.
>
> +config XEN_FRAMEBUFFER
> + tristate "Framebuffer-device frontend driver"
> + depends on XEN && FB
> + select FB_CFB_FILLRECT
> + select FB_CFB_COPYAREA
> + select FB_CFB_IMAGEBLIT
> + default y
> + help
> + The framebuffer-device frontend drivers allows the kernel to create a
> + virtual framebuffer. This framebuffer can be viewed in another
> + domain. Unless this domain has access to a real video card, you
> + probably want to say Y here.
> +
> +config XEN_KEYBOARD
> + tristate "Keyboard-device frontend driver"
> + depends on XEN
Does this want to depend on XEN_FRAMEBUFFER and INPUT?
> + default y
> + help
> + The keyboard-device frontend driver allows the kernel to create a
> + virtual keyboard. This keyboard can then be driven by another
> + domain. If you've said Y to CONFIG_XEN_FRAMEBUFFER, you probably
> + want to say Y here.
> +
> config XEN_SCRUB_PAGES
> bool "Scrub memory before freeing it to Xen"
> default y
> diff -rupN -x '*.orig' linux-2.6.16.29-xen/drivers/xen/console/console.c
> linux-2.6.16.29-xen-vfb/drivers/xen/console/console.c
> --- linux-2.6.16.29-xen/drivers/xen/console/console.c 2006-09-29
> 16:11:51.000000000 +0200
> +++ linux-2.6.16.29-xen-vfb/drivers/xen/console/console.c 2006-11-10
> 09:43:37.000000000 +0100
> @@ -680,3 +712,17 @@ static int __init xencons_init(void)
> printk("Xen virtual console successfully installed as %s%d\n",
> DRV(xencons_driver)->name, xc_num);
>
> + /* Don't need to check about graphical fb for domain 0 */
> + if (is_initial_xendomain())
> + return 0;
> +
> + rc = 0;
> + if (xenbus_scanf(XBT_NIL, "console", "use_graphics", "%d", &rc) < 0)
> + printk(KERN_ERR "Unable to read console/use_graphics\n");
> + if (rc == 0) {
> + /* FIXME: this is ugly */
> + unregister_console(&kcons_info);
> + kcons_info.flags |= CON_CONSDEV;
> + register_console(&kcons_info);
> + }
I'm still not entirely sure I understand why this bit is necessary.
From talking to katz@xxxxxxxxxx last time:
> > > > > Also, why do you need to set CON_CONSDEV at all?
> > > > Otherwise, you don't get kernel console messages.
> > > Why not? None of the other console drivers seem to need to set it
> > > explicitly, and it seems like it would break setting console=blah on
> > > the kernel command line.
> > Nothing else sets it explicitly because it happens implicitly in the
> > console registration code for the first thing registered with
> > register_console(). Alternately, if you have a console=, then
> > register_console ensures that your console device has CON_CONSDEV set.
> > kernel/printk.c if you want to read all the gory details
> The intent of this code seems to be to make sure that CON_CONSDEV is
> definitely set on xenconsole if use_graphics is not set, yes? But as
> far as I can see, if use_graphics is not set, xenfb should never call
> register_console(), and so this is all redundant.
I'm still just as unenlightened as I was then. Plus, xen_console_init
should be called before any of the pvfb stuff anyway, since it's a
console_initcall and pvfb starts from a module_init, so it's doubly
redundant.
> return 0;
> }
>
> diff -rupN -x '*.orig' linux-2.6.16.29-xen/drivers/xen/xenfb/xenfb.c
> linux-2.6.16.29-xen-vfb/drivers/xen/xenfb/xenfb.c
> --- linux-2.6.16.29-xen/drivers/xen/xenfb/xenfb.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ linux-2.6.16.29-xen-vfb/drivers/xen/xenfb/xenfb.c 2006-11-10
> 09:43:37.000000000 +0100
>
> +static int __devinit xenfb_probe(struct xenbus_device *dev,
> + const struct xenbus_device_id *id)
> +{
...
> + ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
> + if (ret < 0) {
> + framebuffer_release(fb_info);
> + xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
> + goto error;
> + }
> +
> + /* FIXME should this be delayed until backend XenbusStateConnected? */
I would, but I'm not sure it matters too much. You have a potentially
slightly confusing situation where someone starts using the
framebuffer before the backend connects, so the backend has to be able
to handle there being events on the ring when it starts, but I think
it probably works as it is.
> + ret = register_framebuffer(fb_info);
> + if (ret) {
> + fb_dealloc_cmap(&info->fb_info->cmap);
> + framebuffer_release(fb_info);
> + xenbus_dev_fatal(dev, ret, "register_framebuffer");
> + goto error;
> + }
> + info->fb_info = fb_info;
> +
> + info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
> + if (IS_ERR(info->kthread)) {
> + ret = PTR_ERR(info->kthread);
> + xenbus_dev_fatal(dev, ret, "register_framebuffer");
> + goto error;
goto error will end up calling xenfb_remove, which will notice that
info->kthread is non-NULL and try to kthread_stop it. That strikes me
as a bad idea.
...
> +static int __devexit xenfb_remove(struct xenbus_device *dev)
This gets called from some error paths even when the driver is
remaining loaded. Is __devexit safe? I'm thinking particularly of
the case where the kernel is compiled without module unloading and
discards exit text segments.
> +{
> + struct xenfb_info *info = dev->dev.driver_data;
> +
> + del_timer(&info->refresh);
> + if (info->kthread)
> + kthread_stop(info->kthread);
> + if (info->irq >= 0)
> + unbind_from_irqhandler(info->irq, info);
> + if (info->fb_info) {
> + unregister_framebuffer(info->fb_info);
> + fb_dealloc_cmap(&info->fb_info->cmap);
> + framebuffer_release(info->fb_info);
> + }
> + free_page((unsigned long)info->page);
> + vfree(info->mfns);
> + kfree(info->pages);
> + vfree(info->fb);
> + kfree(info);
> +
> + return 0;
> +}
> +
> +static struct xenbus_driver xenfb = {
> + .name = "vfb",
> + .owner = THIS_MODULE,
> + .ids = xenfb_ids,
> + .probe = xenfb_probe,
> + .remove = xenfb_remove,
I think this wants to be __devexit_p.
> + /* TODO .resume = xenfb_resume, */
> + .otherend_changed = xenfb_backend_changed,
> +};
> +
> +static int __init xenfb_init(void)
> +{
> + int ret;
> +
> + if (!is_running_on_xen())
> + return -ENODEV;
> +
> + /* Nothing to do if running in dom0. */
> + if (is_initial_xendomain())
> + return -ENODEV;
> + /* if we're not set up to use graphics mode, then don't initialize */
> + if (xenbus_scanf(XBT_NIL, "console", "use_graphics", "%d", &ret) < 0)
> + return -ENODEV;
> + if (ret == 0)
> + return -ENODEV;
Not sure about this. It seems to me like it might be useful to
sometimes have domains which have a PVFB but still use xencons for
kernel printks etc. Perhaps I'm just confused.
> +
> + return xenbus_register_frontend(&xenfb);
> +}
> +
> +static void __exit xenfb_cleanup(void)
> +{
> + return xenbus_unregister_driver(&xenfb);
> +}
> +
> +module_init(xenfb_init);
> +module_exit(xenfb_cleanup);
> +
> +MODULE_LICENSE("GPL");
> diff -rupN -x '*.orig' linux-2.6.16.29-xen/drivers/xen/xenkbd/xenkbd.c
> linux-2.6.16.29-xen-vfb/drivers/xen/xenkbd/xenkbd.c
> --- linux-2.6.16.29-xen/drivers/xen/xenkbd/xenkbd.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ linux-2.6.16.29-xen-vfb/drivers/xen/xenkbd/xenkbd.c 2006-11-10
> 09:43:37.000000000 +0100
> @@ -0,0 +1,123 @@
> +int __devinit xenkbd_probe(struct xenbus_device *dev,
> + const struct xenbus_device_id *id)
> +{
...
> + input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
> + input_dev->keybit[LONG(BTN_MOUSE)]
> + = BIT(BTN_LEFT) | BIT(BTN_MIDDLE) | BIT(BTN_RIGHT);
> + /* FIXME more buttons? */
> + input_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y);
Do you need to set some bits in absbit, as well, since you can
generate absolute coordinate messages?
> + input_dev->name = "Xen Virtual Keyboard/Mouse";
> +
> + input_set_abs_params(input_dev, ABS_X, 0, XENFB_WIDTH, 0, 0);
> + input_set_abs_params(input_dev, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
> +
> + /* FIXME should this be delayed until backend XenbusStateConnected? */
Again, I would move it if I were doing this, but it makes even less
difference here than it did before.
> + ret = register_framebuffer(fb_info);
> + if (ret) {
> + fb_dealloc_cmap(&info->fb_info->cmap);
> + framebuffer_release(fb_info);
> + xenbus_dev_fatal(dev, ret, "register_framebuffer");
> + goto error;
> + }
...
> +static int __devexit xenkbd_remove(struct xenbus_device *dev)
Again, not convinced __devexit is entirely valid here.
> +{
> + struct xenkbd_info *info = dev->dev.driver_data;
> +
> + if (info->irq >= 0)
> + unbind_from_irqhandler(info->irq, info);
> + input_unregister_device(info->dev);
> + free_page((unsigned long)info->page);
> + kfree(info);
> + return 0;
> +}
>
> +static struct xenbus_driver xenkbd = {
> + .name = "vkbd",
> + .owner = THIS_MODULE,
> + .ids = xenkbd_ids,
> + .probe = xenkbd_probe,
> + .remove = xenkbd_remove,
Again, might want to be __devexit_p.
> + //.resume = xenkbd_resume,
> + .otherend_changed = xenkbd_backend_changed,
> +};
> +
> +static int __init xenkbd_init(void)
> +{
> + int ret;
> +
> + if (!is_running_on_xen())
> + return -ENODEV;
> +
> + /* Nothing to do if running in dom0. */
> + if (is_initial_xendomain())
> + return -ENODEV;
> + /* if we're not set up to use graphics mode, then don't initialize */
> + if (xenbus_scanf(XBT_NIL, "console", "use_graphics", "%d", &ret) < 0)
> + return -ENODEV;
> + if (ret == 0)
> + return -ENODEV;
This test obviously wants to be kept in sync with the corresponding
test in xenfb_init, so the same comments apply here as there.
> + return xenbus_register_frontend(&xenkbd);
> +}
> diff -rupN -x '*.orig' linux-2.6.16.29-xen/include/xen/interface/io/xenfb.h
> linux-2.6.16.29-xen-vfb/include/xen/interface/io/xenfb.h
> --- linux-2.6.16.29-xen/include/xen/interface/io/xenfb.h 1970-01-01
> 01:00:00.000000000 +0100
> +++ linux-2.6.16.29-xen-vfb/include/xen/interface/io/xenfb.h 2006-11-10
> 09:43:37.000000000 +0100
I think this really wants to be in xen/include/public/io, but I'd
guess this is just an artifact of the diff.
> +struct xenfb_update
> +{
> + __u8 type; /* XENFB_TYPE_UPDATE */
> + __s32 x; /* source x */
> + __s32 y; /* source y */
> + __s32 width; /* rect width */
> + __s32 height; /* rect height */
> +};
> +
> +#define XENFB_OUT_EVENT_SIZE 40
> +
> +union xenfb_out_event
> +{
> + __u8 type;
> + struct xenfb_update update;
> + char pad[XENFB_OUT_EVENT_SIZE];
> +};
I still think you'd be better off doing tagged unions the usual way,
but your funeral.
> +/*
> + * Wart: xenkbd needs to know resolution. Put it here until a better
> + * solution is found, but don't leak it to the backend.
> + */
Why does xenkbd need to know the resolution? Do you mean xenfb?
As far as I can see, these only get used in xenfb.c, so the #defines
could go there rather than in the IO description header.
> +#ifdef __KERNEL__
> +#define XENFB_WIDTH 800
> +#define XENFB_HEIGHT 600
> +#define XENFB_DEPTH 32
> +#endif
> +
> +#endif
Steven.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home