* [PATCH] efi: Free existing memory map before installing new memory map
@ 2018-06-26 2:41 Sai Praneeth Prakhya
2018-06-26 3:15 ` kbuild test robot
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Sai Praneeth Prakhya @ 2018-06-26 2:41 UTC (permalink / raw)
To: linux-efi, linux-kernel
Cc: Sai Praneeth, Lee Chun-Yi, Borislav Petkov, Dave Young,
Laszlo Ersek, Bhupesh Sharma, Ricardo Neri, Ravi Shankar,
Matt Fleming, Ard Biesheuvel
From: Sai Praneeth <sai.praneeth.prakhya@intel.com>
efi_memmap_install(), unmaps the existing memory map and installs the
new memory map but doesn't free the memory allocated to the existing
memory map. Fortunately, the details about the existing memory map are
stored in efi.memmap. Hence, use them to free the memory.
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Lee Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Young <dyoung@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Note: Patch based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
drivers/firmware/efi/memmap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 678e85704054..68b27b14fe94 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -229,6 +229,9 @@ int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
efi_memmap_unmap();
+ /* Free the memory allocated to the existing memory map */
+ efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
+
data.phys_map = addr;
data.size = efi.memmap.desc_size * nr_map;
data.desc_version = efi.memmap.desc_version;
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-26 2:41 [PATCH] efi: Free existing memory map before installing new memory map Sai Praneeth Prakhya
@ 2018-06-26 3:15 ` kbuild test robot
2018-06-26 3:15 ` kbuild test robot
2018-06-26 9:38 ` Ard Biesheuvel
2 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-06-26 3:15 UTC (permalink / raw)
To: Sai Praneeth Prakhya
Cc: kbuild-all, linux-efi, linux-kernel, Sai Praneeth, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma,
Ricardo Neri, Ravi Shankar, Matt Fleming, Ard Biesheuvel
[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]
Hi Sai,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sai-Praneeth-Prakhya/efi-Free-existing-memory-map-before-installing-new-memory-map/20180626-104301
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/firmware//efi/memmap.c: In function 'efi_memmap_install':
>> drivers/firmware//efi/memmap.c:199:2: error: implicit declaration of function 'efi_memmap_free'; did you mean 'vmemmap_free'? [-Werror=implicit-function-declaration]
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
^~~~~~~~~~~~~~~
vmemmap_free
cc1: some warnings being treated as errors
vim +199 drivers/firmware//efi/memmap.c
180
181 /**
182 * efi_memmap_install - Install a new EFI memory map in efi.memmap
183 * @addr: Physical address of the memory map
184 * @nr_map: Number of entries in the memory map
185 *
186 * Unlike efi_memmap_init_*(), this function does not allow the caller
187 * to switch from early to late mappings. It simply uses the existing
188 * mapping function and installs the new memmap.
189 *
190 * Returns zero on success, a negative error code on failure.
191 */
192 int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
193 {
194 struct efi_memory_map_data data;
195
196 efi_memmap_unmap();
197
198 /* Free the memory allocated to the existing memory map */
> 199 efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
200
201 data.phys_map = addr;
202 data.size = efi.memmap.desc_size * nr_map;
203 data.desc_version = efi.memmap.desc_version;
204 data.desc_size = efi.memmap.desc_size;
205
206 return __efi_memmap_init(&data, efi.memmap.late);
207 }
208
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63997 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-26 2:41 [PATCH] efi: Free existing memory map before installing new memory map Sai Praneeth Prakhya
2018-06-26 3:15 ` kbuild test robot
@ 2018-06-26 3:15 ` kbuild test robot
2018-06-26 7:18 ` Prakhya, Sai Praneeth
2018-06-26 9:38 ` Ard Biesheuvel
2 siblings, 1 reply; 12+ messages in thread
From: kbuild test robot @ 2018-06-26 3:15 UTC (permalink / raw)
To: Sai Praneeth Prakhya
Cc: kbuild-all, linux-efi, linux-kernel, Sai Praneeth, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma,
Ricardo Neri, Ravi Shankar, Matt Fleming, Ard Biesheuvel
[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]
Hi Sai,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sai-Praneeth-Prakhya/efi-Free-existing-memory-map-before-installing-new-memory-map/20180626-104301
config: x86_64-randconfig-x003-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/firmware/efi/memmap.c: In function 'efi_memmap_install':
>> drivers/firmware/efi/memmap.c:199:2: error: implicit declaration of function 'efi_memmap_free'; did you mean 'efi_memmap_walk'? [-Werror=implicit-function-declaration]
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
^~~~~~~~~~~~~~~
efi_memmap_walk
cc1: some warnings being treated as errors
vim +199 drivers/firmware/efi/memmap.c
180
181 /**
182 * efi_memmap_install - Install a new EFI memory map in efi.memmap
183 * @addr: Physical address of the memory map
184 * @nr_map: Number of entries in the memory map
185 *
186 * Unlike efi_memmap_init_*(), this function does not allow the caller
187 * to switch from early to late mappings. It simply uses the existing
188 * mapping function and installs the new memmap.
189 *
190 * Returns zero on success, a negative error code on failure.
191 */
192 int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
193 {
194 struct efi_memory_map_data data;
195
196 efi_memmap_unmap();
197
198 /* Free the memory allocated to the existing memory map */
> 199 efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
200
201 data.phys_map = addr;
202 data.size = efi.memmap.desc_size * nr_map;
203 data.desc_version = efi.memmap.desc_version;
204 data.desc_size = efi.memmap.desc_size;
205
206 return __efi_memmap_init(&data, efi.memmap.late);
207 }
208
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32495 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-26 3:15 ` kbuild test robot
@ 2018-06-26 7:18 ` Prakhya, Sai Praneeth
2018-06-27 6:02 ` [kbuild-all] " Ye Xiaolong
0 siblings, 1 reply; 12+ messages in thread
From: Prakhya, Sai Praneeth @ 2018-06-26 7:18 UTC (permalink / raw)
To: lkp
Cc: kbuild-all, linux-efi, linux-kernel, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma, Neri,
Ricardo, Shankar, Ravi V, Matt Fleming, Ard Biesheuvel
>
> Hi Sai,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
> the wrong git tree, please drop us a note to help improve the system]
Since efi_memmap_free() is a recently introduced API [1], please note that the patch
is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
[1] https://lkml.org/lkml/2018/6/22/39
Regards,
Sai
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-26 2:41 [PATCH] efi: Free existing memory map before installing new memory map Sai Praneeth Prakhya
2018-06-26 3:15 ` kbuild test robot
2018-06-26 3:15 ` kbuild test robot
@ 2018-06-26 9:38 ` Ard Biesheuvel
2018-06-27 4:51 ` Prakhya, Sai Praneeth
2 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-06-26 9:38 UTC (permalink / raw)
To: Sai Praneeth Prakhya
Cc: linux-efi, Linux Kernel Mailing List, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma,
Ricardo Neri, Ravi Shankar, Matt Fleming
On 26 June 2018 at 04:41, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.com> wrote:
> From: Sai Praneeth <sai.praneeth.prakhya@intel.com>
>
> efi_memmap_install(), unmaps the existing memory map and installs the
> new memory map but doesn't free the memory allocated to the existing
> memory map. Fortunately, the details about the existing memory map are
> stored in efi.memmap. Hence, use them to free the memory.
>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Lee Chun-Yi <jlee@suse.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Bhupesh Sharma <bhsharma@redhat.com>
> Cc: Ricardo Neri <ricardo.neri@intel.com>
> Cc: Ravi Shankar <ravi.v.shankar@intel.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Note: Patch based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>
> drivers/firmware/efi/memmap.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 678e85704054..68b27b14fe94 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -229,6 +229,9 @@ int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
>
> efi_memmap_unmap();
>
> + /* Free the memory allocated to the existing memory map */
> + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
> +
> data.phys_map = addr;
> data.size = efi.memmap.desc_size * nr_map;
> data.desc_version = efi.memmap.desc_version;
> --
> 2.7.4
>
If only it were so simple :-)
At this point, efi.memmap.phys_map could point to memory that was
allocated early, allocated late or simply passed to the OS at boot
time by the stub (in which case it was memblock_reserve()d but not
memblock_alloc()d, and it should not be freed)
So only allocations made with efi_memmap_alloc() should be freed here.
I'm not sure /how/ we should keep track of that: perhaps it is simply
a matter of replacing the boolean 'late' with an enum that describes
where the memory came from that phys_map points to.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-26 9:38 ` Ard Biesheuvel
@ 2018-06-27 4:51 ` Prakhya, Sai Praneeth
2018-06-27 7:01 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Prakhya, Sai Praneeth @ 2018-06-27 4:51 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Linux Kernel Mailing List, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma, Neri,
Ricardo, Shankar, Ravi V, Matt Fleming
> > + /* Free the memory allocated to the existing memory map */
> > + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map,
> > + efi.memmap.late);
> > +
> > data.phys_map = addr;
> > data.size = efi.memmap.desc_size * nr_map;
> > data.desc_version = efi.memmap.desc_version;
> > --
> > 2.7.4
> >
>
> If only it were so simple :-)
>
> At this point, efi.memmap.phys_map could point to memory that was allocated
> early, allocated late or simply passed to the OS at boot time by the stub (in
> which case it was memblock_reserve()d but not memblock_alloc()d, and it
> should not be freed)
>
Yes, completely agree that there could be three types of allocations for memmap.
I thought,
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
should work because the previous type of allocation should have been recorded in efi.memmap.late.
But, now I see this will fail for memblock_reserved() memory because it will be mistaken to
memblock_alloced() (I assumed both are almost similar :().
> So only allocations made with efi_memmap_alloc() should be freed here.
Makes sense and I think that also means efi_memmap_free() should be called from function
that called efi_memmap_alloc() and not efi_memmap_install().
> I'm not sure /how/ we should keep track of that: perhaps it is simply a matter of
> replacing the boolean 'late' with an enum that describes where the memory
> came from that phys_map points to.
I did try changing boolean late to enum and it seems to be working fine. I will do more
testing/clean up and will submit a patch for review.
Also, could you please clarify if there is any specific reason why memory allocated
using memblock_reserve() shouldn't be freed. I mean, not with memblock_free() but I
think we could make it _available_ using free_bootmem() (or something similar, please
correct me if this is not the right API). If we allocate and install a new memory map (as
in case with efi_fake_memmap()), I think we should free the memory used by memory map
originally passed by EFI stub, because, at any point of time there should only be one active
memory map. If we don't free the original memory map passed by EFI stub, we will be having
two and hence will be leaking memory.
Regards,
Sai
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
2018-06-26 7:18 ` Prakhya, Sai Praneeth
@ 2018-06-27 6:02 ` Ye Xiaolong
2018-06-27 6:09 ` Ard Biesheuvel
2018-06-27 6:32 ` Prakhya, Sai Praneeth
0 siblings, 2 replies; 12+ messages in thread
From: Ye Xiaolong @ 2018-06-27 6:02 UTC (permalink / raw)
To: Prakhya, Sai Praneeth
Cc: lkp, Shankar, Ravi V, linux-efi, Ard Biesheuvel, Matt Fleming,
Laszlo Ersek, Bhupesh Sharma, linux-kernel, Neri, Ricardo,
Lee Chun-Yi, Borislav Petkov, kbuild-all, Dave Young
Hi,
On 06/26, Prakhya, Sai Praneeth wrote:
>>
>> Hi Sai,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on linus/master]
>> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
>> the wrong git tree, please drop us a note to help improve the system]
>
>Since efi_memmap_free() is a recently introduced API [1], please note that the patch
>is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
I noticed the official efi tree recored in MAINTAINERS is git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
are they identical?
EXTENSIBLE FIRMWARE INTERFACE (EFI)
M: Matt Fleming <matt@codeblueprint.co.uk>
L: linux-efi@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
S: Maintained
F: Documentation/efi-stub.txt
F: arch/ia64/kernel/efi.c
F: arch/x86/boot/compressed/eboot.[ch]
F: arch/x86/include/asm/efi.h
F: arch/x86/platform/efi/*
F: drivers/firmware/efi/*
F: include/linux/efi*.h
Thanks,
Xiaolong
>
>[1] https://lkml.org/lkml/2018/6/22/39
>
>Regards,
>Sai
>_______________________________________________
>kbuild-all mailing list
>kbuild-all@lists.01.org
>https://lists.01.org/mailman/listinfo/kbuild-all
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
2018-06-27 6:02 ` [kbuild-all] " Ye Xiaolong
@ 2018-06-27 6:09 ` Ard Biesheuvel
2018-06-27 6:29 ` Ye Xiaolong
2018-06-27 6:32 ` Prakhya, Sai Praneeth
1 sibling, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-06-27 6:09 UTC (permalink / raw)
To: Ye Xiaolong
Cc: Prakhya, Sai Praneeth, lkp, Shankar, Ravi V, linux-efi,
Matt Fleming, Laszlo Ersek, Bhupesh Sharma, linux-kernel, Neri,
Ricardo, Lee Chun-Yi, Borislav Petkov, kbuild-all, Dave Young
On 27 June 2018 at 08:02, Ye Xiaolong <xiaolong.ye@intel.com> wrote:
> Hi,
>
> On 06/26, Prakhya, Sai Praneeth wrote:
>>>
>>> Hi Sai,
>>>
>>> Thank you for the patch! Yet something to improve:
>>>
>>> [auto build test ERROR on linus/master]
>>> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
>>> the wrong git tree, please drop us a note to help improve the system]
>>
>>Since efi_memmap_free() is a recently introduced API [1], please note that the patch
>>is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>
> I noticed the official efi tree recored in MAINTAINERS is git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
> are they identical?
>
Which version of MAINTAINERS did you look at?
> EXTENSIBLE FIRMWARE INTERFACE (EFI)
> M: Matt Fleming <matt@codeblueprint.co.uk>
> L: linux-efi@vger.kernel.org
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> S: Maintained
> F: Documentation/efi-stub.txt
> F: arch/ia64/kernel/efi.c
> F: arch/x86/boot/compressed/eboot.[ch]
> F: arch/x86/include/asm/efi.h
> F: arch/x86/platform/efi/*
> F: drivers/firmware/efi/*
> F: include/linux/efi*.h
>
>
> Thanks,
> Xiaolong
>
>>
>>[1] https://lkml.org/lkml/2018/6/22/39
>>
>>Regards,
>>Sai
>>_______________________________________________
>>kbuild-all mailing list
>>kbuild-all@lists.01.org
>>https://lists.01.org/mailman/listinfo/kbuild-all
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
2018-06-27 6:09 ` Ard Biesheuvel
@ 2018-06-27 6:29 ` Ye Xiaolong
0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2018-06-27 6:29 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Prakhya, Sai Praneeth, lkp, Shankar, Ravi V, linux-efi,
Matt Fleming, Laszlo Ersek, Bhupesh Sharma, linux-kernel, Neri,
Ricardo, Lee Chun-Yi, Borislav Petkov, kbuild-all, Dave Young
On 06/27, Ard Biesheuvel wrote:
>On 27 June 2018 at 08:02, Ye Xiaolong <xiaolong.ye@intel.com> wrote:
>> Hi,
>>
>> On 06/26, Prakhya, Sai Praneeth wrote:
>>>>
>>>> Hi Sai,
>>>>
>>>> Thank you for the patch! Yet something to improve:
>>>>
>>>> [auto build test ERROR on linus/master]
>>>> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
>>>> the wrong git tree, please drop us a note to help improve the system]
>>>
>>>Since efi_memmap_free() is a recently introduced API [1], please note that the patch
>>>is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>>
>> I noticed the official efi tree recored in MAINTAINERS is git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
>> are they identical?
>>
>
>Which version of MAINTAINERS did you look at?
Oops, I just checked a old version. I'll add above efi.git tree to 0day's repo
pool.
Thanks,
Xiaolong
>
>> EXTENSIBLE FIRMWARE INTERFACE (EFI)
>> M: Matt Fleming <matt@codeblueprint.co.uk>
>> L: linux-efi@vger.kernel.org
>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
>> S: Maintained
>> F: Documentation/efi-stub.txt
>> F: arch/ia64/kernel/efi.c
>> F: arch/x86/boot/compressed/eboot.[ch]
>> F: arch/x86/include/asm/efi.h
>> F: arch/x86/platform/efi/*
>> F: drivers/firmware/efi/*
>> F: include/linux/efi*.h
>>
>>
>> Thanks,
>> Xiaolong
>>
>>>
>>>[1] https://lkml.org/lkml/2018/6/22/39
>>>
>>>Regards,
>>>Sai
>>>_______________________________________________
>>>kbuild-all mailing list
>>>kbuild-all@lists.01.org
>>>https://lists.01.org/mailman/listinfo/kbuild-all
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
2018-06-27 6:02 ` [kbuild-all] " Ye Xiaolong
2018-06-27 6:09 ` Ard Biesheuvel
@ 2018-06-27 6:32 ` Prakhya, Sai Praneeth
1 sibling, 0 replies; 12+ messages in thread
From: Prakhya, Sai Praneeth @ 2018-06-27 6:32 UTC (permalink / raw)
To: Ye, Xiaolong
Cc: lkp, Shankar, Ravi V, linux-efi, Ard Biesheuvel, Matt Fleming,
Laszlo Ersek, Bhupesh Sharma, linux-kernel, Neri, Ricardo,
Lee Chun-Yi, Borislav Petkov, kbuild-all, Dave Young
> >Since efi_memmap_free() is a recently introduced API [1], please note
> >that the patch is based on efi tree
> >@https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>
> I noticed the official efi tree recored in MAINTAINERS is
> git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
> are they identical?
>
> EXTENSIBLE FIRMWARE INTERFACE (EFI)
> M: Matt Fleming <matt@codeblueprint.co.uk>
> L: linux-efi@vger.kernel.org
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> S: Maintained
> F: Documentation/efi-stub.txt
> F: arch/ia64/kernel/efi.c
> F: arch/x86/boot/compressed/eboot.[ch]
> F: arch/x86/include/asm/efi.h
> F: arch/x86/platform/efi/*
> F: drivers/firmware/efi/*
> F: include/linux/efi*.h
Hi Xiaolong,
Ard replaced Matt as EFI maintainer. Please see updated MAINTAINERS file for more details.
https://elixir.bootlin.com/linux/v4.18-rc2/source/MAINTAINERS
EXTENSIBLE FIRMWARE INTERFACE (EFI)
M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
L: linux-efi@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
S: Maintained
F: Documentation/efi-stub.txt
F: arch/*/kernel/efi.c
F: arch/x86/boot/compressed/eboot.[ch]
F: arch/*/include/asm/efi.h
F: arch/x86/platform/efi/
F: drivers/firmware/efi/
F: include/linux/efi*.h
F: arch/arm/boot/compressed/efi-header.S
F: arch/arm64/kernel/efi-entry.S
Regards,
Sai
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-27 4:51 ` Prakhya, Sai Praneeth
@ 2018-06-27 7:01 ` Ard Biesheuvel
2018-06-27 7:28 ` Prakhya, Sai Praneeth
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-06-27 7:01 UTC (permalink / raw)
To: Prakhya, Sai Praneeth
Cc: linux-efi, Linux Kernel Mailing List, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma, Neri,
Ricardo, Shankar, Ravi V, Matt Fleming
On 27 June 2018 at 06:51, Prakhya, Sai Praneeth
<sai.praneeth.prakhya@intel.com> wrote:
>> > + /* Free the memory allocated to the existing memory map */
>> > + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map,
>> > + efi.memmap.late);
>> > +
>> > data.phys_map = addr;
>> > data.size = efi.memmap.desc_size * nr_map;
>> > data.desc_version = efi.memmap.desc_version;
>> > --
>> > 2.7.4
>> >
>>
>> If only it were so simple :-)
>>
>> At this point, efi.memmap.phys_map could point to memory that was allocated
>> early, allocated late or simply passed to the OS at boot time by the stub (in
>> which case it was memblock_reserve()d but not memblock_alloc()d, and it
>> should not be freed)
>>
>
> Yes, completely agree that there could be three types of allocations for memmap.
> I thought,
> efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
>
> should work because the previous type of allocation should have been recorded in efi.memmap.late.
> But, now I see this will fail for memblock_reserved() memory because it will be mistaken to
> memblock_alloced() (I assumed both are almost similar :().
>
>> So only allocations made with efi_memmap_alloc() should be freed here.
>
> Makes sense and I think that also means efi_memmap_free() should be called from function
> that called efi_memmap_alloc() and not efi_memmap_install().
>
>> I'm not sure /how/ we should keep track of that: perhaps it is simply a matter of
>> replacing the boolean 'late' with an enum that describes where the memory
>> came from that phys_map points to.
>
> I did try changing boolean late to enum and it seems to be working fine. I will do more
> testing/clean up and will submit a patch for review.
>
> Also, could you please clarify if there is any specific reason why memory allocated
> using memblock_reserve() shouldn't be freed. I mean, not with memblock_free() but I
> think we could make it _available_ using free_bootmem() (or something similar, please
> correct me if this is not the right API).
On arm64, the memory map is provided to the core kernel by the stub,
and after kexec, a pointer to the same memory map will be passed to
the next kernel. So the kernel does not 'own' that allocation, and it
should not free it or overwrite it.
> If we allocate and install a new memory map (as
> in case with efi_fake_memmap()), I think we should free the memory used by memory map
> originally passed by EFI stub, because, at any point of time there should only be one active
> memory map. If we don't free the original memory map passed by EFI stub, we will be having
> two and hence will be leaking memory.
>
> Regards,
> Sai
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] efi: Free existing memory map before installing new memory map
2018-06-27 7:01 ` Ard Biesheuvel
@ 2018-06-27 7:28 ` Prakhya, Sai Praneeth
0 siblings, 0 replies; 12+ messages in thread
From: Prakhya, Sai Praneeth @ 2018-06-27 7:28 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Linux Kernel Mailing List, Lee Chun-Yi,
Borislav Petkov, Dave Young, Laszlo Ersek, Bhupesh Sharma, Neri,
Ricardo, Shankar, Ravi V, Matt Fleming
> > Also, could you please clarify if there is any specific reason why
> > memory allocated using memblock_reserve() shouldn't be freed. I mean,
> > not with memblock_free() but I think we could make it _available_
> > using free_bootmem() (or something similar, please correct me if this is not
> the right API).
>
> On arm64, the memory map is provided to the core kernel by the stub, and after
> kexec, a pointer to the same memory map will be passed to the next kernel. So
> the kernel does not 'own' that allocation, and it should not free it or overwrite
> it.
Thanks for the reply. It confirms that the issue is only on x86 systems.
I see that arm64 doesn't call efi_memmap_alloc() and hence there is no concept
of allocating memory for new memory map and installing it (so no memory leak).
Regards,
Sai
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-06-27 7:28 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 2:41 [PATCH] efi: Free existing memory map before installing new memory map Sai Praneeth Prakhya
2018-06-26 3:15 ` kbuild test robot
2018-06-26 3:15 ` kbuild test robot
2018-06-26 7:18 ` Prakhya, Sai Praneeth
2018-06-27 6:02 ` [kbuild-all] " Ye Xiaolong
2018-06-27 6:09 ` Ard Biesheuvel
2018-06-27 6:29 ` Ye Xiaolong
2018-06-27 6:32 ` Prakhya, Sai Praneeth
2018-06-26 9:38 ` Ard Biesheuvel
2018-06-27 4:51 ` Prakhya, Sai Praneeth
2018-06-27 7:01 ` Ard Biesheuvel
2018-06-27 7:28 ` Prakhya, Sai Praneeth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).