Re: [Spice-devel] [PATCH] Do endian swapping.

[Christophe Fergeau]

On Wed, Dec 14, 2016 at 04:43:11PM +0100, Michal Suchánek wrote:
> On Wed, 14 Dec 2016 15:32:11 +0100
> Christophe Fergeau  wrote:
> 
> > Hey,
> > 
> > On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> > > This allows running big endian and little endian guest side by side
> > > using cut between them.
> > > 
> > > There is some general design idea that swapping should come as
> > > cloce to virtio_read/virtio_write as possible. In particular, the
> > > protocol between vdagent and vdagentd is guest-specific and in
> > > native endian. With muliple layers of headers this is a bit tricky.
> > > A few message types have to be swapped fully before passing through
> > > vdagentd.
> > > 
> > > Signed-off-by: Michal Suchanek 
> > > ---
> > >  src/vdagentd/uinput.c  |  4 +++
> > >  src/vdagentd/vdagentd.c| 68
> > > ++
> > > src/vdagentd/virtio-port.c | 35 +++- 3 files
> > > changed, 76 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> > > index e2966c4..21292cb 100644
> > > --- a/src/vdagentd/uinput.c
> > > +++ b/src/vdagentd/uinput.c
> > > @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct
> > > vdagentd_uinput **uinputp, };
> > >  int i, down;
> > >  
> > > +mouse->x = le32toh(mouse->x);
> > > +mouse->y = le32toh(mouse->y);
> > > +mouse->buttons = le32toh(mouse->buttons);
> > > +
> > >  if (*uinputp) {
> > >  if (mouse->display_id >= uinput->screen_count) {  
> > 
> > No swapping of mouse->display_id?
> 
> /usr/include/spice-1/spice/vd_agent.h:uint8_t display_id;

> > and would be missing a
> > vdata->nchannels byteswap if I'm not mistaken.
> 
> /usr/include/spice-1/spice/vd_agent.h:uint8_t nchannels;
> 

Ah, my bad, sorry for the noise!

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Do endian swapping.

[Christophe Fergeau]

Hey,

On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> This allows running big endian and little endian guest side by side using
> cut between them.
> 
> There is some general design idea that swapping should come as cloce to
> virtio_read/virtio_write as possible. In particular, the protocol between
> vdagent and vdagentd is guest-specific and in native endian. With muliple
> layers of headers this is a bit tricky. A few message types have to be swapped
> fully before passing through vdagentd.
> 
> Signed-off-by: Michal Suchanek 
> ---
>  src/vdagentd/uinput.c  |  4 +++
>  src/vdagentd/vdagentd.c| 68 
> ++
>  src/vdagentd/virtio-port.c | 35 +++-
>  3 files changed, 76 insertions(+), 31 deletions(-)
> 
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index e2966c4..21292cb 100644
> --- a/src/vdagentd/uinput.c
> +++ b/src/vdagentd/uinput.c
> @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct vdagentd_uinput 
> **uinputp,
>  };
>  int i, down;
>  
> +mouse->x = le32toh(mouse->x);
> +mouse->y = le32toh(mouse->y);
> +mouse->buttons = le32toh(mouse->buttons);
> +
>  if (*uinputp) {
>  if (mouse->display_id >= uinput->screen_count) {

No swapping of mouse->display_id?

> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..f91434d 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -399,15 +422,18 @@ static int virtio_port_read_complete(
>  if (message_header->size != sizeof(VDAgentMaxClipboard))
>  goto size_error;
>  VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
> -syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
> -max_clipboard = msg->max;
> +syslog(LOG_DEBUG, "Set max clipboard: %d", le32toh(msg->max));
> +max_clipboard = le32toh(msg->max);
>  break;
>  case VD_AGENT_AUDIO_VOLUME_SYNC:
>  if (message_header->size < sizeof(VDAgentAudioVolumeSync))
>  goto size_error;
> +VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync *)data;
> +if (message_header->size < sizeof(VDAgentAudioVolumeSync) +
> +vdata->nchannels * sizeof(vdata->volume[0]))
> +goto size_error;

This last change seems unrelated, and would be missing a vdata->nchannels
byteswap if I'm not mistaken.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Do endian swapping.

[Victor Toso]

Hi,

On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> This allows running big endian and little endian guest side by side using
> cut between them.
>
> There is some general design idea that swapping should come as cloce to
> virtio_read/virtio_write as possible. In particular, the protocol between
> vdagent and vdagentd is guest-specific and in native endian. With muliple
> layers of headers this is a bit tricky. A few message types have to be swapped
> fully before passing through vdagentd.
>
> Signed-off-by: Michal Suchanek 
> ---
>  src/vdagentd/uinput.c  |  4 +++
>  src/vdagentd/vdagentd.c| 68 
> ++
>  src/vdagentd/virtio-port.c | 35 +++-
>  3 files changed, 76 insertions(+), 31 deletions(-)
>
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index e2966c4..21292cb 100644
> --- a/src/vdagentd/uinput.c
> +++ b/src/vdagentd/uinput.c
> @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct vdagentd_uinput 
> **uinputp,
>  };
>  int i, down;
>
> +mouse->x = le32toh(mouse->x);
> +mouse->y = le32toh(mouse->y);
> +mouse->buttons = le32toh(mouse->buttons);
> +
>  if (*uinputp) {
>  if (mouse->display_id >= uinput->screen_count) {
>  syslog(LOG_WARNING, "mouse event for unknown monitor (%d >= %d)",
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..f91434d 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -83,7 +83,7 @@ static void send_capabilities(struct vdagent_virtio_port 
> *vport,
>  uint32_t request)
>  {
>  VDAgentAnnounceCapabilities *caps;
> -uint32_t size;
> +uint32_t size, i;
>
>  size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
>  caps = calloc(1, size);
> @@ -92,7 +92,7 @@ static void send_capabilities(struct vdagent_virtio_port 
> *vport,
>  return;
>  }
>
> -caps->request = request;
> +caps->request = htole32(request);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_REPLY);
> @@ -102,6 +102,8 @@ static void send_capabilities(struct vdagent_virtio_port 
> *vport,
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> +for (i = 0; i < VD_AGENT_CAPS_SIZE; i++)
> +caps->caps[i] = le32toh(caps->caps[i]);

hmm, I got confused here... I guess that the macros should take in
consideration the guest/client endianness as well..

>
>  vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
>VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> @@ -122,7 +124,10 @@ static void do_client_monitors(struct 
> vdagent_virtio_port *vport, int port_nr,
>  VDAgentMessage *message_header, VDAgentMonitorsConfig *new_monitors)
>  {
>  VDAgentReply reply;
> -uint32_t size;
> +uint32_t size, i;
> +
> +new_monitors->num_of_monitors = le32toh(new_monitors->num_of_monitors);
> +new_monitors->flags = le32toh(new_monitors->flags);

Instead handling the swapping in every message handler, i think it might
be nicer to do a helper function to be called at
virtio_port_read_complete()

>
>  /* Store monitor config to send to agents when they connect */
>  size = sizeof(VDAgentMonitorsConfig) +
> @@ -132,6 +137,14 @@ static void do_client_monitors(struct 
> vdagent_virtio_port *vport, int port_nr,
>  return;
>  }
>
> +for (i = 0; i < new_monitors->num_of_monitors; i++) {
> +new_monitors->monitors[i].height = 
> le32toh(new_monitors->monitors[i].height);
> +new_monitors->monitors[i].width = 
> le32toh(new_monitors->monitors[i].width);
> +new_monitors->monitors[i].depth = 
> le32toh(new_monitors->monitors[i].depth);
> +new_monitors->monitors[i].x = le32toh(new_monitors->monitors[i].x);
> +new_monitors->monitors[i].y = le32toh(new_monitors->monitors[i].y);
> +}
> +
>  vdagentd_write_xorg_conf(new_monitors);
>  
>  if (!mon_config ||
> @@ -151,8 +164,8 @@ static void do_client_monitors(struct vdagent_virtio_port 
> *vport, int port_nr,
>  (uint8_t *)mon_config, size);
>  
>  /* Acknowledge reception of monitors config to spice server / client */
> -reply.type  = VD_AGENT_MONITORS_CONFIG;
> -reply.error = VD_AGENT_SUCCESS;
> +reply.type  = htole32(VD_AGENT_MONITORS_CONFIG);
> +reply.error = htole32(VD_AGENT_SUCCESS);
>  vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
>(uint8_t *), sizeof(reply));
>  }
> @@ -161,11 +174,15 @@ static void do_client_volume_sync(struct 
> vdagent_virtio_port *vport, int port_nr
>  VDAgentMessage *message_header,
>

Re: [Spice-devel] [PATCH] Do endian swapping.

[Michal Suchánek]

Hello,
On Mon, 28 Nov 2016 15:08:34 +0100
Michal Suchanek  wrote:

> This allows running big endian and little endian guest side by side
> using cut between them.
> 
> There is some general design idea that swapping should come as cloce
> to virtio_read/virtio_write as possible. In particular, the protocol
> between vdagent and vdagentd is guest-specific and in native endian.
> With muliple layers of headers this is a bit tricky. A few message
> types have to be swapped fully before passing through vdagentd.

Is there any problem with this patch?

I was able to test

 - mouse events
 - cut & paste
 - audio volume sync

unused

 - file transfer

unsupported

 - monitor size sync (qemu ppc64 does not support qxl graphics)

Due to switch from big endian to little endian as default on ppc64 the
ability to use the agent is very useful for GUI testing of older
distribution releases.

Thanks

Michal
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel