[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] efi: Free malloc regions on exit
From: |
Alexander Graf |
Subject: |
Re: [PATCH] efi: Free malloc regions on exit |
Date: |
Fri, 27 May 2016 16:16:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 05/24/2016 06:56 AM, Elliott, Robert (Persistent Memory) wrote:
-----Original Message-----
From: Grub-devel [mailto:address@hidden
On Behalf Of Alexander Graf
Sent: Thursday, May 19, 2016 8:38 AM
Subject: [PATCH] efi: Free malloc regions on exit
...
+struct efi_allocation {
If no other file is using this, mark it as static.
Good idea.
+ grub_uint64_t start_addr;
That should be grub_efi_physical_address_t.
+ grub_uint64_t pages;
That should be grub_efi_uint64_t (the parameter type
for add_memory_regions) or grub_efi_uintn_t (the parameter
type for grub_efi_allocate_pages and grub_efi_free_pages).
Yup for the type changes.
+} efi_allocated_memory[16];
Why 16 and not some other number? Consider a #define and making
the comment about 16 in add_memory_regions more generic.
I've added some helpful comment above and moved it into a #define.
+unsigned int efi_allocated_memory_idx = 0;
The C language definition guarantees that global variables
are initialized to 0.
Yes, but multiple other global variables in the file also initialize
with 0. Last time I checked gcc just puts those variables into bss
either way, so you don't really lose anything by explicitly saying = 0
here. It does increase readability imho though to have it.
@@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t
*memory_map,
(void *) ((grub_addr_t) start),
(unsigned) pages);
+ /* Track up to 16 regions that we allocate from */
+ if (efi_allocated_memory_idx <
ARRAY_SIZE(efi_allocated_memory)) {
+ efi_allocated_memory[efi_allocated_memory_idx].start_addr =
start;
+ efi_allocated_memory[efi_allocated_memory_idx].pages =
pages;
+ efi_allocated_memory_idx++;
+ }
+
Consider printing if the magic number is exceeded, rather than
silently changing the behavior.
Good idea. I don't think anyone really cares enough - there should still
be enough ram getting free'd on exit if memory is that heavily
segmented. But I suppose firmware authors want to be aware that their
memory map is a mess, and this could tell them :).
grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
required_pages -= pages;
@@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t
*memory_map,
grub_fatal ("too little memory");
}
+void
+grub_efi_memory_fini (void)
+{
+ unsigned int i;
+
+ for (i = 0; i < efi_allocated_memory_idx; i++) {
+ grub_efi_free_pages (efi_allocated_memory[i].start_addr,
+ efi_allocated_memory[i].pages);
+ }
+}
Consider clearing efi_allocated_memory_idx after that, since
this function cannot be sure it's the last one to use a
global variable.
I'm not sure it's incredibly helpful (since this is a _fini function
that basically denotes the end of grub's life), but sure.
Clearing start_addr might help prevent stale pointer
reference bugs. Pointers that are no longer valid are
dangerous to leave around.
That one however I don't agree with. Array elements are only really
valid if their counter considers them as valid. If we initialize the
counter to 0 that means the whole array should automatically be
considered unused.
Thanks a lot for the review :).
Alex