* [PATCH v2 01/10] usb/msd: Split in and out packet handling
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
` (8 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
Split in and out packet handling int otheir own functions, to make
them a bit more managable.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 266 +++++++++++++++++++++++--------------------
1 file changed, 145 insertions(+), 121 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4f1e8b7f6cb..2d7306b0572 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -395,158 +395,182 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
}
}
-static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
+static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
{
MSDState *s = (MSDState *)dev;
uint32_t tag;
struct usb_msd_cbw cbw;
- uint8_t devep = p->ep->nr;
SCSIDevice *scsi_dev;
int len;
- if (s->needs_reset) {
- p->status = USB_RET_STALL;
- return;
- }
+ switch (s->mode) {
+ case USB_MSDM_CBW:
+ if (p->iov.size != 31) {
+ error_report("usb-msd: Bad CBW size");
+ goto fail;
+ }
+ usb_packet_copy(p, &cbw, 31);
+ if (le32_to_cpu(cbw.sig) != 0x43425355) {
+ error_report("usb-msd: Bad signature %08x",
+ le32_to_cpu(cbw.sig));
+ goto fail;
+ }
+ scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
+ if (scsi_dev == NULL) {
+ error_report("usb-msd: Bad LUN %d", cbw.lun);
+ goto fail;
+ }
+ tag = le32_to_cpu(cbw.tag);
+ s->data_len = le32_to_cpu(cbw.data_len);
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ } else if (cbw.flags & 0x80) {
+ s->mode = USB_MSDM_DATAIN;
+ } else {
+ s->mode = USB_MSDM_DATAOUT;
+ }
+ trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
+ cbw.cmd_len, s->data_len);
+ assert(le32_to_cpu(s->csw.residue) == 0);
+ s->scsi_len = 0;
+ s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
+ cbw.cmd, cbw.cmd_len, NULL);
+ if (s->commandlog) {
+ scsi_req_print(s->req);
+ }
+ len = scsi_req_enqueue(s->req);
+ if (len) {
+ scsi_req_continue(s->req);
+ }
+ break;
- switch (p->pid) {
- case USB_TOKEN_OUT:
- if (devep != 2)
+ case USB_MSDM_DATAOUT:
+ trace_usb_msd_data_out(p->iov.size, s->data_len);
+ if (p->iov.size > s->data_len) {
goto fail;
+ }
- switch (s->mode) {
- case USB_MSDM_CBW:
- if (p->iov.size != 31) {
- error_report("usb-msd: Bad CBW size");
- goto fail;
- }
- usb_packet_copy(p, &cbw, 31);
- if (le32_to_cpu(cbw.sig) != 0x43425355) {
- error_report("usb-msd: Bad signature %08x",
- le32_to_cpu(cbw.sig));
- goto fail;
- }
- scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
- if (scsi_dev == NULL) {
- error_report("usb-msd: Bad LUN %d", cbw.lun);
- goto fail;
- }
- tag = le32_to_cpu(cbw.tag);
- s->data_len = le32_to_cpu(cbw.data_len);
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- } else if (cbw.flags & 0x80) {
- s->mode = USB_MSDM_DATAIN;
- } else {
- s->mode = USB_MSDM_DATAOUT;
- }
- trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
- cbw.cmd_len, s->data_len);
- assert(le32_to_cpu(s->csw.residue) == 0);
- s->scsi_len = 0;
- s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, cbw.cmd_len, NULL);
- if (s->commandlog) {
- scsi_req_print(s->req);
- }
- len = scsi_req_enqueue(s->req);
+ if (s->scsi_len) {
+ usb_msd_copy_data(s, p);
+ }
+ if (le32_to_cpu(s->csw.residue)) {
+ len = p->iov.size - p->actual_length;
if (len) {
- scsi_req_continue(s->req);
+ usb_packet_skip(p, len);
+ if (len > s->data_len) {
+ len = s->data_len;
+ }
+ s->data_len -= len;
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ }
}
- break;
+ }
+ if (p->actual_length < p->iov.size) {
+ trace_usb_msd_packet_async();
+ s->packet = p;
+ p->status = USB_RET_ASYNC;
+ }
+ break;
- case USB_MSDM_DATAOUT:
- trace_usb_msd_data_out(p->iov.size, s->data_len);
- if (p->iov.size > s->data_len) {
- goto fail;
- }
+ default:
+ goto fail;
+ }
+ return;
- if (s->scsi_len) {
- usb_msd_copy_data(s, p);
- }
- if (le32_to_cpu(s->csw.residue)) {
- len = p->iov.size - p->actual_length;
- if (len) {
- usb_packet_skip(p, len);
- if (len > s->data_len) {
- len = s->data_len;
- }
- s->data_len -= len;
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- }
- }
- }
- if (p->actual_length < p->iov.size) {
- trace_usb_msd_packet_async();
- s->packet = p;
- p->status = USB_RET_ASYNC;
- }
- break;
+fail:
+ p->status = USB_RET_STALL;
+}
- default:
+static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
+{
+ MSDState *s = (MSDState *)dev;
+ int len;
+
+ switch (s->mode) {
+ case USB_MSDM_DATAOUT:
+ if (s->data_len != 0 || p->iov.size < 13) {
goto fail;
}
+ /* Waiting for SCSI write to complete. */
+ trace_usb_msd_packet_async();
+ s->packet = p;
+ p->status = USB_RET_ASYNC;
break;
- case USB_TOKEN_IN:
- if (devep != 1)
+ case USB_MSDM_CSW:
+ if (p->iov.size < 13) {
goto fail;
+ }
- switch (s->mode) {
- case USB_MSDM_DATAOUT:
- if (s->data_len != 0 || p->iov.size < 13) {
- goto fail;
- }
- /* Waiting for SCSI write to complete. */
+ if (s->req) {
+ /* still in flight */
trace_usb_msd_packet_async();
s->packet = p;
p->status = USB_RET_ASYNC;
- break;
+ } else {
+ usb_msd_send_status(s, p);
+ s->mode = USB_MSDM_CBW;
+ }
+ break;
- case USB_MSDM_CSW:
- if (p->iov.size < 13) {
- goto fail;
+ case USB_MSDM_DATAIN:
+ trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
+ if (s->scsi_len) {
+ usb_msd_copy_data(s, p);
+ }
+ if (le32_to_cpu(s->csw.residue)) {
+ len = p->iov.size - p->actual_length;
+ if (len) {
+ usb_packet_skip(p, len);
+ if (len > s->data_len) {
+ len = s->data_len;
+ }
+ s->data_len -= len;
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ }
}
+ }
+ if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
+ trace_usb_msd_packet_async();
+ s->packet = p;
+ p->status = USB_RET_ASYNC;
+ }
+ break;
- if (s->req) {
- /* still in flight */
- trace_usb_msd_packet_async();
- s->packet = p;
- p->status = USB_RET_ASYNC;
- } else {
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- }
- break;
+ default:
+ goto fail;
+ }
+ return;
- case USB_MSDM_DATAIN:
- trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
- if (s->scsi_len) {
- usb_msd_copy_data(s, p);
- }
- if (le32_to_cpu(s->csw.residue)) {
- len = p->iov.size - p->actual_length;
- if (len) {
- usb_packet_skip(p, len);
- if (len > s->data_len) {
- len = s->data_len;
- }
- s->data_len -= len;
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- }
- }
- }
- if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
- trace_usb_msd_packet_async();
- s->packet = p;
- p->status = USB_RET_ASYNC;
- }
- break;
+fail:
+ p->status = USB_RET_STALL;
+}
+
+static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
+{
+ MSDState *s = (MSDState *)dev;
+ uint8_t devep = p->ep->nr;
- default:
+ if (s->needs_reset) {
+ p->status = USB_RET_STALL;
+ return;
+ }
+
+ switch (p->pid) {
+ case USB_TOKEN_OUT:
+ if (devep != 2) {
+ goto fail;
+ }
+ usb_msd_handle_data_out(dev, p);
+ break;
+
+ case USB_TOKEN_IN:
+ if (devep != 1) {
goto fail;
}
+ usb_msd_handle_data_in(dev, p);
break;
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 01/10] usb/msd: Split in and out packet handling Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 10:18 ` Philippe Mathieu-Daudé
2025-04-11 10:21 ` Philippe Mathieu-Daudé
2025-04-11 8:04 ` [PATCH v2 03/10] usb/msd: Improved handling of mass storage reset Nicholas Piggin
` (7 subsequent siblings)
9 siblings, 2 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
These structures are hardware interfaces, ensure the layout is
correct.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 2d7306b0572..87c22476f6b 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -27,7 +27,7 @@
#define MassStorageReset 0xff
#define GetMaxLun 0xfe
-struct usb_msd_cbw {
+struct QEMU_PACKED usb_msd_cbw {
uint32_t sig;
uint32_t tag;
uint32_t data_len;
@@ -636,6 +636,9 @@ static const TypeInfo usb_storage_dev_type_info = {
static void usb_msd_register_types(void)
{
+ qemu_build_assert(sizeof(struct usb_msd_cbw) == 31);
+ qemu_build_assert(sizeof(struct usb_msd_csw) == 13);
+
type_register_static(&usb_storage_dev_type_info);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct
2025-04-11 8:04 ` [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
@ 2025-04-11 10:18 ` Philippe Mathieu-Daudé
2025-04-11 10:21 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:18 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 10:04, Nicholas Piggin wrote:
> These structures are hardware interfaces, ensure the layout is
> correct.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/usb/dev-storage.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct
2025-04-11 8:04 ` [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
2025-04-11 10:18 ` Philippe Mathieu-Daudé
@ 2025-04-11 10:21 ` Philippe Mathieu-Daudé
2025-04-11 10:23 ` Philippe Mathieu-Daudé
2025-04-12 5:32 ` Nicholas Piggin
1 sibling, 2 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:21 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 10:04, Nicholas Piggin wrote:
> These structures are hardware interfaces, ensure the layout is
> correct.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/usb/dev-storage.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 2d7306b0572..87c22476f6b 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -27,7 +27,7 @@
> #define MassStorageReset 0xff
> #define GetMaxLun 0xfe
>
> -struct usb_msd_cbw {
> +struct QEMU_PACKED usb_msd_cbw {
> uint32_t sig;
> uint32_t tag;
> uint32_t data_len;
> @@ -636,6 +636,9 @@ static const TypeInfo usb_storage_dev_type_info = {
>
> static void usb_msd_register_types(void)
> {
> + qemu_build_assert(sizeof(struct usb_msd_cbw) == 31);
> + qemu_build_assert(sizeof(struct usb_msd_csw) == 13);
Can we add definitions for these 13/31 magic values? Then
we can use them in try_get_valid_cbw().
> +
> type_register_static(&usb_storage_dev_type_info);
> }
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct
2025-04-11 10:21 ` Philippe Mathieu-Daudé
@ 2025-04-11 10:23 ` Philippe Mathieu-Daudé
2025-04-12 5:32 ` Nicholas Piggin
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:23 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 12:21, Philippe Mathieu-Daudé wrote:
> On 11/4/25 10:04, Nicholas Piggin wrote:
>> These structures are hardware interfaces, ensure the layout is
>> correct.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> hw/usb/dev-storage.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
>> index 2d7306b0572..87c22476f6b 100644
>> --- a/hw/usb/dev-storage.c
>> +++ b/hw/usb/dev-storage.c
>> @@ -27,7 +27,7 @@
>> #define MassStorageReset 0xff
>> #define GetMaxLun 0xfe
>> -struct usb_msd_cbw {
>> +struct QEMU_PACKED usb_msd_cbw {
>> uint32_t sig;
>> uint32_t tag;
>> uint32_t data_len;
>> @@ -636,6 +636,9 @@ static const TypeInfo usb_storage_dev_type_info = {
>> static void usb_msd_register_types(void)
>> {
>> + qemu_build_assert(sizeof(struct usb_msd_cbw) == 31);
>> + qemu_build_assert(sizeof(struct usb_msd_csw) == 13);
>
> Can we add definitions for these 13/31 magic values? Then
> we can use them in try_get_valid_cbw().
Maybe USB_MSD_CBW/CSW_MIN_SIZE?
>
>> +
>> type_register_static(&usb_storage_dev_type_info);
>> }
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct
2025-04-11 10:21 ` Philippe Mathieu-Daudé
2025-04-11 10:23 ` Philippe Mathieu-Daudé
@ 2025-04-12 5:32 ` Nicholas Piggin
1 sibling, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-12 5:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On Fri Apr 11, 2025 at 8:21 PM AEST, Philippe Mathieu-Daudé wrote:
> On 11/4/25 10:04, Nicholas Piggin wrote:
>> These structures are hardware interfaces, ensure the layout is
>> correct.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> hw/usb/dev-storage.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
>> index 2d7306b0572..87c22476f6b 100644
>> --- a/hw/usb/dev-storage.c
>> +++ b/hw/usb/dev-storage.c
>> @@ -27,7 +27,7 @@
>> #define MassStorageReset 0xff
>> #define GetMaxLun 0xfe
>>
>> -struct usb_msd_cbw {
>> +struct QEMU_PACKED usb_msd_cbw {
>> uint32_t sig;
>> uint32_t tag;
>> uint32_t data_len;
>> @@ -636,6 +636,9 @@ static const TypeInfo usb_storage_dev_type_info = {
>>
>> static void usb_msd_register_types(void)
>> {
>> + qemu_build_assert(sizeof(struct usb_msd_cbw) == 31);
>> + qemu_build_assert(sizeof(struct usb_msd_csw) == 13);
>
> Can we add definitions for these 13/31 magic values? Then
> we can use them in try_get_valid_cbw().
Good idea, I've done something to improve them.
Thanks,
Nick
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 03/10] usb/msd: Improved handling of mass storage reset
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 01/10] usb/msd: Split in and out packet handling Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 10:19 ` Philippe Mathieu-Daudé
2025-04-11 8:04 ` [PATCH v2 04/10] usb/msd: Improve packet validation error logging Nicholas Piggin
` (6 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
The mass storage reset request handling does not reset in-flight
SCSI requests or USB MSD packets. Implement this by calling the
device reset handler which should take care of everything.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 87c22476f6b..c7c36ac80fa 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -359,7 +359,7 @@ static void usb_msd_handle_control(USBDevice *dev, USBPacket *p,
/* Class specific requests. */
case ClassInterfaceOutRequest | MassStorageReset:
/* Reset state ready for the next CBW. */
- s->mode = USB_MSDM_CBW;
+ usb_msd_handle_reset(dev);
break;
case ClassInterfaceRequest | GetMaxLun:
maxlun = 0;
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 04/10] usb/msd: Improve packet validation error logging
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (2 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 03/10] usb/msd: Improved handling of mass storage reset Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 05/10] usb/msd: Allow CBW packet size greater than 31 Nicholas Piggin
` (5 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
Errors in incoming USB MSD packet format or context would typically
be guest software errors. Log these under guest errors.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 53 +++++++++++++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 11 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index c7c36ac80fa..6668114ea74 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
+#include "qemu/log.h"
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/config-file.h"
@@ -395,6 +396,36 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
}
}
+static bool try_get_valid_cbw(USBPacket *p, struct usb_msd_cbw *cbw)
+{
+ uint32_t sig;
+
+ if (p->iov.size != 31) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW size %zu\n",
+ p->iov.size);
+ return false;
+ }
+ usb_packet_copy(p, cbw, 31);
+ sig = le32_to_cpu(cbw->sig);
+ if (sig != 0x43425355) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW signature 0x%08x\n",
+ sig);
+ return false;
+ }
+
+ return true;
+}
+
+static bool check_valid_csw(USBPacket *p)
+{
+ if (p->iov.size < 13) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CSW size %zu\n",
+ p->iov.size);
+ return false;
+ }
+ return true;
+}
+
static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
{
MSDState *s = (MSDState *)dev;
@@ -405,19 +436,13 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
switch (s->mode) {
case USB_MSDM_CBW:
- if (p->iov.size != 31) {
- error_report("usb-msd: Bad CBW size");
- goto fail;
- }
- usb_packet_copy(p, &cbw, 31);
- if (le32_to_cpu(cbw.sig) != 0x43425355) {
- error_report("usb-msd: Bad signature %08x",
- le32_to_cpu(cbw.sig));
+ if (!try_get_valid_cbw(p, &cbw)) {
goto fail;
}
scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
if (scsi_dev == NULL) {
- error_report("usb-msd: Bad LUN %d", cbw.lun);
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW LUN %d\n",
+ cbw.lun);
goto fail;
}
tag = le32_to_cpu(cbw.tag);
@@ -489,9 +514,15 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
switch (s->mode) {
case USB_MSDM_DATAOUT:
- if (s->data_len != 0 || p->iov.size < 13) {
+ if (!check_valid_csw(p)) {
+ goto fail;
+ }
+ if (s->data_len != 0) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: CSW received before "
+ "all data was sent\n");
goto fail;
}
+
/* Waiting for SCSI write to complete. */
trace_usb_msd_packet_async();
s->packet = p;
@@ -499,7 +530,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSDM_CSW:
- if (p->iov.size < 13) {
+ if (!check_valid_csw(p)) {
goto fail;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 05/10] usb/msd: Allow CBW packet size greater than 31
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (3 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 04/10] usb/msd: Improve packet validation error logging Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 06/10] usb/msd: Split async packet tracking into data and csw Nicholas Piggin
` (4 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
The CBW structure is 31 bytes, so CBW DATAOUT packets must be at least
31 bytes. QEMU enforces exactly 31 bytes, but this is inconsistent with
how it handles CSW packets (where it allows greater than or equal to 13
bytes) despite wording in the spec[*] being similar for both packet
types: "shall end as a short packet with exactly 31 bytes transferred".
[*] USB MSD Bulk-Only Transport 1.0
For consistency, and on the principle of being tolerant in accepting
input, relax the CBW size check.
Alternatively, both checks could be tightened to exact. Or a message
could be printed warning of possible guest error if size is not exact,
but still accept the packets.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 6668114ea74..27093de5c84 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -400,7 +400,7 @@ static bool try_get_valid_cbw(USBPacket *p, struct usb_msd_cbw *cbw)
{
uint32_t sig;
- if (p->iov.size != 31) {
+ if (p->iov.size < 31) {
qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW size %zu\n",
p->iov.size);
return false;
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 06/10] usb/msd: Split async packet tracking into data and csw
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (4 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 05/10] usb/msd: Allow CBW packet size greater than 31 Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 07/10] usb/msd: Add some additional assertions Nicholas Piggin
` (3 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
The async packet handling logic has places that infer whether the
async packet is data or CSW, based on context. This is not wrong,
it just makes the logic easier to follow if they are categorised
when they are accepted.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 5 +-
hw/usb/dev-storage.c | 121 +++++++++++++++++++++++++++----------------
2 files changed, 79 insertions(+), 47 deletions(-)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index f9fd862b529..a40d15f5def 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -33,8 +33,11 @@ struct MSDState {
struct usb_msd_csw csw;
SCSIRequest *req;
SCSIBus bus;
+
/* For async completion. */
- USBPacket *packet;
+ USBPacket *data_packet;
+ USBPacket *csw_in_packet;
+
/* usb-storage only */
BlockConf conf;
bool removable;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 27093de5c84..a9d8d4e8618 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -178,18 +178,33 @@ static const USBDesc desc = {
.str = desc_strings,
};
-static void usb_msd_packet_complete(MSDState *s, int status)
+static void usb_msd_data_packet_complete(MSDState *s, int status)
{
- USBPacket *p = s->packet;
+ USBPacket *p = s->data_packet;
/*
- * Set s->packet to NULL before calling usb_packet_complete
- * because another request may be issued before
- * usb_packet_complete returns.
+ * Set s->data_packet to NULL before calling usb_packet_complete
+ * because another request may be issued before usb_packet_complete
+ * returns.
*/
trace_usb_msd_packet_complete();
+ s->data_packet = NULL;
+ p->status = status;
+ usb_packet_complete(&s->dev, p);
+}
+
+static void usb_msd_csw_packet_complete(MSDState *s, int status)
+{
+ USBPacket *p = s->csw_in_packet;
+
+ /*
+ * Set s->csw_in_packet to NULL before calling usb_packet_complete
+ * because another request may be issued before usb_packet_complete
+ * returns.
+ */
+ trace_usb_msd_packet_complete();
+ s->csw_in_packet = NULL;
p->status = status;
- s->packet = NULL;
usb_packet_complete(&s->dev, p);
}
@@ -197,8 +212,12 @@ static void usb_msd_fatal_error(MSDState *s)
{
trace_usb_msd_fatal_error();
- if (s->packet) {
- usb_msd_packet_complete(s, USB_RET_STALL);
+ if (s->data_packet) {
+ usb_msd_data_packet_complete(s, USB_RET_STALL);
+ }
+
+ if (s->csw_in_packet) {
+ usb_msd_csw_packet_complete(s, USB_RET_STALL);
}
/*
@@ -243,7 +262,7 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
- USBPacket *p = s->packet;
+ USBPacket *p = s->data_packet;
if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
usb_msd_fatal_error(s);
@@ -254,10 +273,10 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
s->scsi_off = 0;
if (p) {
usb_msd_copy_data(s, p);
- p = s->packet;
+ p = s->data_packet;
if (p && p->actual_length == p->iov.size) {
/* USB_RET_SUCCESS status clears previous ASYNC status */
- usb_msd_packet_complete(s, USB_RET_SUCCESS);
+ usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
}
}
}
@@ -265,7 +284,7 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
void usb_msd_command_complete(SCSIRequest *req, size_t resid)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
- USBPacket *p = s->packet;
+ USBPacket *p = s->data_packet;
trace_usb_msd_cmd_complete(req->status, req->tag);
@@ -274,35 +293,37 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->csw.residue = cpu_to_le32(s->data_len);
s->csw.status = req->status != 0;
- if (s->packet) {
- if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
- /* A deferred packet with no write data remaining must be
- the status read packet. */
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- } else if (s->mode == USB_MSDM_CSW) {
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- } else {
- if (s->data_len) {
- int len = (p->iov.size - p->actual_length);
- usb_packet_skip(p, len);
- if (len > s->data_len) {
- len = s->data_len;
- }
- s->data_len -= len;
- }
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ scsi_req_unref(req);
+ s->req = NULL;
+
+ if (p) {
+ g_assert(s->mode == USB_MSDM_DATAIN || s->mode == USB_MSDM_DATAOUT);
+ if (s->data_len) {
+ int len = (p->iov.size - p->actual_length);
+ usb_packet_skip(p, len);
+ if (len > s->data_len) {
+ len = s->data_len;
}
+ s->data_len -= len;
+ }
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
}
/* USB_RET_SUCCESS status clears previous ASYNC status */
- usb_msd_packet_complete(s, USB_RET_SUCCESS);
+ usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
} else if (s->data_len == 0) {
s->mode = USB_MSDM_CSW;
}
- scsi_req_unref(req);
- s->req = NULL;
+
+ if (s->mode == USB_MSDM_CSW) {
+ p = s->csw_in_packet;
+ if (p) {
+ usb_msd_send_status(s, p);
+ s->mode = USB_MSDM_CBW;
+ /* USB_RET_SUCCESS status clears previous ASYNC status */
+ usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
+ }
+ }
}
void usb_msd_request_cancelled(SCSIRequest *req)
@@ -332,8 +353,12 @@ void usb_msd_handle_reset(USBDevice *dev)
}
assert(s->req == NULL);
- if (s->packet) {
- usb_msd_packet_complete(s, USB_RET_STALL);
+ if (s->data_packet) {
+ usb_msd_data_packet_complete(s, USB_RET_STALL);
+ }
+
+ if (s->csw_in_packet) {
+ usb_msd_csw_packet_complete(s, USB_RET_STALL);
}
memset(&s->csw, 0, sizeof(s->csw));
@@ -388,11 +413,15 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
{
MSDState *s = USB_STORAGE_DEV(dev);
- assert(s->packet == p);
- s->packet = NULL;
-
- if (s->req) {
- scsi_req_cancel(s->req);
+ if (p == s->data_packet) {
+ s->data_packet = NULL;
+ if (s->req) {
+ scsi_req_cancel(s->req);
+ }
+ } else if (p == s->csw_in_packet) {
+ s->csw_in_packet = NULL;
+ } else {
+ g_assert_not_reached();
}
}
@@ -493,7 +522,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
if (p->actual_length < p->iov.size) {
trace_usb_msd_packet_async();
- s->packet = p;
+ s->data_packet = p;
p->status = USB_RET_ASYNC;
}
break;
@@ -525,7 +554,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
/* Waiting for SCSI write to complete. */
trace_usb_msd_packet_async();
- s->packet = p;
+ s->csw_in_packet = p;
p->status = USB_RET_ASYNC;
break;
@@ -537,7 +566,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
if (s->req) {
/* still in flight */
trace_usb_msd_packet_async();
- s->packet = p;
+ s->csw_in_packet = p;
p->status = USB_RET_ASYNC;
} else {
usb_msd_send_status(s, p);
@@ -565,7 +594,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
}
if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
trace_usb_msd_packet_async();
- s->packet = p;
+ s->data_packet = p;
p->status = USB_RET_ASYNC;
}
break;
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 07/10] usb/msd: Add some additional assertions
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (5 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 06/10] usb/msd: Split async packet tracking into data and csw Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 10:27 ` Philippe Mathieu-Daudé
2025-04-11 8:04 ` [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names Nicholas Piggin
` (2 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
Add more assertions to help verify internal logic.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index a9d8d4e8618..3b806872587 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -264,13 +264,24 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->data_packet;
- if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
- usb_msd_fatal_error(s);
- return;
+ if (s->mode == USB_MSDM_DATAIN) {
+ if (req->cmd.mode == SCSI_XFER_TO_DEV) {
+ usb_msd_fatal_error(s);
+ return;
+ }
+ } else if (s->mode == USB_MSDM_DATAOUT) {
+ if (req->cmd.mode != SCSI_XFER_TO_DEV) {
+ usb_msd_fatal_error(s);
+ return;
+ }
+ } else {
+ g_assert_not_reached();
}
+ assert(s->scsi_len == 0);
s->scsi_len = len;
s->scsi_off = 0;
+
if (p) {
usb_msd_copy_data(s, p);
p = s->data_packet;
@@ -288,6 +299,10 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
trace_usb_msd_cmd_complete(req->status, req->tag);
+ g_assert(s->req);
+ /* The CBW is what starts the SCSI request */
+ g_assert(s->mode != USB_MSDM_CBW);
+
s->csw.sig = cpu_to_le32(0x53425355);
s->csw.tag = cpu_to_le32(req->tag);
s->csw.residue = cpu_to_le32(s->data_len);
@@ -486,7 +501,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
assert(le32_to_cpu(s->csw.residue) == 0);
- s->scsi_len = 0;
+ assert(s->scsi_len == 0);
s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
cbw.cmd, cbw.cmd_len, NULL);
if (s->commandlog) {
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 07/10] usb/msd: Add some additional assertions
2025-04-11 8:04 ` [PATCH v2 07/10] usb/msd: Add some additional assertions Nicholas Piggin
@ 2025-04-11 10:27 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:27 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 10:04, Nicholas Piggin wrote:
> Add more assertions to help verify internal logic.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/usb/dev-storage.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index a9d8d4e8618..3b806872587 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -264,13 +264,24 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
> MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
> USBPacket *p = s->data_packet;
>
> - if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
> - usb_msd_fatal_error(s);
> - return;
> + if (s->mode == USB_MSDM_DATAIN) {
Or switch().
> + if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> + usb_msd_fatal_error(s);
> + return;
> + }
> + } else if (s->mode == USB_MSDM_DATAOUT) {
> + if (req->cmd.mode != SCSI_XFER_TO_DEV) {
> + usb_msd_fatal_error(s);
> + return;
> + }
> + } else {
> + g_assert_not_reached();
> }
>
> + assert(s->scsi_len == 0);
> s->scsi_len = len;
> s->scsi_off = 0;
> +
> if (p) {
> usb_msd_copy_data(s, p);
> p = s->data_packet;
> @@ -288,6 +299,10 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
>
> trace_usb_msd_cmd_complete(req->status, req->tag);
>
> + g_assert(s->req);
> + /* The CBW is what starts the SCSI request */
> + g_assert(s->mode != USB_MSDM_CBW);
> +
> s->csw.sig = cpu_to_le32(0x53425355);
> s->csw.tag = cpu_to_le32(req->tag);
> s->csw.residue = cpu_to_le32(s->data_len);
> @@ -486,7 +501,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
> trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
> cbw.cmd_len, s->data_len);
> assert(le32_to_cpu(s->csw.residue) == 0);
> - s->scsi_len = 0;
> + assert(s->scsi_len == 0);
Preferably having scsi_len changes in a distinct patch,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
> cbw.cmd, cbw.cmd_len, NULL);
> if (s->commandlog) {
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (6 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 07/10] usb/msd: Add some additional assertions Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 10:28 ` Philippe Mathieu-Daudé
2025-04-11 10:37 ` Philippe Mathieu-Daudé
2025-04-11 8:04 ` [PATCH v2 09/10] usb/msd: Permit a DATA-IN or CSW packet before CBW packet Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 10/10] usb/msd: Add more tracing Nicholas Piggin
9 siblings, 2 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
This reflects a little better what it does, particularly with a
subsequent change to relax the order packets are seen in. This
field is not the general state of the MSD state machine, rather
it follows packets that are completed as part of a CBW command.
The difference is a bit subtle, so for a concrete example, the
next change will permit the host to send a CSW packet before it
sends the associated CBW packet. In that case the CSW packet
will be tracked and the MSD state machine will move, but this
mode / cbw_state field would remain unchanged (in the "expecting
CBW" state), until the CBW packet arrives.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 12 +++++------
hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
2 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index a40d15f5def..c109544f632 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -10,11 +10,11 @@
#include "hw/usb.h"
#include "hw/scsi/scsi.h"
-enum USBMSDMode {
- USB_MSDM_CBW, /* Command Block. */
- USB_MSDM_DATAOUT, /* Transfer data to device. */
- USB_MSDM_DATAIN, /* Transfer data from device. */
- USB_MSDM_CSW /* Command Status. */
+enum USBMSDCBWState {
+ USB_MSD_CBW_NONE, /* Ready, waiting for CBW packet. */
+ USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
+ USB_MSD_CBW_DATAIN, /* Expecting DATA-IN (from device) packet */
+ USB_MSD_CBW_CSW /* No more data, expecting CSW packet. */
};
struct QEMU_PACKED usb_msd_csw {
@@ -26,7 +26,7 @@ struct QEMU_PACKED usb_msd_csw {
struct MSDState {
USBDevice dev;
- enum USBMSDMode mode;
+ enum USBMSDCBWState cbw_state;
uint32_t scsi_off;
uint32_t scsi_len;
uint32_t data_len;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 3b806872587..ed6d9b70b96 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -264,12 +264,12 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->data_packet;
- if (s->mode == USB_MSDM_DATAIN) {
+ if (s->cbw_state == USB_MSD_CBW_DATAIN) {
if (req->cmd.mode == SCSI_XFER_TO_DEV) {
usb_msd_fatal_error(s);
return;
}
- } else if (s->mode == USB_MSDM_DATAOUT) {
+ } else if (s->cbw_state == USB_MSD_CBW_DATAOUT) {
if (req->cmd.mode != SCSI_XFER_TO_DEV) {
usb_msd_fatal_error(s);
return;
@@ -301,7 +301,7 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
g_assert(s->req);
/* The CBW is what starts the SCSI request */
- g_assert(s->mode != USB_MSDM_CBW);
+ g_assert(s->cbw_state != USB_MSD_CBW_NONE);
s->csw.sig = cpu_to_le32(0x53425355);
s->csw.tag = cpu_to_le32(req->tag);
@@ -312,7 +312,8 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->req = NULL;
if (p) {
- g_assert(s->mode == USB_MSDM_DATAIN || s->mode == USB_MSDM_DATAOUT);
+ g_assert(s->cbw_state == USB_MSD_CBW_DATAIN ||
+ s->cbw_state == USB_MSD_CBW_DATAOUT);
if (s->data_len) {
int len = (p->iov.size - p->actual_length);
usb_packet_skip(p, len);
@@ -322,19 +323,19 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->data_len -= len;
}
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
} else if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
- if (s->mode == USB_MSDM_CSW) {
+ if (s->cbw_state == USB_MSD_CBW_CSW) {
p = s->csw_in_packet;
if (p) {
usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
+ s->cbw_state = USB_MSD_CBW_NONE;
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
}
@@ -377,7 +378,7 @@ void usb_msd_handle_reset(USBDevice *dev)
}
memset(&s->csw, 0, sizeof(s->csw));
- s->mode = USB_MSDM_CBW;
+ s->cbw_state = USB_MSD_CBW_NONE;
s->needs_reset = false;
}
@@ -478,8 +479,8 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
SCSIDevice *scsi_dev;
int len;
- switch (s->mode) {
- case USB_MSDM_CBW:
+ switch (s->cbw_state) {
+ case USB_MSD_CBW_NONE:
if (!try_get_valid_cbw(p, &cbw)) {
goto fail;
}
@@ -492,11 +493,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
tag = le32_to_cpu(cbw.tag);
s->data_len = le32_to_cpu(cbw.data_len);
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
} else if (cbw.flags & 0x80) {
- s->mode = USB_MSDM_DATAIN;
+ s->cbw_state = USB_MSD_CBW_DATAIN;
} else {
- s->mode = USB_MSDM_DATAOUT;
+ s->cbw_state = USB_MSD_CBW_DATAOUT;
}
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
@@ -513,7 +514,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
break;
- case USB_MSDM_DATAOUT:
+ case USB_MSD_CBW_DATAOUT:
trace_usb_msd_data_out(p->iov.size, s->data_len);
if (p->iov.size > s->data_len) {
goto fail;
@@ -531,7 +532,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
s->data_len -= len;
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
}
}
@@ -556,8 +557,8 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
MSDState *s = (MSDState *)dev;
int len;
- switch (s->mode) {
- case USB_MSDM_DATAOUT:
+ switch (s->cbw_state) {
+ case USB_MSD_CBW_DATAOUT:
if (!check_valid_csw(p)) {
goto fail;
}
@@ -573,7 +574,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
p->status = USB_RET_ASYNC;
break;
- case USB_MSDM_CSW:
+ case USB_MSD_CBW_CSW:
if (!check_valid_csw(p)) {
goto fail;
}
@@ -585,11 +586,11 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
p->status = USB_RET_ASYNC;
} else {
usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
+ s->cbw_state = USB_MSD_CBW_NONE;
}
break;
- case USB_MSDM_DATAIN:
+ case USB_MSD_CBW_DATAIN:
trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
if (s->scsi_len) {
usb_msd_copy_data(s, p);
@@ -603,11 +604,12 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
}
s->data_len -= len;
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
}
}
- if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
+ if (p->actual_length < p->iov.size &&
+ s->cbw_state == USB_MSD_CBW_DATAIN) {
trace_usb_msd_packet_async();
s->data_packet = p;
p->status = USB_RET_ASYNC;
@@ -672,7 +674,7 @@ static const VMStateDescription vmstate_usb_msd = {
.minimum_version_id = 1,
.fields = (const VMStateField[]) {
VMSTATE_USB_DEVICE(dev, MSDState),
- VMSTATE_UINT32(mode, MSDState),
+ VMSTATE_UINT32(cbw_state, MSDState),
VMSTATE_UINT32(scsi_len, MSDState),
VMSTATE_UINT32(scsi_off, MSDState),
VMSTATE_UINT32(data_len, MSDState),
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names
2025-04-11 8:04 ` [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names Nicholas Piggin
@ 2025-04-11 10:28 ` Philippe Mathieu-Daudé
2025-04-11 10:37 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:28 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 10:04, Nicholas Piggin wrote:
> This reflects a little better what it does, particularly with a
> subsequent change to relax the order packets are seen in. This
> field is not the general state of the MSD state machine, rather
> it follows packets that are completed as part of a CBW command.
>
> The difference is a bit subtle, so for a concrete example, the
> next change will permit the host to send a CSW packet before it
> sends the associated CBW packet. In that case the CSW packet
> will be tracked and the MSD state machine will move, but this
> mode / cbw_state field would remain unchanged (in the "expecting
> CBW" state), until the CBW packet arrives.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> include/hw/usb/msd.h | 12 +++++------
> hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
> 2 files changed, 32 insertions(+), 30 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names
2025-04-11 8:04 ` [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names Nicholas Piggin
2025-04-11 10:28 ` Philippe Mathieu-Daudé
@ 2025-04-11 10:37 ` Philippe Mathieu-Daudé
2025-04-12 5:33 ` Nicholas Piggin
1 sibling, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:37 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 10:04, Nicholas Piggin wrote:
> This reflects a little better what it does, particularly with a
> subsequent change to relax the order packets are seen in. This
> field is not the general state of the MSD state machine, rather
> it follows packets that are completed as part of a CBW command.
>
> The difference is a bit subtle, so for a concrete example, the
> next change will permit the host to send a CSW packet before it
> sends the associated CBW packet. In that case the CSW packet
> will be tracked and the MSD state machine will move, but this
> mode / cbw_state field would remain unchanged (in the "expecting
> CBW" state), until the CBW packet arrives.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> include/hw/usb/msd.h | 12 +++++------
> hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
> 2 files changed, 32 insertions(+), 30 deletions(-)
>
> diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
> index a40d15f5def..c109544f632 100644
> --- a/include/hw/usb/msd.h
> +++ b/include/hw/usb/msd.h
> @@ -10,11 +10,11 @@
> #include "hw/usb.h"
> #include "hw/scsi/scsi.h"
>
> -enum USBMSDMode {
> - USB_MSDM_CBW, /* Command Block. */
> - USB_MSDM_DATAOUT, /* Transfer data to device. */
> - USB_MSDM_DATAIN, /* Transfer data from device. */
> - USB_MSDM_CSW /* Command Status. */
Since modifying this, please add
typedef
> +enum USBMSDCBWState {
> + USB_MSD_CBW_NONE, /* Ready, waiting for CBW packet. */
> + USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
> + USB_MSD_CBW_DATAIN, /* Expecting DATA-IN (from device) packet */
> + USB_MSD_CBW_CSW /* No more data, expecting CSW packet. */
> }
USBMSDCBWState;
>
> struct QEMU_PACKED usb_msd_csw {
> @@ -26,7 +26,7 @@ struct QEMU_PACKED usb_msd_csw {
>
> struct MSDState {
> USBDevice dev;
> - enum USBMSDMode mode;
> + enum USBMSDCBWState cbw_state;
USBMSDCBWState cbw_state;
> uint32_t scsi_off;
> uint32_t scsi_len;
> uint32_t data_len;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names
2025-04-11 10:37 ` Philippe Mathieu-Daudé
@ 2025-04-12 5:33 ` Nicholas Piggin
0 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-12 5:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On Fri Apr 11, 2025 at 8:37 PM AEST, Philippe Mathieu-Daudé wrote:
> On 11/4/25 10:04, Nicholas Piggin wrote:
>> This reflects a little better what it does, particularly with a
>> subsequent change to relax the order packets are seen in. This
>> field is not the general state of the MSD state machine, rather
>> it follows packets that are completed as part of a CBW command.
>>
>> The difference is a bit subtle, so for a concrete example, the
>> next change will permit the host to send a CSW packet before it
>> sends the associated CBW packet. In that case the CSW packet
>> will be tracked and the MSD state machine will move, but this
>> mode / cbw_state field would remain unchanged (in the "expecting
>> CBW" state), until the CBW packet arrives.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> include/hw/usb/msd.h | 12 +++++------
>> hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
>> 2 files changed, 32 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
>> index a40d15f5def..c109544f632 100644
>> --- a/include/hw/usb/msd.h
>> +++ b/include/hw/usb/msd.h
>> @@ -10,11 +10,11 @@
>> #include "hw/usb.h"
>> #include "hw/scsi/scsi.h"
>>
>> -enum USBMSDMode {
>> - USB_MSDM_CBW, /* Command Block. */
>> - USB_MSDM_DATAOUT, /* Transfer data to device. */
>> - USB_MSDM_DATAIN, /* Transfer data from device. */
>> - USB_MSDM_CSW /* Command Status. */
>
> Since modifying this, please add
>
> typedef
Done.
Thanks,
Nick
>
>> +enum USBMSDCBWState {
>> + USB_MSD_CBW_NONE, /* Ready, waiting for CBW packet. */
>> + USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
>> + USB_MSD_CBW_DATAIN, /* Expecting DATA-IN (from device) packet */
>> + USB_MSD_CBW_CSW /* No more data, expecting CSW packet. */
>> }
>
> USBMSDCBWState;
>
>>
>> struct QEMU_PACKED usb_msd_csw {
>> @@ -26,7 +26,7 @@ struct QEMU_PACKED usb_msd_csw {
>>
>> struct MSDState {
>> USBDevice dev;
>> - enum USBMSDMode mode;
>> + enum USBMSDCBWState cbw_state;
>
> USBMSDCBWState cbw_state;
>
>> uint32_t scsi_off;
>> uint32_t scsi_len;
>> uint32_t data_len;
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 09/10] usb/msd: Permit a DATA-IN or CSW packet before CBW packet
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (7 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 08/10] usb/msd: Rename mode to cbw_state, and tweak names Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 8:04 ` [PATCH v2 10/10] usb/msd: Add more tracing Nicholas Piggin
9 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
The USB MSD protocol has 3 packets that make up a command, and only one
command may be active at any time.
- CBW to start a command (that contains a SCSI request).
- DATA (IN or OUT) to request data transfer between host and SCSI layer.
- CSW to return status and complete the command.
DATA is omitted if the request has no data.
The QEMU MSD model requires these packets to arrive in this order, CBW,
DATA, CSW. This is the way the state machine is generally described in
the MSD spec, and this must be how most USB stacks operate. Except AIX.
Universal Serial Bus Mass Storage Class Bulk-Only Transport 1.0 contains
one word in one sentence that permits the relaxed ordering:
3.3 Host/Device Packet Transfer Order
The host shall send the CBW before the associated Data-Out, and the
device shall send Data-In after the associated CBW and before the
associated CSW. The host may request Data-In or CSW before sending the
associated CBW.
Complicating matters, DATA-IN and CSW are both input packets that arrive
in the same manner, so before a CBW it is impossible to determine if an
IN packet is for data or CSW.
So permit "unusually-ordered" packets by tracking them as an "unknown"
packet until the CBW arrives, then they are categorized into a DATA or
CSW packet.
It is not clear whether the spec permits multiple such packets before
the CBW. This implementation permits only one, which seems to be enough
for AIX.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 1 +
hw/usb/dev-storage.c | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index c109544f632..2ed3664b31d 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -37,6 +37,7 @@ struct MSDState {
/* For async completion. */
USBPacket *data_packet;
USBPacket *csw_in_packet;
+ USBPacket *unknown_in_packet;
/* usb-storage only */
BlockConf conf;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index ed6d9b70b96..654b9071d33 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -436,6 +436,8 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
}
} else if (p == s->csw_in_packet) {
s->csw_in_packet = NULL;
+ } else if (p == s->unknown_in_packet) {
+ s->unknown_in_packet = NULL;
} else {
g_assert_not_reached();
}
@@ -499,6 +501,19 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
} else {
s->cbw_state = USB_MSD_CBW_DATAOUT;
}
+ if (s->unknown_in_packet) {
+ if (s->cbw_state == USB_MSD_CBW_DATAIN) {
+ /* Must be a DATAIN packet */
+ s->data_packet = s->unknown_in_packet;
+ } else {
+ /* Must be the CSW packet */
+ if (!check_valid_csw(s->unknown_in_packet)) {
+ goto fail;
+ }
+ s->csw_in_packet = s->unknown_in_packet;
+ }
+ s->unknown_in_packet = NULL;
+ }
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
assert(le32_to_cpu(s->csw.residue) == 0);
@@ -516,6 +531,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
case USB_MSD_CBW_DATAOUT:
trace_usb_msd_data_out(p->iov.size, s->data_len);
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAOUT state");
+ goto fail;
+ }
+
if (p->iov.size > s->data_len) {
goto fail;
}
@@ -558,7 +578,22 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
int len;
switch (s->cbw_state) {
+ case USB_MSD_CBW_NONE:
+ if (s->unknown_in_packet) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: second IN packet was"
+ "received before CBW\n");
+ goto fail;
+ }
+ trace_usb_msd_packet_async();
+ s->unknown_in_packet = p;
+ p->status = USB_RET_ASYNC;
+ break;
+
case USB_MSD_CBW_DATAOUT:
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAOUT state");
+ goto fail;
+ }
if (!check_valid_csw(p)) {
goto fail;
}
@@ -575,6 +610,10 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSD_CBW_CSW:
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAOUT state");
+ goto fail;
+ }
if (!check_valid_csw(p)) {
goto fail;
}
@@ -592,6 +631,10 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
case USB_MSD_CBW_DATAIN:
trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAIN state");
+ goto fail;
+ }
if (s->scsi_len) {
usb_msd_copy_data(s, p);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 10/10] usb/msd: Add more tracing
2025-04-11 8:04 [PATCH v2 00/10] usb/msd: Permit relaxed ordering of IN packets Nicholas Piggin
` (8 preceding siblings ...)
2025-04-11 8:04 ` [PATCH v2 09/10] usb/msd: Permit a DATA-IN or CSW packet before CBW packet Nicholas Piggin
@ 2025-04-11 8:04 ` Nicholas Piggin
2025-04-11 10:36 ` Philippe Mathieu-Daudé
9 siblings, 1 reply; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-11 8:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Nicholas Piggin, qemu-devel, Kevin Wolf
Add tracing for more received packet types, cbw_state changes, and
some more SCSI callbacks. These were useful in debugging relaxed
packet ordering support.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 23 +++++++++++++++++++++--
hw/usb/trace-events | 9 ++++++++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 654b9071d33..0ed39de189d 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -187,7 +187,7 @@ static void usb_msd_data_packet_complete(MSDState *s, int status)
* because another request may be issued before usb_packet_complete
* returns.
*/
- trace_usb_msd_packet_complete();
+ trace_usb_msd_data_packet_complete();
s->data_packet = NULL;
p->status = status;
usb_packet_complete(&s->dev, p);
@@ -202,7 +202,7 @@ static void usb_msd_csw_packet_complete(MSDState *s, int status)
* because another request may be issued before usb_packet_complete
* returns.
*/
- trace_usb_msd_packet_complete();
+ trace_usb_msd_csw_packet_complete();
s->csw_in_packet = NULL;
p->status = status;
usb_packet_complete(&s->dev, p);
@@ -231,7 +231,11 @@ static void usb_msd_fatal_error(MSDState *s)
static void usb_msd_copy_data(MSDState *s, USBPacket *p)
{
uint32_t len;
+
len = p->iov.size - p->actual_length;
+
+ trace_usb_msd_copy_data(s->req->tag, len);
+
if (len > s->scsi_len)
len = s->scsi_len;
usb_packet_copy(p, scsi_req_get_buf(s->req) + s->scsi_off, len);
@@ -264,6 +268,8 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->data_packet;
+ trace_usb_msd_transfer_data(req->tag, len);
+
if (s->cbw_state == USB_MSD_CBW_DATAIN) {
if (req->cmd.mode == SCSI_XFER_TO_DEV) {
usb_msd_fatal_error(s);
@@ -324,11 +330,13 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
}
if (s->data_len == 0) {
s->cbw_state = USB_MSD_CBW_CSW;
+ trace_usb_msd_cbw_state(s->cbw_state);
}
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
} else if (s->data_len == 0) {
s->cbw_state = USB_MSD_CBW_CSW;
+ trace_usb_msd_cbw_state(s->cbw_state);
}
if (s->cbw_state == USB_MSD_CBW_CSW) {
@@ -336,6 +344,7 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
if (p) {
usb_msd_send_status(s, p);
s->cbw_state = USB_MSD_CBW_NONE;
+ trace_usb_msd_cbw_state(s->cbw_state);
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
}
@@ -379,6 +388,7 @@ void usb_msd_handle_reset(USBDevice *dev)
memset(&s->csw, 0, sizeof(s->csw));
s->cbw_state = USB_MSD_CBW_NONE;
+ trace_usb_msd_cbw_state(s->cbw_state);
s->needs_reset = false;
}
@@ -429,6 +439,8 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
{
MSDState *s = USB_STORAGE_DEV(dev);
+ trace_usb_msd_cancel_io();
+
if (p == s->data_packet) {
s->data_packet = NULL;
if (s->req) {
@@ -516,6 +528,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
+ trace_usb_msd_cbw_state(s->cbw_state);
assert(le32_to_cpu(s->csw.residue) == 0);
assert(s->scsi_len == 0);
s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
@@ -553,6 +566,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
s->data_len -= len;
if (s->data_len == 0) {
s->cbw_state = USB_MSD_CBW_CSW;
+ trace_usb_msd_cbw_state(s->cbw_state);
}
}
}
@@ -579,6 +593,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
switch (s->cbw_state) {
case USB_MSD_CBW_NONE:
+ trace_usb_msd_unknown_in(p->iov.size);
if (s->unknown_in_packet) {
qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: second IN packet was"
"received before CBW\n");
@@ -590,6 +605,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSD_CBW_DATAOUT:
+ trace_usb_msd_csw_in();
if (s->unknown_in_packet) {
error_report("usb-msd: unknown_in_packet in DATAOUT state");
goto fail;
@@ -610,6 +626,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSD_CBW_CSW:
+ trace_usb_msd_csw_in();
if (s->unknown_in_packet) {
error_report("usb-msd: unknown_in_packet in DATAOUT state");
goto fail;
@@ -626,6 +643,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
} else {
usb_msd_send_status(s, p);
s->cbw_state = USB_MSD_CBW_NONE;
+ trace_usb_msd_cbw_state(s->cbw_state);
}
break;
@@ -648,6 +666,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
s->data_len -= len;
if (s->data_len == 0) {
s->cbw_state = USB_MSD_CBW_CSW;
+ trace_usb_msd_cbw_state(s->cbw_state);
}
}
}
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index dd04f14add1..688f7ba0b3d 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -264,12 +264,19 @@ usb_msd_maxlun(unsigned maxlun) "%d"
usb_msd_send_status(unsigned status, unsigned tag, size_t size) "status %d, tag 0x%x, len %zd"
usb_msd_data_in(unsigned packet, unsigned remaining, unsigned total) "%d/%d (scsi %d)"
usb_msd_data_out(unsigned packet, unsigned remaining) "%d/%d"
+usb_msd_unknown_in(unsigned packet) "%d"
+usb_msd_csw_in(void) ""
usb_msd_packet_async(void) ""
-usb_msd_packet_complete(void) ""
+usb_msd_data_packet_complete(void) ""
+usb_msd_csw_packet_complete(void) ""
usb_msd_cmd_submit(unsigned lun, unsigned tag, unsigned flags, unsigned len, unsigned data_len) "lun %u, tag 0x%x, flags 0x%08x, len %d, data-len %d"
usb_msd_cmd_complete(unsigned status, unsigned tag) "status %d, tag 0x%x"
+usb_msd_copy_data(unsigned tag, unsigned len) "tag 0x%x len %d"
+usb_msd_transfer_data(unsigned tag, unsigned len) "tag 0x%x len %d"
usb_msd_cmd_cancel(unsigned tag) "tag 0x%x"
+usb_msd_cancel_io(void) ""
usb_msd_fatal_error(void) ""
+usb_msd_cbw_state(unsigned cbw_state) "cbw-state %d"
# dev-uas.c
usb_uas_reset(int addr) "dev %d"
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 10/10] usb/msd: Add more tracing
2025-04-11 8:04 ` [PATCH v2 10/10] usb/msd: Add more tracing Nicholas Piggin
@ 2025-04-11 10:36 ` Philippe Mathieu-Daudé
2025-04-12 5:33 ` Nicholas Piggin
0 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-11 10:36 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On 11/4/25 10:04, Nicholas Piggin wrote:
> Add tracing for more received packet types, cbw_state changes, and
> some more SCSI callbacks. These were useful in debugging relaxed
> packet ordering support.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/usb/dev-storage.c | 23 +++++++++++++++++++++--
> hw/usb/trace-events | 9 ++++++++-
> 2 files changed, 29 insertions(+), 3 deletions(-)
> static void usb_msd_copy_data(MSDState *s, USBPacket *p)
> {
> uint32_t len;
> +
> len = p->iov.size - p->actual_length;
> +
> + trace_usb_msd_copy_data(s->req->tag, len);
> +
> if (len > s->scsi_len)
> len = s->scsi_len;
> usb_packet_copy(p, scsi_req_get_buf(s->req) + s->scsi_off, len);
> @@ -264,6 +268,8 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
> MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
> USBPacket *p = s->data_packet;
>
> + trace_usb_msd_transfer_data(req->tag, len);
> +
> if (s->cbw_state == USB_MSD_CBW_DATAIN) {
> if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> usb_msd_fatal_error(s);
> @@ -324,11 +330,13 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
> }
> if (s->data_len == 0) {
> s->cbw_state = USB_MSD_CBW_CSW;
> + trace_usb_msd_cbw_state(s->cbw_state);
> }
> /* USB_RET_SUCCESS status clears previous ASYNC status */
> usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
> } else if (s->data_len == 0) {
> s->cbw_state = USB_MSD_CBW_CSW;
> + trace_usb_msd_cbw_state(s->cbw_state);
> }
Maybe helpful to log state transition?
void usb_msd_cbw_change_state(MSDState *s,
enum USBMSDCBWState cbw_state)
{
if (s->cbw_state != cbw_state) {
trace_usb_msd_cbw_state(s->cbw_state, cbw_state);
s->cbw_state = cbw_state;
}
}
Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 10/10] usb/msd: Add more tracing
2025-04-11 10:36 ` Philippe Mathieu-Daudé
@ 2025-04-12 5:33 ` Nicholas Piggin
0 siblings, 0 replies; 22+ messages in thread
From: Nicholas Piggin @ 2025-04-12 5:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Gerd Hoffmann; +Cc: qemu-devel, Kevin Wolf
On Fri Apr 11, 2025 at 8:36 PM AEST, Philippe Mathieu-Daudé wrote:
> On 11/4/25 10:04, Nicholas Piggin wrote:
>> Add tracing for more received packet types, cbw_state changes, and
>> some more SCSI callbacks. These were useful in debugging relaxed
>> packet ordering support.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> hw/usb/dev-storage.c | 23 +++++++++++++++++++++--
>> hw/usb/trace-events | 9 ++++++++-
>> 2 files changed, 29 insertions(+), 3 deletions(-)
>
>
>> static void usb_msd_copy_data(MSDState *s, USBPacket *p)
>> {
>> uint32_t len;
>> +
>> len = p->iov.size - p->actual_length;
>> +
>> + trace_usb_msd_copy_data(s->req->tag, len);
>> +
>> if (len > s->scsi_len)
>> len = s->scsi_len;
>> usb_packet_copy(p, scsi_req_get_buf(s->req) + s->scsi_off, len);
>> @@ -264,6 +268,8 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
>> MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
>> USBPacket *p = s->data_packet;
>>
>> + trace_usb_msd_transfer_data(req->tag, len);
>> +
>> if (s->cbw_state == USB_MSD_CBW_DATAIN) {
>> if (req->cmd.mode == SCSI_XFER_TO_DEV) {
>> usb_msd_fatal_error(s);
>> @@ -324,11 +330,13 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
>> }
>> if (s->data_len == 0) {
>> s->cbw_state = USB_MSD_CBW_CSW;
>> + trace_usb_msd_cbw_state(s->cbw_state);
>> }
>> /* USB_RET_SUCCESS status clears previous ASYNC status */
>> usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
>> } else if (s->data_len == 0) {
>> s->cbw_state = USB_MSD_CBW_CSW;
>> + trace_usb_msd_cbw_state(s->cbw_state);
>> }
>
> Maybe helpful to log state transition?
>
> void usb_msd_cbw_change_state(MSDState *s,
> enum USBMSDCBWState cbw_state)
> {
> if (s->cbw_state != cbw_state) {
> trace_usb_msd_cbw_state(s->cbw_state, cbw_state);
> s->cbw_state = cbw_state;
> }
> }
Yeah that's not a bad idea. I added that. I made a few more tweaks
and added some more trace points too, but nothing major.
>
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 22+ messages in thread