[PATCH] usb: allow max 8192 bytes for desc

[PATCH] usb: allow max 8192 bytes for desc

[zhenwei pi]

A device of USB video class usually uses larger desc structure, so
use larger buffer to avoid failure. (dev-video.c is ready)

Allocating memory dynamically by g_malloc of the orignal version of
this change, Philippe suggested just using the stack. Test the two
versions of qemu binary, the size of stack gets no change.

CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 hw/usb/desc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/desc.c b/hw/usb/desc.c
index 8b6eaea407..57d2aedba1 100644
--- a/hw/usb/desc.c
+++ b/hw/usb/desc.c
@@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
     bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
     const USBDesc *desc = usb_device_get_usb_desc(dev);
     const USBDescDevice *other_dev;
-    uint8_t buf[256];
+    uint8_t buf[8192];
     uint8_t type = value >> 8;
     uint8_t index = value & 0xff;
     int flags, ret = -1;
-- 
2.25.1

Re: [PATCH] usb: allow max 8192 bytes for desc

[Philippe Mathieu-Daudé]

On 1/11/22 11:49, zhenwei pi wrote:
> A device of USB video class usually uses larger desc structure, so
> use larger buffer to avoid failure. (dev-video.c is ready)
> 
> Allocating memory dynamically by g_malloc of the orignal version of
> this change, Philippe suggested just using the stack. Test the two
> versions of qemu binary, the size of stack gets no change.
> 
> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  hw/usb/desc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> index 8b6eaea407..57d2aedba1 100644
> --- a/hw/usb/desc.c
> +++ b/hw/usb/desc.c
> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>      bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>      const USBDesc *desc = usb_device_get_usb_desc(dev);
>      const USBDescDevice *other_dev;
> -    uint8_t buf[256];
> +    uint8_t buf[8192];
>      uint8_t type = value >> 8;
>      uint8_t index = value & 0xff;
>      int flags, ret = -1;

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH] usb: allow max 8192 bytes for desc

[Peter Maydell]

On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
>
> A device of USB video class usually uses larger desc structure, so
> use larger buffer to avoid failure. (dev-video.c is ready)
>
> Allocating memory dynamically by g_malloc of the orignal version of
> this change, Philippe suggested just using the stack. Test the two
> versions of qemu binary, the size of stack gets no change.
>
> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  hw/usb/desc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> index 8b6eaea407..57d2aedba1 100644
> --- a/hw/usb/desc.c
> +++ b/hw/usb/desc.c
> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>      bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>      const USBDesc *desc = usb_device_get_usb_desc(dev);
>      const USBDescDevice *other_dev;
> -    uint8_t buf[256];
> +    uint8_t buf[8192];
>      uint8_t type = value >> 8;
>      uint8_t index = value & 0xff;
>      int flags, ret = -1;

I think 8K is too large to be allocating as an array on
the stack, so if we need this buffer to be larger we should
switch to some other allocation strategy for it.

thanks
-- PMM

Re: [PATCH] usb: allow max 8192 bytes for desc

[Daniel P. Berrangé]

On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
> >
> > A device of USB video class usually uses larger desc structure, so
> > use larger buffer to avoid failure. (dev-video.c is ready)
> >
> > Allocating memory dynamically by g_malloc of the orignal version of
> > this change, Philippe suggested just using the stack. Test the two
> > versions of qemu binary, the size of stack gets no change.
> >
> > CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > ---
> >  hw/usb/desc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> > index 8b6eaea407..57d2aedba1 100644
> > --- a/hw/usb/desc.c
> > +++ b/hw/usb/desc.c
> > @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
> >      bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
> >      const USBDesc *desc = usb_device_get_usb_desc(dev);
> >      const USBDescDevice *other_dev;
> > -    uint8_t buf[256];
> > +    uint8_t buf[8192];
> >      uint8_t type = value >> 8;
> >      uint8_t index = value & 0xff;
> >      int flags, ret = -1;
> 
> I think 8K is too large to be allocating as an array on
> the stack, so if we need this buffer to be larger we should
> switch to some other allocation strategy for it.

