WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] Paravirt framebuffer series [0/6]

To: Christian.Limpach@xxxxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH] Paravirt framebuffer series [0/6]
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Wed, 23 Aug 2006 15:50:48 +0200
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Amos Waterland <apw@xxxxxxxxxx>, Jeremy Katz <katzj@xxxxxxxxxx>, Ian Pratt <ian.pratt@xxxxxxxxxxxxx>, Anthony Liguori <anthony@xxxxxxxxxxxxx>, Christian Limpach <chris@xxxxxxxxxxxxx>
Delivery-date: Wed, 23 Aug 2006 06:51:15 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <3d8eece20608210344t4d860d30sb832df34f156bd53@xxxxxxxxxxxxxx> (Christian Limpach's message of "Mon, 21 Aug 2006 11:44:07 +0100")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1155935108.11663.48.camel@xxxxxxxxxxxxxx> <3d8eece20608210344t4d860d30sb832df34f156bd53@xxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux)
"Christian Limpach" <christian.limpach@xxxxxxxxx> writes:

[Review of frontend...]

I replied to that separately.

>> diff -r 6ca424e1867e -r 103a9f38f17f tools/xenfb/vncfb.c
>> --- /dev/null        Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/vncfb.c    Fri Aug 18 16:20:27 2006 -0400
>> @@ -0,0 +1,154 @@
>
>> +#if 0
>> +#define BUG() do { printf("%d\n", __LINE__); return 1; } while(0)
>> +#else
>> +extern void abort();
> #include <stdlib.h> ?

My debug code crept into the patch, oops.

Anthony's code had the first BUG() definition.  I hate it.  I want
code to crash hard when it detects it messed up.  Should be cleaned
up.  What behavior do you prefer?

>> +#define BUG() abort();
>> +#endif
>> +
>
>> +
>> +    rfbRunEventLoop(server, -1, TRUE);
> I'm a little worried that you appear to have a multithreaded program
> with no locks here, but nevermind.

As far as I can see, our client code doesn't share data with
LibVNCServer, it only calls its services.  Therefore, I don't think
locking is necessary, unless the LibVNCServer API requires it.  And
that would be odd, wouldn't it?  I can't tell for sure, as I'm not
familiar with it.

>> +struct xenfb *xenfb_new(void)
>> +{
> ...
>> +
>> +    xenfb->fd = open("/dev/xen/evtchn", O_RDWR);
> xc_evtchn_open, maybe.

Certainly.

>> +    if (xenfb->fd == -1) {
> ...
>> +}
>> +
>> +int xenfb_get_fileno(struct xenfb *xenfb_pub)
>> +{
>> +    struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +    if (xenfb->domid == -1)
>> +            return -1;
> Eh?

I don't understand why this is necessary or useful either.  I'll
remove it and see what happens.

>> +
>> +    return xenfb->fd;
>> +}
>> +
>> +static void xenfb_unset_domid(struct xenfb_private *xenfb)
> This does a fair bit more than unset the domid.

Sometimes things outgrow initial names... renamed to
xenfb_detach_dom().

>> +{
>> +    struct ioctl_evtchn_unbind unbind;
>> +
>> +    xenfb->domid = -1;
>> +
>> +    munmap(xenfb->fb, xenfb->fb_info->mem_length);
>> +    munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +    munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +    unbind.port = xenfb->fbdev_port;
>> +    ioctl(xenfb->fd, IOCTL_EVTCHN_UNBIND, &unbind);
>> +    unbind.port = xenfb->kbd_port;
>> +    ioctl(xenfb->fd, IOCTL_EVTCHN_UNBIND, &unbind);
> xc_evtchn_unbind?

Yes.

>> +}
>
>> +static int xenfb_fb_event(struct xenfb_private *xenfb, union xenfb_in_event 
>> *event)
>> +{
>> +    uint32_t prod;
>> +    struct xenfb_page *info = xenfb->fb_info;
>> +    struct ioctl_evtchn_notify notify;
>> +
>> +    prod = info->in_prod;
>> +    if (prod - info->in_cons == XENFB_RING_SIZE(info->in)) {
>> +        errno = EAGAIN;
>> +        return -1;
>> +    }
>> +
>> +    XENFB_RING_REF(info->in, prod) = *event;
> Need a memory barrier here.

You didn't ask for memory barriers in similar code in the frontend.
Did you just miss it there, or do I miss something?

>> +    info->in_prod = prod + 1;
>> +    notify.port = xenfb->fbdev_port;
>> +    return ioctl(xenfb->fd, IOCTL_EVTCHN_NOTIFY, &notify);
> xc_evtchn_notify()?

Yes.

>> +}
>> +
>> +static int xenfb_kbd_event(struct xenfb_private *xenfb, union 
>> xenkbd_in_event *event)
>> +{
>> +    uint32_t prod;
>> +    struct xenkbd_info *info = xenfb->kbd_info;
>> +    struct ioctl_evtchn_notify notify;
>> +
>> +    prod = info->in_prod;
>> +    if (prod - info->in_cons == XENKBD_RING_SIZE(info->in)) {
>> +        errno = EAGAIN;
>> +        return -1;
>> +    }
>> +
>> +    XENKBD_RING_REF(info->in, prod) = *event;
> Need a memory barrier.

Yes, just as in xenfb_fb_event() above.

>> +    info->in_prod = prod + 1;
>> +    notify.port = xenfb->kbd_port;
>> +    return ioctl(xenfb->fd, IOCTL_EVTCHN_NOTIFY, &notify);
>> +}
>> +
>> +static char *xenfb_path_in_dom(struct xs_handle *h,
>> +                     unsigned domid, const char *path,
>> +                     char *buffer, size_t size)
>> +{
>> +    char *domp = xs_get_domain_path(h, domid);
>> +    int n = snprintf(buffer, size, "%s/%s", domp, path);
>> +    free(domp);
>> +    if (n >= size)
>> +            return NULL;
>> +    return buffer;
>> +}
>> +
>> +static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned domid,
>> +                       const char *path, const char *fmt,
>> +                       void *dest)
> Not entirely convinced this is a good idea.  You could do this
> properly; vscanf isn't hard to use.

Laziness on my part.  Always try the most stupid thing that could
possibly solve the problem first ;)

