Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[Gerd Hoffmann <kraxel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

On 03/08/12 01:41, David Gibson wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> -    assert(s->csw.sig == 0x53425355);
> +    assert(s->csw.sig == cpu_to_le32(0x53425355));

Correct but incomplete.  residue is sent in host endian instead of
little endian too.  It is zero most of the time though, so it easily
goes unnoticed.

Can you try the attached patch instead?

cheers,
  Gerd


[-- Attachment #2: 0001-usb-storage-fix-byteorder-issues.patch --]
[-- Type: text/plain, Size: 5473 bytes --]

>From a6ac421d0aa1bcfd42601a655e4995c67f6c903c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 9 Mar 2012 13:19:28 +0100
Subject: [PATCH] usb-storage: fix byteorder issues.

Store the command word in MSDStatus.  Add the function
usb_msd_recv_command to copy the command, simliar to
usb_msd_send_status which copyes the status.

Do all byteswapping in usb_msd_recv_command and usb_msd_send_status
functions where the copying from/to the usb packets happens, so all
other places can just use native endian when accessing command and
status packet data.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-storage.c |   76 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 6ffaf70..7da6205 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -45,6 +45,16 @@ struct usb_msd_csw {
     uint8_t status;
 };
 
+struct usb_msd_cbw {
+    uint32_t sig;
+    uint32_t tag;
+    uint32_t data_len;
+    uint8_t flags;
+    uint8_t lun;
+    uint8_t cmd_len;
+    uint8_t cmd[16];
+};
+
 typedef struct {
     USBDevice dev;
     enum USBMSDMode mode;
@@ -52,6 +62,7 @@ typedef struct {
     uint8_t *scsi_buf;
     uint32_t data_len;
     uint32_t residue;
+    struct usb_msd_cbw cbw;
     struct usb_msd_csw csw;
     SCSIRequest *req;
     SCSIBus bus;
@@ -63,16 +74,6 @@ typedef struct {
     USBPacket *packet;
 } MSDState;
 
-struct usb_msd_cbw {
-    uint32_t sig;
-    uint32_t tag;
-    uint32_t data_len;
-    uint8_t flags;
-    uint8_t lun;
-    uint8_t cmd_len;
-    uint8_t cmd[16];
-};
-
 enum {
     STR_MANUFACTURER = 1,
     STR_PRODUCT,
@@ -188,6 +189,26 @@ static void usb_msd_copy_data(MSDState *s, USBPacket *p)
     }
 }
 
+static int usb_msd_recv_command(MSDState *s, USBPacket *p)
+{
+    if (p->iov.size != 31) {
+        fprintf(stderr, "usb-msd: Bad CBW size");
+        return -1;
+    }
+
+    usb_packet_copy(p, &s->cbw, 31);
+    s->cbw.sig = le32_to_cpu(s->cbw.sig);
+    s->cbw.tag = le32_to_cpu(s->cbw.tag);
+    s->cbw.data_len = le32_to_cpu(s->cbw.data_len);
+
+    if (s->cbw.sig != 0x43425355) {
+        fprintf(stderr, "usb-msd: Bad signature %08x\n",
+                s->cbw.sig);
+        return -1;
+    }
+    return 0;
+}
+
 static void usb_msd_send_status(MSDState *s, USBPacket *p)
 {
     int len;
@@ -197,7 +218,12 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
 
     assert(s->csw.sig == 0x53425355);
     len = MIN(sizeof(s->csw), p->iov.size);
+
+    s->csw.sig = cpu_to_le32(s->csw.sig);
+    s->csw.tag = cpu_to_le32(s->csw.tag);
+    s->csw.residue = cpu_to_le32(s->csw.residue);
     usb_packet_copy(p, &s->csw, len);
+
     memset(&s->csw, 0, sizeof(s->csw));
 }
 
@@ -231,8 +257,8 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status, size_t r
     DPRINTF("Command complete %d tag 0x%x\n", status, req->tag);
     s->residue = s->data_len;
 
-    s->csw.sig = cpu_to_le32(0x53425355);
-    s->csw.tag = cpu_to_le32(req->tag);
+    s->csw.sig = 0x53425355;
+    s->csw.tag = req->tag;
     s->csw.residue = s->residue;
     s->csw.status = status != 0;
 
@@ -338,9 +364,7 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
 static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
 {
     MSDState *s = (MSDState *)dev;
-    uint32_t tag;
     int ret = 0;
-    struct usb_msd_cbw cbw;
     uint8_t devep = p->ep->nr;
 
     switch (p->pid) {
@@ -350,35 +374,27 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
 
         switch (s->mode) {
         case USB_MSDM_CBW:
-            if (p->iov.size != 31) {
-                fprintf(stderr, "usb-msd: Bad CBW size");
-                goto fail;
-            }
-            usb_packet_copy(p, &cbw, 31);
-            if (le32_to_cpu(cbw.sig) != 0x43425355) {
-                fprintf(stderr, "usb-msd: Bad signature %08x\n",
-                        le32_to_cpu(cbw.sig));
+            if (usb_msd_recv_command(s, p) != 0) {
                 goto fail;
             }
-            DPRINTF("Command on LUN %d\n", cbw.lun);
-            if (cbw.lun != 0) {
-                fprintf(stderr, "usb-msd: Bad LUN %d\n", cbw.lun);
+            DPRINTF("Command on LUN %d\n", s->cbw.lun);
+            if (s->cbw.lun != 0) {
+                fprintf(stderr, "usb-msd: Bad LUN %d\n", s->cbw.lun);
                 goto fail;
             }
-            tag = le32_to_cpu(cbw.tag);
-            s->data_len = le32_to_cpu(cbw.data_len);
+            s->data_len = s->cbw.data_len;
             if (s->data_len == 0) {
                 s->mode = USB_MSDM_CSW;
-            } else if (cbw.flags & 0x80) {
+            } else if (s->cbw.flags & 0x80) {
                 s->mode = USB_MSDM_DATAIN;
             } else {
                 s->mode = USB_MSDM_DATAOUT;
             }
             DPRINTF("Command tag 0x%x flags %08x len %d data %d\n",
-                    tag, cbw.flags, cbw.cmd_len, s->data_len);
+                    tag, s->cbw.flags, s->cbw.cmd_len, s->data_len);
             s->residue = 0;
             s->scsi_len = 0;
-            s->req = scsi_req_new(s->scsi_dev, tag, 0, cbw.cmd, NULL);
+            s->req = scsi_req_new(s->scsi_dev, s->cbw.tag, 0, s->cbw.cmd, NULL);
             scsi_req_enqueue(s->req);
             if (s->req && s->req->cmd.xfer != SCSI_XFER_NONE) {
                 scsi_req_continue(s->req);
-- 
1.7.1