IIUC, querying USB device descriptors is not a hot path, so using
heap allocation feels sufficient.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Re: [PATCH] usb: allow max 8192 bytes for desc

[zhenwei pi]

On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
>> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
>>>
>>> A device of USB video class usually uses larger desc structure, so
>>> use larger buffer to avoid failure. (dev-video.c is ready)
>>>
>>> Allocating memory dynamically by g_malloc of the orignal version of
>>> this change, Philippe suggested just using the stack. Test the two
>>> versions of qemu binary, the size of stack gets no change.
>>>
>>> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>> ---
>>>   hw/usb/desc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>>> index 8b6eaea407..57d2aedba1 100644
>>> --- a/hw/usb/desc.c
>>> +++ b/hw/usb/desc.c
>>> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>>>       bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>>>       const USBDesc *desc = usb_device_get_usb_desc(dev);
>>>       const USBDescDevice *other_dev;
>>> -    uint8_t buf[256];
>>> +    uint8_t buf[8192];
>>>       uint8_t type = value >> 8;
>>>       uint8_t index = value & 0xff;
>>>       int flags, ret = -1;
>>
>> I think 8K is too large to be allocating as an array on
>> the stack, so if we need this buffer to be larger we should
>> switch to some other allocation strategy for it.
> 
> IIUC, querying USB device descriptors is not a hot path, so using
> heap allocation feels sufficient.
> 
Yes, I tested this a lot, and found that it's an unlikely code path:
1, during guest startup, guest tries to probe device.
2, run 'lsusb' command in guest(or other similar commands).

The original patch and context link:
https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/
> 
> Regards,
> Daniel
> 

-- 
zhenwei pi

Re: Re: [PATCH] usb: allow max 8192 bytes for desc

[Daniel P. Berrangé]

On Tue, Jan 11, 2022 at 08:27:35PM +0800, zhenwei pi wrote:
> 
> 
> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
> > On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
> > > On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
> > > > 
> > > > A device of USB video class usually uses larger desc structure, so
> > > > use larger buffer to avoid failure. (dev-video.c is ready)
> > > > 
> > > > Allocating memory dynamically by g_malloc of the orignal version of
> > > > this change, Philippe suggested just using the stack. Test the two
> > > > versions of qemu binary, the size of stack gets no change.
> > > > 
> > > > CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > > > ---
> > > >   hw/usb/desc.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> > > > index 8b6eaea407..57d2aedba1 100644
> > > > --- a/hw/usb/desc.c
> > > > +++ b/hw/usb/desc.c
> > > > @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
> > > >       bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
> > > >       const USBDesc *desc = usb_device_get_usb_desc(dev);
> > > >       const USBDescDevice *other_dev;
> > > > -    uint8_t buf[256];
> > > > +    uint8_t buf[8192];
> > > >       uint8_t type = value >> 8;
> > > >       uint8_t index = value & 0xff;
> > > >       int flags, ret = -1;
> > > 
> > > I think 8K is too large to be allocating as an array on
> > > the stack, so if we need this buffer to be larger we should
> > > switch to some other allocation strategy for it.
> > 
> > IIUC, querying USB device descriptors is not a hot path, so using
> > heap allocation feels sufficient.
> > 
> Yes, I tested this a lot, and found that it's an unlikely code path:
> 1, during guest startup, guest tries to probe device.
> 2, run 'lsusb' command in guest(or other similar commands).
> 
> The original patch and context link:
> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/

Yes, the orignal patch is better I think.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Re: Re: [PATCH] usb: allow max 8192 bytes for desc

[zhenwei pi]

