|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] PV framebuffer
Steven Smith <sos22-xen@xxxxxxxxxxxxx> writes:
>> 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?
Already discussed elsewhere.
>> 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?
XEN_FRAMEBUFFER: yes. Jeremy fixed that, but I mismerged.
INPUT: I missed that.
>> + 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.
Jeremy, could you elaborate?
>> 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.
Yes, it works.
I might move it eventually just for cleanliness.
>> + 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.
> ...
Oops. Thanks for catching.
>> +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.
Insufficient understanding of __devexit on my part. Fixing.
>> +{
>> + 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.
Only if xenfb is __devexit, right?
>> + /* 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.
I guess you're right.
>> +
>> + 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?
We missed that. Curiously, everything seems to work all the same (as
tested with evdev). Apparently, no consumer actually tests the bits
correctly. Fixing anyway.
>> + 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.
Understood.
>> + 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.
Same as above.
>> +{
>> + 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.
Same as above.
>> + //.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.
Agreed.
>> + 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.
Where in the *Linux* tree you want it? include/xen/interface or
include/xen/public or somewhere else?
>> +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.
While I personally prefer Anthony's way to do it, I'd convert to yours
if I could ensure the type's size remains exactly XENFB_OUT_EVENT_SIZE
portably and without undue ugliness. Alternatively, if I could ensure
the ring memory layout remains the same even when the type size
changes.
>> +/*
>> + * 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.
That's where they used to be. But xenkbd needs to tell the input
layer about the resolution to make absolute pointer work.
>> +#ifdef __KERNEL__
>> +#define XENFB_WIDTH 800
>> +#define XENFB_HEIGHT 600
>> +#define XENFB_DEPTH 32
>> +#endif
>> +
>> +#endif
>
> Steven.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |