Re: [Xen-devel] [RFC] PVFB: Add refresh period to XenStore parameters?

[Markus Armbruster <armbru@xxxxxxxxxx>]

Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx> writes:

> Hello,
>
> Something like this then?

Yes.  Comments inline.

> ioemu: transmit idleness information to avoid useless polls
>
> Signed-off-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>
>
> diff -r 32342f2b5742 extras/mini-os/fbfront.c
> --- a/extras/mini-os/fbfront.c        Fri May 02 15:12:43 2008 +0100
> +++ b/extras/mini-os/fbfront.c        Mon May 05 17:44:16 2008 +0100
> @@ -249,11 +249,41 @@ struct fbfront_dev {
>      int stride;
>      int mem_length;
>      int offset;
> +
> +    int status;

Suggest a more descriptive name, or, if you can't think of any, a
descriptive comment.

Oh, see comments on fbif.h below.

>  };
>  
>  void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
>  {
> +    struct fbfront_dev *dev = data;
> +    struct xenfb_page *page = dev->page;
> +    uint32_t prod, cons;
> +    int i;
> +
>      wake_up(&fbfront_queue);
> +    prod = page->in_prod;
> +
> +    if (prod == page->in_cons)
> +        return;
> +
> +    rmb();      /* ensure we see ring contents up to prod */
> +
> +    for (i = 0, cons = page->in_cons; cons != prod; i++, cons++) {

Purpose of i ?

> +        union xenfb_in_event *event = &XENFB_IN_RING_REF(page, cons);
> +        switch (event->type) {
> +        case XENFB_TYPE_BACKEND_STATUS:
> +            dev->status = event->status.status;
> +            printk("got status %d\n", dev->status);
> +            break;
> +        default:
> +            /* ignore */

Suggest /* ignore unknown events */

> +            break;
> +        }
> +    }
> +    mb();       /* ensure we got ring contents */
> +    page->in_cons = cons;
> +
> +    notify_remote_via_evtchn(dev->evtchn);
>  }
>  
>  struct fbfront_dev *init_fbfront(char *nodename, unsigned long *mfns, int 
> width, int height, int depth, int stride, int n)
> @@ -292,6 +322,7 @@ struct fbfront_dev *init_fbfront(char *n
>      dev->stride = s->line_length = stride;
>      dev->mem_length = s->mem_length = n * PAGE_SIZE;
>      dev->offset = 0;
> +    dev->status = XENFB_BACKEND_STATUS_ACTIVE;
>  
>      const int max_pd = sizeof(s->pd) / sizeof(s->pd[0]);
>      unsigned long mapped = 0;
> @@ -405,6 +436,11 @@ static void fbfront_out_event(struct fbf
>      wmb(); /* ensure ring contents visible */
>      page->out_prod = prod + 1;
>      notify_remote_via_evtchn(dev->evtchn);
> +}
> +
> +int fbfront_status(struct fbfront_dev *dev)
> +{
> +    return dev->status;
>  }
>  
>  void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int 
> height)
> diff -r 32342f2b5742 extras/mini-os/include/fbfront.h
> --- a/extras/mini-os/include/fbfront.h        Fri May 02 15:12:43 2008 +0100
> +++ b/extras/mini-os/include/fbfront.h        Mon May 05 17:44:16 2008 +0100
> @@ -36,6 +36,7 @@ int fbfront_open(struct fbfront_dev *dev
>  int fbfront_open(struct fbfront_dev *dev);
>  #endif
>  
> +int fbfront_status(struct fbfront_dev *dev);
>  void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int 
> height);
>  void fbfront_resize(struct fbfront_dev *dev, int width, int height, int 
> stride, int depth, int offset);
>  
> diff -r 32342f2b5742 tools/ioemu/hw/xenfb.c
> --- a/tools/ioemu/hw/xenfb.c  Fri May 02 15:12:43 2008 +0100
> +++ b/tools/ioemu/hw/xenfb.c  Mon May 05 17:44:16 2008 +0100
> @@ -59,6 +59,7 @@ struct xenfb {
>       int offset;             /* offset of the framebuffer */
>       int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>       int button_state;       /* Last seen pointer button state */
> +        int notified_active;   /* Did we request update */

Tab, please, if it's not too much trouble.  Mixing tabs and spaces for
indentation makes diffs unnecessarily hard to read.

>       char protocol[64];      /* frontend protocol */
>  };
>  
> @@ -536,6 +537,40 @@ static void xenfb_on_fb_event(struct xen
>       xc_evtchn_notify(xenfb->evt_xch, xenfb->fb.port);
>  }
>  
> +static int xenfb_queue_full(struct xenfb *xenfb)
> +{
> +        struct xenfb_page *page = xenfb->fb.page;
> +        uint32_t cons, prod;
> +
> +        prod = page->in_prod;
> +        cons = page->in_cons;
> +        return prod - cons == XENFB_IN_RING_LEN;
> +}
> +
> +static void xenfb_send_event(struct xenfb *xenfb, union xenfb_in_event 
> *event)
> +{
> +        uint32_t prod;
> +        struct xenfb_page *page = xenfb->fb.page;
> +
> +        prod = page->in_prod;
> +        /* caller ensures !xenfb_queue_full() */
> +        xen_mb();                   /* ensure ring space available */
> +        XENFB_IN_RING_REF(page, prod) = *event;
> +        xen_wmb();                  /* ensure ring contents visible */
> +        page->in_prod = prod + 1;
> +
> +     xc_evtchn_notify(xenfb->evt_xch, xenfb->fb.port);

Ditto.

> +}
> +
> +static void xenfb_send_status(struct xenfb *xenfb, int active)
> +{
> +        union xenfb_in_event event;

Please memset(&event, 0, sizeof(event))

Makes backward-compatible evolution of the protocol so much easier,
and removes all doubts about leaking potentially sensitive bits.

> +        event.type = XENFB_TYPE_BACKEND_STATUS;
> +        event.status.status = active ? XENFB_BACKEND_STATUS_ACTIVE
> +                                     : XENFB_BACKEND_STATUS_IDLE ;
> +        xenfb_send_event(xenfb, &event);
> +}
> +
>  static void xenfb_on_kbd_event(struct xenfb *xenfb)
>  {
>       struct xenkbd_page *page = xenfb->kbd.page;
> @@ -707,6 +742,7 @@ static int xenfb_read_frontend_fb_config
>                              xenfb->protocol) < 0)
>                  xenfb->protocol[0] = '\0';
>          xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", 
> "1");
> +        xenfb->notified_active = 1;
>  
>          /* TODO check for permitted ranges */
>          fb_page = xenfb->fb.page;
> @@ -1185,10 +1221,21 @@ static void xenfb_guest_copy(struct xenf
>      dpy_update(xenfb->ds, x, y, w, h);
>  }
>  
> -/* Periodic update of display, no need for any in our case */
> +/* Periodic update of display, just announce idleness to the front end */
>  static void xenfb_update(void *opaque)
>  {
>      struct xenfb *xenfb = opaque;
> +    if (xenfb->ds->idle) {
> +        if (xenfb->notified_active && !xenfb_queue_full(xenfb)) {
> +            xenfb_send_status(xenfb, 0);
> +            xenfb->notified_active = 0;
> +        }
> +    } else {
> +        if (!xenfb->notified_active && !xenfb_queue_full(xenfb)) {
> +            xenfb_send_status(xenfb, 1);
> +            xenfb->notified_active = 1;
> +        }
> +    }

This breaks if we ever support frontends without feature-update.
Worth a comment.

>  }
>  
>  /* QEMU display state changed, so refresh the framebuffer copy */

