Glenn Washburn <developm...@efficientek.com> writes:
> On Tue, 12 Oct 2021 18:29:52 +1100
> Daniel Axtens <d...@axtens.net> wrote:
>
>> I spent more than a trivial quantity of time figuring out pre_size and
>> whether a memory region's size contains the header cell or not.
>>
>> Document the meanings of all the properties. Hopefully now no-one else
>> has to figure it out!
>>
>> Signed-off-by: Daniel Axtens <d...@axtens.net>
>> ---
>> include/grub/mm_private.h | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/include/grub/mm_private.h b/include/grub/mm_private.h
>> index c2c4cb1511c6..e80a059dd4e4 100644
>> --- a/include/grub/mm_private.h
>> +++ b/include/grub/mm_private.h
>> @@ -25,11 +25,18 @@
>> #define GRUB_MM_FREE_MAGIC 0x2d3c2808
>> #define GRUB_MM_ALLOC_MAGIC 0x6db08fa4
>>
>> +/* A header describing a block of memory - either allocated or free */
>> typedef struct grub_mm_header
>> {
>> + /* The 'next' free block in this region. A circular list.
>> + Only meaningful if the block is free.
>> + */
>> struct grub_mm_header *next;
>
> This and the subsequent comments do not follow the multiline comment
> style[1].
Will fix, thanks.
> One nit, "region. A circular" -> "region's circular".
Sure, will do.
> This comment leads me to wonder if the block is not free, what is the
> value of next? Seems like it should be NULL, if its not meaningful.
In practice the value will be:
- if the block was converted from an existing free block: whatever the
'next' pointer was when the block was free
- if the block was carved out of the middle or end of an existing free
block: whatever happened to be at that offset in memory.
We certainly _could_ null it out. I don't think there's any real value
in doing so but I'm open to being convinced. I suppose you could make an
argument that it reduces the chance that we'll follow a pointer to
somewhere random in memory but we check the magic before we check next
so we're fairly safe.
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
>
>> + /* The region size in cells (not bytes). Includes the header cell. */
>
> What exactly does "cell" mean in this context? I'm gathering cell is
> not defined as in this link[2], which is equivalent to "bit". Perhaps
> "size" should be renamed to "ncells" or something more descriptive.
>
> [2] https://en.wikipedia.org/wiki/Memory_cell_(computing)
The definition of cell is from mm.c:
The memory space is used as cells instead of bytes for simplicity. This
is important for some CPUs which may not access multiple bytes at a time
when the first byte is not aligned at a certain boundary (typically,
4-byte or 8-byte). The size of each cell is equal to the size of struct
grub_mm_header, so the header of each allocated/free block fits into one
cell precisely. One cell is 16 bytes on 32-bit platforms and 32 bytes
on 64-bit platforms.
I don't want to change variable names in this patch but I will update
the comment.
Kind regards,
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
