Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows PV frontend

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Thu, 11 Sep 2014, Owen Smith wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 11 September 2014 02:23
> > To: Owen Smith
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini
> > Subject: Re: [PATCH 3/3] xenfb: Add "feature-no-abs-rescale" for Windows
> > PV frontend
> > 
> > On Wed, 10 Sep 2014, Owen smith wrote:
> > > Windows PV frontend requires absolute mouse values in the range [0,
> > > 0x7fff]. Add a feature to not rescale the axes to DisplaySurface size.
> > > Also allows Windows PV frontend to connect without a vfb device. Moves
> > > the rescaling of axes to a seperate function with additional
> > > null-pointer checks.
> > >
> > > Signed-off-by: Owen smith <owen.smith@citrix.com>
> > > ---
> > >  hw/display/xenfb.c | 52
> > > +++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 37 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index
> > > c4375c8..65c373b 100644
> > > --- a/hw/display/xenfb.c
> > > +++ b/hw/display/xenfb.c
> > > @@ -61,6 +61,7 @@ struct common {
> > >  struct XenInput {
> > >      struct common c;
> > >      int abs_pointer_wanted; /* Whether guest supports absolute
> > > pointer */
> > > +    int no_abs_rescale;     /* Whether guest wants rescaled abs axes */
> > >      int button_state;       /* Last seen pointer button state */
> > >      int extended;
> > >      QEMUPutKbdEntry *qkbd;
> > > @@ -325,6 +326,25 @@ static void xenfb_key_event(void *opaque, int
> > scancode)
> > >      xenfb_send_key(xenfb, down, scancode2linux[scancode]);  }
> > >
> > > +static void xenfb_rescale_mouse(struct XenInput *xenfb, int *dx, int
> > > +*dy) {
> > > +    DisplaySurface *surface;
> > > +    int dw, dh;
> > > +
> > > +    if (xenfb->c.con == NULL) {
> > > +        return;
> > > +    }
> > > +    surface = qemu_console_surface(xenfb->c.con);
> > > +    if (surface == NULL) {
> > > +        return;
> > > +    }
> > > +    dw = surface_width(surface);
> > > +    dh = surface_height(surface);
> > > +
> > > +    *dx = *dx * (dw - 1) / 0x7fff;
> > > +    *dy = *dy * (dh - 1) / 0x7fff;
> > > +}
> > > +
> > >  /*
> > >   * Send a mouse event from the client to the guest OS
> > >   *
> > > @@ -338,18 +358,16 @@ static void xenfb_mouse_event(void *opaque,
> > >  			      int dx, int dy, int dz, int button_state)  {
> > >      struct XenInput *xenfb = opaque;
> > > -    DisplaySurface *surface = qemu_console_surface(xenfb->c.con);
> > > -    int dw = surface_width(surface);
> > > -    int dh = surface_height(surface);
> > >      int i;
> > >
> > > -    if (xenfb->abs_pointer_wanted)
> > > -	xenfb_send_position(xenfb,
> > > -			    dx * (dw - 1) / 0x7fff,
> > > -			    dy * (dh - 1) / 0x7fff,
> > > -			    dz);
> > > -    else
> > > +    if (xenfb->abs_pointer_wanted) {
> > > +        if (!xenfb->no_abs_rescale) {
> > > +            xenfb_rescale_mouse(xenfb, &dx, &dy);
> > > +        }
> > > +        xenfb_send_position(xenfb, dx, dy, dz);
> > > +    } else {
> > >  	xenfb_send_motion(xenfb, dx, dy, dz);
> > > +    }
> > >
> > >      for (i = 0 ; i < 8 ; i++) {
> > >  	int lastDown = xenfb->button_state & (1 << i); @@ -366,6 +382,7
> > @@
> > > static void xenfb_mouse_event(void *opaque,  static int
> > > input_init(struct XenDevice *xendev)  {
> > >      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> > > +    xenstore_write_be_int(xendev, "feature-no-abs-rescale", 1);
> > >      return 0;
> > >  }
> > >
> > > @@ -374,7 +391,17 @@ static int input_initialise(struct XenDevice
> > *xendev)
> > >      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
> > >      int rc;
> > >
> > > -    if (!in->c.con) {
> > > +    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > > +                             &in->abs_pointer_wanted) == -1) {
> > > +        in->abs_pointer_wanted = 0;
> > > +    }
> > > +
> > > +    if (xenstore_read_fe_int(xendev, "request-no-abs-rescale",
> > > +                             &in->no_abs_rescale) == -1) {
> > > +        in->no_abs_rescale = 0;
> > > +    }
> > 
> > I don't think is safe to move checking for "request-abs-pointer" so
> > early: what if the guest writes request-abs-pointer after initialise but before
> > connect?
> > 
> > Also it would be nice to document this protocol extension somewhere. I do
> > realize that the existing documentation on vfb is somewhat lacking.
> > 
> 
> I'll change this to read the feature flags during the input_connected for v2. 
> I've spotted that neither feature has anything in the xen header, so I'll include 
> a patch to document this with v2.

Thanks


> > > +    if (!in->c.con && !in->no_abs_rescale) {
> > 
> > Why this change?
> > 
> 
> Without an additional check here, the frontend would not connect. in->c.con is
> not initialised without a vfb device. Overloading no_abs_rescale here is not right,
> I'll add an option that the frontend can set to ignore the absence of the console.

You don't need a new option for that. We wouldn't be introducing any
regression by letting vkdb connect without a vfb device.  You could just
remove the check.


> 
> > 
> > >          xen_be_printf(xendev, 1, "ds not set (yet)\n");
> > >          return -1;
> > >      }
> > > @@ -390,11 +417,6 @@ static void input_connected(struct XenDevice
> > > *xendev)  {
> > >      struct XenInput *in = container_of(xendev, struct XenInput,
> > > c.xendev);
> > >
> > > -    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
> > > -                             &in->abs_pointer_wanted) == -1) {
> > > -        in->abs_pointer_wanted = 0;
> > > -    }
> > > -
> > >      if (in->qmouse) {
> > >          qemu_remove_mouse_event_handler(in->qmouse);
> > >      }
> > > --
> > > 2.1.0
> > >
>