[CONFIG_STUBDOM stuff snipped, I'm too ignorant about that to comment...]
> diff -r 32342f2b5742 tools/ioemu/sdl.c
> --- a/tools/ioemu/sdl.c       Fri May 02 15:12:43 2008 +0100
> +++ b/tools/ioemu/sdl.c       Mon May 05 17:44:16 2008 +0100
[more snippage due to ignorance...]
> diff -r 32342f2b5742 tools/ioemu/vl.h
> --- a/tools/ioemu/vl.h        Fri May 02 15:12:43 2008 +0100
> +++ b/tools/ioemu/vl.h        Mon May 05 17:44:16 2008 +0100
[ditto...]
> diff -r 32342f2b5742 tools/ioemu/vnc.c
[ditto...]
> diff -r 32342f2b5742 xen/include/public/io/fbif.h
> --- a/xen/include/public/io/fbif.h    Fri May 02 15:12:43 2008 +0100
> +++ b/xen/include/public/io/fbif.h    Mon May 05 17:44:16 2008 +0100
> @@ -80,14 +80,30 @@ union xenfb_out_event
>  
>  /*
>   * Frontends should ignore unknown in events.
> - * No in events currently defined.
>   */
> +
> +/*
> + * Backend idleness report
> + * Backend sends it when the output window is somehow non visible
> + * (minimized, no client, etc.)
> + */
> +#define XENFB_TYPE_BACKEND_STATUS 1
> +
> +#define XENFB_BACKEND_STATUS_IDLE 0
> +#define XENFB_BACKEND_STATUS_ACTIVE 1
> +
> +struct xenfb_backend_status
> +{
> +    uint8_t type;    /* XENFB_TYPE_BACKEND_STATUS */
> +    uint8_t status;  /* XENFB_BACKEND_STATUS_* */
> +};

I'm not entirely happy with the protocol defined here.

1. Encoding of idleness

   If idleness is and will remain boolean, I'd rather encode it the
   old-fashioned C way as zero / non-zero, because that removes all
   doubt on how to interpret funny status values.

   If you want to reserve funny status values for future extensions of
   the notion "idleness", then you need to document that here, along
   with what frontends should do for values they don't know.

2. Names

   If the message is intended just for reporting idleness, it should
   be called something like BACKEND_IDLE instead of STATUS.

   If it is meant to be extensible for reporting status other than
   idleness, the member status isn't named properly.  If the member
   holds just idleness, it should be named is_idle (if yes/no),
   idleness (if more than two values), or something like that.  If
   idleness is yes/no, you could also encode it as a bit in a status
   word instead.

Aside: going through the ring requires us to write down the protocol
here.  You didn't (have to) do that when you went through xenstore.
Guess what I like better :)

>  
>  #define XENFB_IN_EVENT_SIZE 40
>  
>  union xenfb_in_event
>  {
>      uint8_t type;
> +    struct xenfb_backend_status status;
>      char pad[XENFB_IN_EVENT_SIZE];
>  };
>  

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