I feel the proper solution belongs into a library anyway.

>> +{
>> +    char buffer[1024];
>> +    char *p;
>> +    int ret;
>> +
>> +    p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
>> +    p = xs_read(xsh, XBT_NULL, p, NULL);
>> +    if (!p)
>> +            return -ENOENT;
>> +    ret = sscanf(p, fmt, dest);
>> +    free(p);
>> +    if (ret != 1)
>> +            return -EDOM;
>> +    return 0;
>> +}
>> +
>> +bool xenfb_set_domid(struct xenfb *xenfb_pub, int domid)
> Perhaps misnamed?  This does a bit more than setting the domid.

Renamed to xenfb_attach_dom().

>> +{
> ...
>> +    }
>> +    xs_daemon_close(xsh);
>> +    xsh = NULL;
>> +
>> +    printf("%d, %d, %d\n", xenfb->fd, xenfb->fbdev_evtchn, domid);
> Umm?

Debugging leftover, removed.

>> +
>> +    bind.remote_port = xenfb->fbdev_evtchn;
>> +    bind.remote_domain = domid;
>> +    xenfb->fbdev_port = ioctl(xenfb->fd, IOCTL_EVTCHN_BIND_INTERDOMAIN, 
>> &bind);
> I'm goign to stop pointing out the xc_evtchn wrappers now.

I think I got them all :)

>> +
>> +    xenfb->kbd_info = xc_map_foreign_range(xenfb->xc, domid, XC_PAGE_SIZE,
>> +                                           PROT_READ | PROT_WRITE,
>> +                                           xenfb->kbd_mfn);
>> +    if (xenfb->kbd_info == NULL)
>> +            goto error_fbinfo;
>> +
>> +    while (!xenfb->fb_info->initialized) {
>> +            struct timeval tv = { 0, 10000 };
>> +            select(0, NULL, NULL, NULL, &tv);
>> +    }
> If you ever need to go into this loop there's a problem somewhere.
> You shouldn't be able to connect things up until the shared area is
> initialised.

