Re: [PATCH] usb: allow max 8192 bytes for desc
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 +, Peter Maydell wrote: >>> On Tue, 11 Jan 2022 at 10:54, 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é Signed-off-by: zhenwei pi --- 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-pizhen...@bytedance.com/ > >> >> Regards, >> Daniel >> >
Re: Re: Re: [PATCH] usb: allow max 8192 bytes for desc
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 +, Peter Maydell wrote: On Tue, 11 Jan 2022 at 10:54, 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é Signed-off-by: zhenwei pi --- 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-pizhen...@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-pizhen...@bytedance.com/ -- zhenwei pi
Re: Re: [PATCH] usb: allow max 8192 bytes for desc
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 +, Peter Maydell wrote: > > > On Tue, 11 Jan 2022 at 10:54, 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é > > > > Signed-off-by: zhenwei pi > > > > --- > > > > 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-pizhen...@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: [PATCH] usb: allow max 8192 bytes for desc
On 1/11/22 8:25 PM, Daniel P. Berrangé wrote: On Tue, Jan 11, 2022 at 12:21:42PM +, Peter Maydell wrote: On Tue, 11 Jan 2022 at 10:54, 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é Signed-off-by: zhenwei pi --- 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-pizhen...@bytedance.com/ Regards, Daniel -- zhenwei pi
Re: [PATCH] usb: allow max 8192 bytes for desc
On Tue, Jan 11, 2022 at 12:21:42PM +, Peter Maydell wrote: > On Tue, 11 Jan 2022 at 10:54, 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é > > Signed-off-by: zhenwei pi > > --- > > 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: [PATCH] usb: allow max 8192 bytes for desc
On Tue, 11 Jan 2022 at 10:54, 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é > Signed-off-by: zhenwei pi > --- > 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
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é > Signed-off-by: zhenwei pi > --- > 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é
