[Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Aurelien Jarno]

virtio-net used to work on cross-endianness configurations, but doesn't
anymore with recent guest kernels, as the new features don't handle
endianness correctly.

This patch fixes wrong conversion, and add missing ones to make
virtio-net working. Tested on the following configurations:
- i386 guest on x86_64 host
- ppc guest on x86_64 host
- i386 guest on mips host
- ppc guest on mips host

Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/virtio-net.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ec1bf8d..515fb19 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -81,7 +81,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = to_virtio_net(vdev);
     struct virtio_net_config netcfg;
 
-    netcfg.status = n->status;
+    netcfg.status = lduw_p(&n->status);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, sizeof(netcfg));
 }
@@ -340,7 +340,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
     n->mac_table.multi_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
 
-    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
+    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
 
     if (sizeof(mac_data.entries) +
         (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
@@ -356,7 +356,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     n->mac_table.first_multi = n->mac_table.in_use;
 
-    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
+    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
 
     if (sizeof(mac_data.entries) +
         (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
@@ -386,7 +386,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    vid = lduw_le_p(elem->out_sg[1].iov_base);
+    vid = lduw_p(elem->out_sg[1].iov_base);
 
     if (vid >= MAX_VLAN)
         return VIRTIO_NET_ERR;
@@ -675,8 +675,9 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
         virtqueue_fill(n->rx_vq, &elem, total, i++);
     }
 
-    if (mhdr)
-        mhdr->num_buffers = i;
+    if (mhdr) {
+        mhdr->num_buffers = lduw_p(&i);
+    }
 
     virtqueue_flush(n->rx_vq, i);
     virtio_notify(&n->vdev, n->rx_vq);
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/2] virtio-blk: fix cross-endianness targets

[Aurelien Jarno]

virtio-blk doesn't work on cross-endian configuration, as endianness is
not handled correctly.

This patch adds missing endianness conversions to make virtio-blk
working. Tested on the following configurations:
- i386 guest on x86_64 host
- ppc guest on x86_64 host
- i386 guest on mips host
- ppc guest on mips host

Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/virtio-blk.c |   40 +++++++++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index f62ccd1..9f2a9c0 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -55,7 +55,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 
     trace_virtio_blk_req_complete(req, status);
 
-    req->in->status = status;
+    stb_p(&req->in->status, status);
     virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
     virtio_notify(&s->vdev, s->vq);
 
@@ -94,7 +94,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     trace_virtio_blk_rw_complete(req, ret);
 
     if (ret) {
-        int is_read = !(req->out->type & VIRTIO_BLK_T_OUT);
+        int is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -223,10 +223,10 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
         status = VIRTIO_BLK_S_OK;
     }
 
-    req->scsi->errors = hdr.status;
-    req->scsi->residual = hdr.resid;
-    req->scsi->sense_len = hdr.sb_len_wr;
-    req->scsi->data_len = hdr.dxfer_len;
+    stl_p(&req->scsi->errors, hdr.status);
+    stl_p(&req->scsi->residual, hdr.resid);
+    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
+    stl_p(&req->scsi->data_len, hdr.dxfer_len);
 
     virtio_blk_req_complete(req, status);
 }
@@ -280,10 +280,13 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     BlockRequest *blkreq;
+    uint64_t sector;
 
-    trace_virtio_blk_handle_write(req, req->out->sector, req->qiov.size / 512);
+    sector = ldq_p(&req->out->sector);
 
-    if (req->out->sector & req->dev->sector_mask) {
+    trace_virtio_blk_handle_write(req, sector, req->qiov.size / 512);
+
+    if (sector & req->dev->sector_mask) {
         virtio_blk_rw_complete(req, -EIO);
         return;
     }
@@ -293,7 +296,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     }
 
     blkreq = &mrb->blkreq[mrb->num_writes];
-    blkreq->sector = req->out->sector;
+    blkreq->sector = sector;
     blkreq->nb_sectors = req->qiov.size / BDRV_SECTOR_SIZE;
     blkreq->qiov = &req->qiov;
     blkreq->cb = virtio_blk_rw_complete;
@@ -306,13 +309,16 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     BlockDriverAIOCB *acb;
+    uint64_t sector;
+
+    sector = ldq_p(&req->out->sector);
 
-    if (req->out->sector & req->dev->sector_mask) {
+    if (sector & req->dev->sector_mask) {
         virtio_blk_rw_complete(req, -EIO);
         return;
     }
 
-    acb = bdrv_aio_readv(req->dev->bs, req->out->sector, &req->qiov,
+    acb = bdrv_aio_readv(req->dev->bs, sector, &req->qiov,
                          req->qiov.size / BDRV_SECTOR_SIZE,
                          virtio_blk_rw_complete, req);
     if (!acb) {
@@ -323,6 +329,8 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 static void virtio_blk_handle_request(VirtIOBlockReq *req,
     MultiReqBuffer *mrb)
 {
+    uint32_t type;
+
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
         error_report("virtio-blk missing headers");
         exit(1);
@@ -337,17 +345,19 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     req->out = (void *)req->elem.out_sg[0].iov_base;
     req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
 
-    if (req->out->type & VIRTIO_BLK_T_FLUSH) {
+    type = ldl_p(&req->out->type);
+
+    if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
-    } else if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
+    } else if (type & VIRTIO_BLK_T_SCSI_CMD) {
         virtio_blk_handle_scsi(req);
-    } else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
+    } else if (type & VIRTIO_BLK_T_GET_ID) {
         VirtIOBlock *s = req->dev;
 
         memcpy(req->elem.in_sg[0].iov_base, s->sn,
                MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn)));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
-    } else if (req->out->type & VIRTIO_BLK_T_OUT) {
+    } else if (type & VIRTIO_BLK_T_OUT) {
         qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
                                  req->elem.out_num - 1);
         virtio_blk_handle_write(req, mrb);
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: fix cross-endianness targets

[Aurelien Jarno]

On Sat, Jan 15, 2011 at 08:27:44PM +0100, Aurelien Jarno wrote:
> virtio-blk doesn't work on cross-endian configuration, as endianness is
> not handled correctly.
> 
> This patch adds missing endianness conversions to make virtio-blk
> working. Tested on the following configurations:
> - i386 guest on x86_64 host
> - ppc guest on x86_64 host
> - i386 guest on mips host
> - ppc guest on mips host
> 
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/virtio-blk.c |   40 +++++++++++++++++++++++++---------------
>  1 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index f62ccd1..9f2a9c0 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -55,7 +55,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
>  
>      trace_virtio_blk_req_complete(req, status);
>  
> -    req->in->status = status;
> +    stb_p(&req->in->status, status);
>      virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
>      virtio_notify(&s->vdev, s->vq);
>  
> @@ -94,7 +94,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>      trace_virtio_blk_rw_complete(req, ret);
>  
>      if (ret) {
> -        int is_read = !(req->out->type & VIRTIO_BLK_T_OUT);
> +        int is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
>          if (virtio_blk_handle_rw_error(req, -ret, is_read))
>              return;
>      }
> @@ -223,10 +223,10 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>          status = VIRTIO_BLK_S_OK;
>      }
>  
> -    req->scsi->errors = hdr.status;
> -    req->scsi->residual = hdr.resid;
> -    req->scsi->sense_len = hdr.sb_len_wr;
> -    req->scsi->data_len = hdr.dxfer_len;
> +    stl_p(&req->scsi->errors, hdr.status);
> +    stl_p(&req->scsi->residual, hdr.resid);
> +    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
> +    stl_p(&req->scsi->data_len, hdr.dxfer_len);
>  
>      virtio_blk_req_complete(req, status);
>  }
> @@ -280,10 +280,13 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>  static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>  {
>      BlockRequest *blkreq;
> +    uint64_t sector;
>  
> -    trace_virtio_blk_handle_write(req, req->out->sector, req->qiov.size / 512);
> +    sector = ldq_p(&req->out->sector);
>  
> -    if (req->out->sector & req->dev->sector_mask) {
> +    trace_virtio_blk_handle_write(req, sector, req->qiov.size / 512);
> +
> +    if (sector & req->dev->sector_mask) {
>          virtio_blk_rw_complete(req, -EIO);
>          return;
>      }
> @@ -293,7 +296,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>      }
>  
>      blkreq = &mrb->blkreq[mrb->num_writes];
> -    blkreq->sector = req->out->sector;
> +    blkreq->sector = sector;
>      blkreq->nb_sectors = req->qiov.size / BDRV_SECTOR_SIZE;
>      blkreq->qiov = &req->qiov;
>      blkreq->cb = virtio_blk_rw_complete;
> @@ -306,13 +309,16 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>  static void virtio_blk_handle_read(VirtIOBlockReq *req)
>  {
>      BlockDriverAIOCB *acb;
> +    uint64_t sector;
> +
> +    sector = ldq_p(&req->out->sector);
>  
> -    if (req->out->sector & req->dev->sector_mask) {
> +    if (sector & req->dev->sector_mask) {
>          virtio_blk_rw_complete(req, -EIO);
>          return;
>      }
>  
> -    acb = bdrv_aio_readv(req->dev->bs, req->out->sector, &req->qiov,
> +    acb = bdrv_aio_readv(req->dev->bs, sector, &req->qiov,
>                           req->qiov.size / BDRV_SECTOR_SIZE,
>                           virtio_blk_rw_complete, req);
>      if (!acb) {
> @@ -323,6 +329,8 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
>  static void virtio_blk_handle_request(VirtIOBlockReq *req,
>      MultiReqBuffer *mrb)
>  {
> +    uint32_t type;
> +
>      if (req->elem.out_num < 1 || req->elem.in_num < 1) {
>          error_report("virtio-blk missing headers");
>          exit(1);
> @@ -337,17 +345,19 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>      req->out = (void *)req->elem.out_sg[0].iov_base;
>      req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
>  
> -    if (req->out->type & VIRTIO_BLK_T_FLUSH) {
> +    type = ldl_p(&req->out->type);
> +
> +    if (type & VIRTIO_BLK_T_FLUSH) {
>          virtio_blk_handle_flush(req, mrb);
> -    } else if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
> +    } else if (type & VIRTIO_BLK_T_SCSI_CMD) {
>          virtio_blk_handle_scsi(req);
> -    } else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
> +    } else if (type & VIRTIO_BLK_T_GET_ID) {
>          VirtIOBlock *s = req->dev;
>  
>          memcpy(req->elem.in_sg[0].iov_base, s->sn,
>                 MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn)));
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> -    } else if (req->out->type & VIRTIO_BLK_T_OUT) {
> +    } else if (type & VIRTIO_BLK_T_OUT) {
>          qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
>                                   req->elem.out_num - 1);
>          virtio_blk_handle_write(req, mrb);
> -- 
> 1.7.2.3
> 

Ping?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: fix cross-endianness targets

[Stefan Hajnoczi]

On Sat, Jan 15, 2011 at 7:27 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> virtio-blk doesn't work on cross-endian configuration, as endianness is
> not handled correctly.
>
> This patch adds missing endianness conversions to make virtio-blk
> working. Tested on the following configurations:
> - i386 guest on x86_64 host
> - ppc guest on x86_64 host
> - i386 guest on mips host
> - ppc guest on mips host
>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/virtio-blk.c |   40 +++++++++++++++++++++++++---------------
>  1 files changed, 25 insertions(+), 15 deletions(-)

Nice to have cross-endian working.

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Aurelien Jarno]

On Sat, Jan 15, 2011 at 08:27:43PM +0100, Aurelien Jarno wrote:
> virtio-net used to work on cross-endianness configurations, but doesn't
> anymore with recent guest kernels, as the new features don't handle
> endianness correctly.
> 
> This patch fixes wrong conversion, and add missing ones to make
> virtio-net working. Tested on the following configurations:
> - i386 guest on x86_64 host
> - ppc guest on x86_64 host
> - i386 guest on mips host
> - ppc guest on mips host
> 
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/virtio-net.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ec1bf8d..515fb19 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -81,7 +81,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = to_virtio_net(vdev);
>      struct virtio_net_config netcfg;
>  
> -    netcfg.status = n->status;
> +    netcfg.status = lduw_p(&n->status);
>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>      memcpy(config, &netcfg, sizeof(netcfg));
>  }
> @@ -340,7 +340,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      n->mac_table.multi_overflow = 0;
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
>  
>      if (sizeof(mac_data.entries) +
>          (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> @@ -356,7 +356,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  
>      n->mac_table.first_multi = n->mac_table.in_use;
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
>  
>      if (sizeof(mac_data.entries) +
>          (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
> @@ -386,7 +386,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
>      }
>  
> -    vid = lduw_le_p(elem->out_sg[1].iov_base);
> +    vid = lduw_p(elem->out_sg[1].iov_base);
>  
>      if (vid >= MAX_VLAN)
>          return VIRTIO_NET_ERR;
> @@ -675,8 +675,9 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>          virtqueue_fill(n->rx_vq, &elem, total, i++);
>      }
>  
> -    if (mhdr)
> -        mhdr->num_buffers = i;
> +    if (mhdr) {
> +        mhdr->num_buffers = lduw_p(&i);
> +    }
>  
>      virtqueue_flush(n->rx_vq, i);
>      virtio_notify(&n->vdev, n->rx_vq);
> -- 
> 1.7.2.3
> 

Ping?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

RE: [Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Liu Yu-B13201]

 

> -----Original Message-----
> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> On Behalf Of Aurelien Jarno
> Sent: Sunday, January 16, 2011 3:28 AM
> To: qemu-devel@nongnu.org
> Cc: Anthony Liguori; Aurelien Jarno
> Subject: [Qemu-devel] [PATCH 1/2] virtio-net: fix 
> cross-endianness support
> 
> virtio-net used to work on cross-endianness configurations, 
> but doesn't
> anymore with recent guest kernels, as the new features don't handle
> endianness correctly.
> 
> This patch fixes wrong conversion, and add missing ones to make
> virtio-net working. Tested on the following configurations:
> - i386 guest on x86_64 host
> - ppc guest on x86_64 host
> - i386 guest on mips host
> - ppc guest on mips host
> 
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/virtio-net.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ec1bf8d..515fb19 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -81,7 +81,7 @@ static void 
> virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = to_virtio_net(vdev);
>      struct virtio_net_config netcfg;
>  
> -    netcfg.status = n->status;
> +    netcfg.status = lduw_p(&n->status);
>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>      memcpy(config, &netcfg, sizeof(netcfg));
>  }
> @@ -340,7 +340,7 @@ static int 
> virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      n->mac_table.multi_overflow = 0;
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
>  
>      if (sizeof(mac_data.entries) +
>          (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> @@ -356,7 +356,7 @@ static int 
> virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  
>      n->mac_table.first_multi = n->mac_table.in_use;
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
>  

Hello Aurelien,

Not clear what is happen, but this commit break e500(PowerPC big endian) kvm.
Looks like e500 is happy to use ldl_le_p(), but this patch change it to use ldl_be_p().
Any thoughts about that?

Thanks,
Yu

Re: [Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Aurelien Jarno]

On Mon, Feb 28, 2011 at 08:43:41AM +0000, Liu Yu-B13201 wrote:
>  
> 
> > -----Original Message-----
> > From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> > [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> > On Behalf Of Aurelien Jarno
> > Sent: Sunday, January 16, 2011 3:28 AM
> > To: qemu-devel@nongnu.org
> > Cc: Anthony Liguori; Aurelien Jarno
> > Subject: [Qemu-devel] [PATCH 1/2] virtio-net: fix 
> > cross-endianness support
> > 
> > virtio-net used to work on cross-endianness configurations, 
> > but doesn't
> > anymore with recent guest kernels, as the new features don't handle
> > endianness correctly.
> > 
> > This patch fixes wrong conversion, and add missing ones to make
> > virtio-net working. Tested on the following configurations:
> > - i386 guest on x86_64 host
> > - ppc guest on x86_64 host
> > - i386 guest on mips host
> > - ppc guest on mips host
> > 
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  hw/virtio-net.c |   13 +++++++------
> >  1 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index ec1bf8d..515fb19 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -81,7 +81,7 @@ static void 
> > virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >      VirtIONet *n = to_virtio_net(vdev);
> >      struct virtio_net_config netcfg;
> >  
> > -    netcfg.status = n->status;
> > +    netcfg.status = lduw_p(&n->status);
> >      memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >      memcpy(config, &netcfg, sizeof(netcfg));
> >  }
> > @@ -340,7 +340,7 @@ static int 
> > virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >      n->mac_table.multi_overflow = 0;
> >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> >  
> > -    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
> > +    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
> >  
> >      if (sizeof(mac_data.entries) +
> >          (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> > @@ -356,7 +356,7 @@ static int 
> > virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  
> >      n->mac_table.first_multi = n->mac_table.in_use;
> >  
> > -    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
> > +    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
> >  
> 
> Hello Aurelien,

Hi,

> Not clear what is happen, but this commit break e500(PowerPC big endian) kvm.
> Looks like e500 is happy to use ldl_le_p(), but this patch change it to use ldl_be_p().

I am sorry about that. I have just found I have done my tests on the big
endian host using and old kernel, so which doesn't use this part of the
code.

> Ainy thoughts about that?
> 

My patch basically fixed the big endian guest on little endian host and 
broke the big endian guest on big endian host support. 

The endianness handling in virtio-net is different than in virtio-blk.
In virtio-blk we basically have to byteswap the data when the endianness
of the guest is different than the one of the guest. If I am correct, in
virtio-net it seems we have to byteswap the data solely depending on the 
guest endianness and independently on the host endianness. This is
something quite weird that we don't have in cpu-all.h

I am currently trying to write a patch, will come back to you when it's
ready.

Aurelien

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Stefan Hajnoczi]

On Thu, Mar 3, 2011 at 7:16 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> The endianness handling in virtio-net is different than in virtio-blk.
> In virtio-blk we basically have to byteswap the data when the endianness
> of the guest is different than the one of the guest. If I am correct, in
> virtio-net it seems we have to byteswap the data solely depending on the
> guest endianness and independently on the host endianness. This is
> something quite weird that we don't have in cpu-all.h

That sounds odd.  Virtio is guest endian so the host must do
guest<->host endian conversion.  Which bits of code make you think
otherwise?

Stefan

Re: [Qemu-devel] [PATCH 1/2] virtio-net: fix cross-endianness support

[Aurelien Jarno]

On Thu, Mar 03, 2011 at 07:50:03PM +0000, Stefan Hajnoczi wrote:
> On Thu, Mar 3, 2011 at 7:16 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > The endianness handling in virtio-net is different than in virtio-blk.
> > In virtio-blk we basically have to byteswap the data when the endianness
> > of the guest is different than the one of the guest. If I am correct, in
> > virtio-net it seems we have to byteswap the data solely depending on the
> > guest endianness and independently on the host endianness. This is
> > something quite weird that we don't have in cpu-all.h
> 
> That sounds odd.  Virtio is guest endian so the host must do
> guest<->host endian conversion.  Which bits of code make you think
> otherwise?
> 

That's exactly what my patch was doing, changing the ldx_le_p() into 
ldx_p(). In pratice it doesn't work.

Basically, with ldlx_le_p(), it only works when the endianness of the
guess and of the host are the same. With ldx_p() it only works when the
host is little endian. That's how I conclude that byteswapping should
only be done on big endian guests.

I agree it's weird and should be validated by some real tests, I am 
planning to do that soon.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH] virtio-net: Fix lduw_p() pointer argument of wrong size

[Stefan Hajnoczi]

A pointer to a size_t variable was passed as the void * pointer to
lduw_p() in virtio_net_receive().  Instead of acting on the 16-bit value
this caused failure on big-endian hosts.

Avoid this issue in the future by using stw_p() instead.  In general we
should use ld*_p() for loading from target memory and st*_p() for
storing to target memory anyway, not the other way around.

Also tighten up a correct use of lduw_p() when stw_p() should be used
instead in virtio_net_get_config().

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/virtio-net.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 20cf680..5962298 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -79,7 +79,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = to_virtio_net(vdev);
     struct virtio_net_config netcfg;
 
-    netcfg.status = lduw_p(&n->status);
+    stw_p(&netcfg.status, n->status);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, sizeof(netcfg));
 }
@@ -679,7 +679,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
     }
 
     if (mhdr) {
-        mhdr->num_buffers = lduw_p(&i);
+        stw_p(&mhdr->num_buffers, i);
     }
 
     virtqueue_flush(n->rx_vq, i);
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH] virtio-net: Fix lduw_p() pointer argument of wrong size

[Aurelien Jarno]

On Thu, Mar 03, 2011 at 09:42:28PM +0000, Stefan Hajnoczi wrote:
> A pointer to a size_t variable was passed as the void * pointer to
> lduw_p() in virtio_net_receive().  Instead of acting on the 16-bit value
> this caused failure on big-endian hosts.
> 
> Avoid this issue in the future by using stw_p() instead.  In general we
> should use ld*_p() for loading from target memory and st*_p() for
> storing to target memory anyway, not the other way around.
> 
> Also tighten up a correct use of lduw_p() when stw_p() should be used
> instead in virtio_net_get_config().
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  hw/virtio-net.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Thanks a lot for the quick patch, I confirm it fixes big endian support.
I have applied it, also to the stable branch.

> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 20cf680..5962298 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -79,7 +79,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = to_virtio_net(vdev);
>      struct virtio_net_config netcfg;
>  
> -    netcfg.status = lduw_p(&n->status);
> +    stw_p(&netcfg.status, n->status);
>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>      memcpy(config, &netcfg, sizeof(netcfg));
>  }
> @@ -679,7 +679,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>      }
>  
>      if (mhdr) {
> -        mhdr->num_buffers = lduw_p(&i);
> +        stw_p(&mhdr->num_buffers, i);
>      }
>  
>      virtqueue_flush(n->rx_vq, i);
> -- 
> 1.7.2.3
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] RE: [PATCH] virtio-net: Fix lduw_p() pointer argument of wrong size

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Aurelien Jarno [mailto:aurelien@aurel32.net] 
> Sent: Friday, March 04, 2011 6:46 AM
> To: Stefan Hajnoczi
> Cc: Liu Yu-B13201; Anthony Liguori; qemu-devel@nongnu.org
> Subject: Re: [PATCH] virtio-net: Fix lduw_p() pointer 
> argument of wrong size
> 
> On Thu, Mar 03, 2011 at 09:42:28PM +0000, Stefan Hajnoczi wrote:
> > A pointer to a size_t variable was passed as the void * pointer to
> > lduw_p() in virtio_net_receive().  Instead of acting on the 
> 16-bit value
> > this caused failure on big-endian hosts.
> > 
> > Avoid this issue in the future by using stw_p() instead.  
> In general we
> > should use ld*_p() for loading from target memory and st*_p() for
> > storing to target memory anyway, not the other way around.
> > 
> > Also tighten up a correct use of lduw_p() when stw_p() 
> should be used
> > instead in virtio_net_get_config().
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > ---
> >  hw/virtio-net.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Thanks a lot for the quick patch, I confirm it fixes big 
> endian support.
> I have applied it, also to the stable branch.
> 

Thank you both very much. This solved my issue.

Thanks,
Yu