I think this is a leftover of the pre-xenstore way to connect things.
I'll look into getting rid of the initialized flag.

>> +    xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ | 
>> PROT_WRITE, xenfb->fbmfns, xenfb->n_fbmfns);
>> +    if (xenfb->fb == NULL)
>> +            goto error_fbmfns;
>> +
>> +    {
>> +            union xenfb_in_event event;
> I know we do this in lots of other places, but it's really ugly.
> Can this move to the start of the function, please?

Sure.

>> +
>> +            event.type = XENFB_TYPE_SET_EVENTS;
>> +            event.set_events.flags = XENFB_FLAG_UPDATE;
>> +
>> +            if (xenfb_fb_event(xenfb, &event))
>> +                    goto error_fb;
>> +    }
>> +            
>> +    munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
> Why was fbmfns in xenfb rather than a local?

I guess Anthony felt it logically belongs there.  It could be made
local, along with a few other members.  I can do that.

>> +static void xenfb_update(struct xenfb_private *xenfb, int x, int y, int 
>> width, int height)
>> +{
>> +    if (xenfb->pub.update)
>> +            xenfb->pub.update(&xenfb->pub, x, y, width, height);
> Can pub.update ever be null?

Can't happen because both users (vncfb.c and sdlfb.c) fill it in
before they call xenfb_update().  But xenfb.c doesn't want to know
about that, so it checks.

>> +}
>> +
>> +static void xenfb_on_fb_event(struct xenfb_private *xenfb)
>> +{
>> +    uint32_t prod, cons;
>> +    struct xenfb_page *info = xenfb->fb_info;
>> +
>> +    prod = info->out_prod;
>> +    rmb();                  /* ensure we see ring contents up to prod */
>> +    for (cons = info->out_cons; cons != prod; cons++) {
>> +            union xenfb_out_event *event = &XENFB_RING_REF(info->out, cons);
>> +
>> +            switch (event->type) {
>> +            case XENFB_TYPE_UPDATE:
>> +                    xenfb_update(xenfb, event->update.x, event->update.y, 
>> event->update.width, event->update.height);
>> +                    break;
>> +            }
>> +    }
>> +    /* FIXME do I need a wmb() here? */
> Maybe.  Leaving it out means that the frontend can't reliably test for
> ring overflow.

I'm not sure I understand.  Can you give an example of how things can
go wrong?

>> +    info->out_cons = cons;
>> +}
>> +
>> +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
>> +{
>> +    uint32_t prod, cons;
>> +    struct xenkbd_info *info = xenfb->kbd_info;
>> +
>> +    prod = info->out_prod;
>> +    rmb();                  /* ensure we see ring contents up to prod */
>> +    for (cons = info->out_cons; cons != prod; cons++) {
>> +            union xenkbd_out_event *event = &XENKBD_RING_REF(info->out, 
>> cons);
>> +
>> +            switch (event->type) {
>> +            default:
>> +                    break;
>> +            }
> Huh?

Skeleton code.  There are no output events defined, yet.

>> +    }
>> +    /* FIXME do I need a wmb() here? */
> No, because the whole function is completely pointless.

It's a skeleton.  I can remove it if it bothers you.  If we keep it,
it should be as correct as we can make it.

Otherwise, same deal as for xenfb_fb_event().

>> +    info->out_cons = cons;
>> +}
>> +
>> +int xenfb_on_incoming(struct xenfb *xenfb_pub)
>> +{
>> +    struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +    evtchn_port_t port;
>> +
>> +    if (read(xenfb->fd, &port, sizeof(port)) != sizeof(port))
>> +            return -1;
>> +
>> +    if (port == xenfb->fbdev_port) {
>> +            xenfb_on_fb_event(xenfb);
>> +    } else if (port == xenfb->kbd_port) {
>> +            xenfb_on_kbd_event(xenfb);
> Does this ever happen?

Not with the current frontend.

>> +    }
>> +
>> +    if (write(xenfb->fd, &port, sizeof(port)) != sizeof(port))
>> +            return -1;
>> +
>> +    return 0;
>> +}
>> +

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