Alexey,
On 04/09/19 6:53 PM, Alexey Brodkin wrote:
Hi Faiz,
-----Original Message----- From: Faiz Abbas faiz_abbas@ti.com Sent: Wednesday, September 4, 2019 4:09 PM To: Alexey Brodkin abrodkin@synopsys.com Cc: paulemge@forallsecure.com; trini@konsulko.com; u-boot@lists.denx.de Subject: Re: [PATCH] Revert "part: Allocate only one legacy_mbr buffer"
Hi Alexey,
On 04/09/19 6:27 PM, Alexey Brodkin wrote:
Hi Faiz,
[snip]
> I guess what you really want to do is to allocate buffer for "mbr" > dynamically of size which is max(sizeof(legacy_mbr), dev_desc->blksz). >
With the assumption that blksz is always greater than sizeof(legacy_mbr), this should work:
ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, DIV_ROUND_UP(dev_desc->blksz, sizeof(legacy_mbr)));
No this won't work. See ALLOC_CACHE_ALIGN_BUFFER does static allocation in compile-time while blksz is set in runtime.
That's why I mentioned switch to runtime allocation.
Hmm. So the problem isn't as much about allocating too much in the buffer but about using dynamically determined size for static array declaration.
In fact it was a problem of huge static allocation I discovered just by chance building U-Boot for a very memory-limited device, see [1].
This is being used all over this disk/part_dos.c file. Shouldn't we fix all of that? Not sure how it has been working all along.
That part I don't quite understand. What's being used, where and how? And what's the problem with dynamic allocation of the buffer for MBR?
Isn't the following line (being used in different functions in disk/part_dos.c) also problematic because we don't know the value of blksz at compile time?
ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
Ok I see now. Looks like I missed that Variable-Length Array (VLA) feature added to C99 so up-to-date GCC apparently supports it.
But I would strongly recommend to get rid of VLA usage by all means, see [1] & [2].
[1] https://lwn.net/Articles/749064/ [2] https://lwn.net/Articles/749089/
Interesting. This is news to me.
Thanks, Faiz