On Tue, Mar 08, 2016 at 02:26:36PM +0700, Peter Maydell wrote: > On 8 March 2016 at 14:00, Peter Xu <pet...@redhat.com> wrote: > > First of all, this function cannot be inlined even with always_inline, > > so removing inline. > > Please don't mix two different changes in one patch.
Sorry. Will follow this. > > > After that, make its stack bounded. > > > > Suggested-by: Paolo Bonzini <pbonz...@redhat.com> > > CC: Gerd Hoffmann <kra...@redhat.com> > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > hw/usb/hcd-xhci.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > > index 44b6f8c..3dd5a02 100644 > > --- a/hw/usb/hcd-xhci.c > > +++ b/hw/usb/hcd-xhci.c > > @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState > > *xhci, dma_addr_t addr, > > } > > } > > > > -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > > - uint32_t *buf, size_t len) > > +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > > + uint32_t *buf, size_t len) > > { > > int i; > > - uint32_t tmp[len / sizeof(uint32_t)]; > > + uint32_t n = len / sizeof(uint32_t); > > +#define __BUF_SIZE (12) > > + uint32_t tmp[__BUF_SIZE]; > > > > + assert(__BUF_SIZE >= n); > > assert((len % sizeof(uint32_t)) == 0); > > > > - for (i = 0; i < (len / sizeof(uint32_t)); i++) { > > + for (i = 0; i < n; i++) { > > tmp[i] = cpu_to_le32(buf[i]); > > } > > pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len); > > +#undef __BUF_SIZE > > All the patches in this series seem to be following the > same pattern of #defining an arbitrary fixed length for > the array. This does not at all seem to me to be an > improvement. We should be avoiding unbounded stack > allocations, but we need to do this by either changing > the code to work correctly without an unbounded allocation > or by using a heap allocation instead of a stack allocation. > > In some cases, like this one, the original code isn't even > unbounded -- we always call this function with a length > parameter which is a small compile time constant, so the > stack usage is definitely bounded. So this change is making > the code uglier and less flexible for no benefit that I > can see. I was trying to avoid the "stack unlimited" warning. There will be a warning generated before applying this patch. The patch solved this. Though I'd say this patch might not be good enough. :( Then, will drop this one too if I have no further better idea. Thanks for review. Peter