[v2] usb/msd: Permit relaxed ordering of IN packets

[PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct

Posted by Nicholas Piggin 4 days, 16 hours ago

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

Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct

Posted by Philippe Mathieu-Daudé 4 days, 13 hours ago

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);
>   }
>

Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct

Posted by Nicholas Piggin 3 days, 18 hours ago

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

Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct

Posted by Philippe Mathieu-Daudé 4 days, 13 hours ago

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);
>>   }
>

Re: [PATCH v2 02/10] usb/msd: Ensure packet structure layout is correct

Posted by Philippe Mathieu-Daudé 4 days, 13 hours ago

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>