On 1/11/22 8:38 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 08:27:35PM +0800, zhenwei pi wrote:
>>
>>
>> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
>>> On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
>>>> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com> wrote:
>>>>>
>>>>> A device of USB video class usually uses larger desc structure, so
>>>>> use larger buffer to avoid failure. (dev-video.c is ready)
>>>>>
>>>>> Allocating memory dynamically by g_malloc of the orignal version of
>>>>> this change, Philippe suggested just using the stack. Test the two
>>>>> versions of qemu binary, the size of stack gets no change.
>>>>>
>>>>> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>>>> ---
>>>>>    hw/usb/desc.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>>>>> index 8b6eaea407..57d2aedba1 100644
>>>>> --- a/hw/usb/desc.c
>>>>> +++ b/hw/usb/desc.c
>>>>> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket *p,
>>>>>        bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>>>>>        const USBDesc *desc = usb_device_get_usb_desc(dev);
>>>>>        const USBDescDevice *other_dev;
>>>>> -    uint8_t buf[256];
>>>>> +    uint8_t buf[8192];
>>>>>        uint8_t type = value >> 8;
>>>>>        uint8_t index = value & 0xff;
>>>>>        int flags, ret = -1;
>>>>
>>>> I think 8K is too large to be allocating as an array on
>>>> the stack, so if we need this buffer to be larger we should
>>>> switch to some other allocation strategy for it.
>>>
>>> IIUC, querying USB device descriptors is not a hot path, so using
>>> heap allocation feels sufficient.
>>>
>> Yes, I tested this a lot, and found that it's an unlikely code path:
>> 1, during guest startup, guest tries to probe device.
>> 2, run 'lsusb' command in guest(or other similar commands).
>>
>> The original patch and context link:
>> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/
> 
> Yes, the orignal patch is better I think.
> 
> 
> 
> Regards,
> Daniel
> 
By the way, could you please review the v2 version of "camera subsystem"?

https://patchwork.kernel.org/project/qemu-devel/cover/20220106085304.795010-1-pizhenwei@bytedance.com/

-- 
zhenwei pi

Re: [PATCH] usb: allow max 8192 bytes for desc

[Philippe Mathieu-Daudé]

On 1/11/22 13:27, zhenwei pi wrote:
> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
>> On Tue, Jan 11, 2022 at 12:21:42PM +0000, Peter Maydell wrote:
>>> On Tue, 11 Jan 2022 at 10:54, zhenwei pi <pizhenwei@bytedance.com>
>>> wrote:
>>>>
>>>> A device of USB video class usually uses larger desc structure, so
>>>> use larger buffer to avoid failure. (dev-video.c is ready)
>>>>
>>>> Allocating memory dynamically by g_malloc of the orignal version of
>>>> this change, Philippe suggested just using the stack. Test the two
>>>> versions of qemu binary, the size of stack gets no change.
>>>>
>>>> CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>>> ---
>>>>   hw/usb/desc.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>>>> index 8b6eaea407..57d2aedba1 100644
>>>> --- a/hw/usb/desc.c
>>>> +++ b/hw/usb/desc.c
>>>> @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev,
>>>> USBPacket *p,
>>>>       bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
>>>>       const USBDesc *desc = usb_device_get_usb_desc(dev);
>>>>       const USBDescDevice *other_dev;
>>>> -    uint8_t buf[256];
>>>> +    uint8_t buf[8192];
>>>>       uint8_t type = value >> 8;
>>>>       uint8_t index = value & 0xff;
>>>>       int flags, ret = -1;
>>>
>>> I think 8K is too large to be allocating as an array on
>>> the stack, so if we need this buffer to be larger we should
>>> switch to some other allocation strategy for it.
>>
>> IIUC, querying USB device descriptors is not a hot path, so using
>> heap allocation feels sufficient.
>>
> Yes, I tested this a lot, and found that it's an unlikely code path:
> 1, during guest startup, guest tries to probe device.
> 2, run 'lsusb' command in guest(or other similar commands).

Sorry, I thought this was a hot path without looking at the code.

> The original patch and context link:
> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhenwei@bytedance.com/
> 
>>
>> Regards,
>> Daniel
>>
>