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

[Markus Armbruster <armbru@xxxxxxxxxx>]

"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