[PATCH v3 11/13] arm64/efi: use plain memblock API for adding and removing reserved RAM

[Ard Biesheuvel]

On 21 November 2014 at 16:21, Mark Salter <msalter at redhat.com> wrote:
> On Fri, 2014-11-21 at 13:07 +0100, Ard Biesheuvel wrote:
>> On 20 November 2014 18:54, Mark Salter <msalter at redhat.com> wrote:
>> > On Thu, 2014-11-20 at 18:38 +0100, Ard Biesheuvel wrote:
>> >> On 20 November 2014 18:28, Mark Salter <msalter at redhat.com> wrote:
>> >> > On Tue, 2014-11-18 at 13:57 +0100, Ard Biesheuvel wrote:
>> >> >> diff --git a/drivers/firmware/efi/virtmap.c
>> >> >> b/drivers/firmware/efi/virtmap.c
>> >> >> index 98735fb43581..4b6a5c31629f 100644
>> >> >> --- a/drivers/firmware/efi/virtmap.c
>> >> >> +++ b/drivers/firmware/efi/virtmap.c
>> >> >> @@ -8,6 +8,7 @@
>> >> >>   */
>> >> >>
>> >> >>  #include <linux/efi.h>
>> >> >> +#include <linux/memblock.h>
>> >> >>  #include <linux/mm_types.h>
>> >> >>  #include <linux/rbtree.h>
>> >> >>  #include <linux/rwsem.h>
>> >> >> @@ -97,8 +98,15 @@ void __init efi_virtmap_init(void)
>> >> >>                 u64 paddr, npages, size;
>> >> >>                 pgprot_t prot;
>> >> >>
>> >> >> -               if (!efi_mem_is_usable_region(md))
>> >> >> +               paddr = md->phys_addr;
>> >> >> +               npages = md->num_pages;
>> >> >> +               memrange_efi_to_native(&paddr, &npages);
>> >> >> +               size = npages << PAGE_SHIFT;
>> >> >> +
>> >> >> +               if (!efi_mem_is_usable_region(md)) {
>> >> >>                         efi_register_mem_resource(md);
>> >> >> +                       memblock_remove(paddr, size);
>> >> >> +               }
>> >> >
>> >> > What exactly is the memblock_remove trying to accomplish? With it, I
>> >> > get:
>> >> >
>> >>
>> >> The idea was pfn_valid() will then return false for those pfns,
>> >> allowing us to distinguish them from memory that the kernel may be
>> >> using, primarily for /dev/mem filtering. But apparently it is causing
>> >> problems for you, so I will have to figure out if there is a better
>> >> way of addressing this.
>> >>
>> > Okay. Well I think that works for pfn_valid, but it is confusing the
>> > mem_cgroup code. I think because you end up with multiple memory regions
>> > after creating the holes. Earlier memory management code saw one memory
>> > region. And because this comes after paging_init(), all of the memory
>> > ends up in the kernel linear mapping which is something we were trying
>> > to avoid.
>> >
>>
>> I will drop the memblock_remove() then. I guess I could make the test
>> in devmem_is_allowed() do
>>
>> if (!memblock_is_memory() || memblock_is_reserved())
>>
>> instead of 'if (!pfn_valid())' so that reserved regions become
>> accessible with having to remove them.
>
> Maybe we should add a new memblock to keep track of uefi tables.
> The memblock_is_reserved() seems overly permissive to me.
>

OK. So instead, I will split reserve_regions() into 2 passes:
- first pass adds all EFI_MEMORY_WB regions (unrounded)
- second pass one removes all reserved regions (rounded up to OS page size)

That way, the devmem stuff should still work as well.

QEMU example:

Processing EFI memory map:
  0x0000b711d000-0x0000b71dffff [Runtime Data       |RUN|  |  |  |
|WB|WT|WC|UC]
  0x0000bfb21000-0x0000bfb34fff [Runtime Code       |RUN|  |  |  |
|WB|WT|WC|UC]
  0x0000bfb35000-0x0000bfb66fff [Runtime Data       |RUN|  |  |  |
|WB|WT|WC|UC]
  0x0000bfb6a000-0x0000bfb6afff [Runtime Data       |RUN|  |  |  |
|WB|WT|WC|UC]
  0x0000bfb82000-0x0000bfb82fff [Runtime Data       |RUN|  |  |  |
|WB|WT|WC|UC]
  0x000004000000-0x000007ffffff [Memory Mapped I/O  |RUN|  |  |  |   |
 |  |  |UC]
  0x000009010000-0x000009010fff [Memory Mapped I/O  |RUN|  |  |  |   |
 |  |  |UC]
  0x000040000000-0x0000407affff [Loader Data        |   |  |  |  |
|WB|WT|WC|UC]
  0x0000b71e0000-0x0000bb77efff [Conventional Memory|   |  |  |  |
|WB|WT|WC|UC]
  0x0000bb77f000-0x0000bbc00fff [Boot Data          |   |  |  |  |
|WB|WT|WC|UC]
  0x0000bbc01000-0x0000bbdc2fff [Conventional Memory|   |  |  |  |
|WB|WT|WC|UC]
  0x0000bbdc3000-0x0000bc01ffff [Boot Data          |   |  |  |  |
|WB|WT|WC|UC]
  0x0000bc020000-0x0000bfa43fff [Conventional Memory|   |  |  |  |
|WB|WT|WC|UC]
  0x0000bfa44000-0x0000bfb20fff [Boot Code          |   |  |  |  |
|WB|WT|WC|UC]
  0x0000407b0000-0x00005fdfffff [Conventional Memory|   |  |  |  |
|WB|WT|WC|UC]
  0x00005fe00000-0x00005fe0ffff [Loader Data        |   |  |  |  |
|WB|WT|WC|UC]
  0x0000bfb67000-0x0000bfb69fff [Boot Data          |   |  |  |  |
|WB|WT|WC|UC]
  0x00005fe10000-0x0000b6337fff [Conventional Memory|   |  |  |  |
|WB|WT|WC|UC]
  0x0000bfb6b000-0x0000bfb7efff [Conventional Memory|   |  |  |  |
|WB|WT|WC|UC]
  0x0000bfb7f000-0x0000bfb81fff [Boot Data          |   |  |  |  |
|WB|WT|WC|UC]
  0x0000b6338000-0x0000b6338fff [Loader Data        |   |  |  |  |
|WB|WT|WC|UC]
  0x0000bfb83000-0x0000bfffffff [Boot Data          |   |  |  |  |
|WB|WT|WC|UC]
  0x0000b6339000-0x0000b6a4bfff [Loader Code        |   |  |  |  |
|WB|WT|WC|UC]
  0x0000b6a4c000-0x0000b711cfff [Boot Code          |   |  |  |  |
|WB|WT|WC|UC]

MEMBLOCK configuration:
 memory size = 0x7fef5000 reserved size = 0x1724000
 memory.cnt  = 0x5
 memory[0x0] [0x00000040000000-0x000000b711cfff], 0x7711d000 bytes flags: 0x0
 memory[0x1] [0x000000b71e0000-0x000000bfb20fff], 0x8941000 bytes flags: 0x0
 memory[0x2] [0x000000bfb67000-0x000000bfb69fff], 0x3000 bytes flags: 0x0
 memory[0x3] [0x000000bfb6b000-0x000000bfb81fff], 0x17000 bytes flags: 0x0
 memory[0x4] [0x000000bfb83000-0x000000bfffffff], 0x47d000 bytes flags: 0x0
 reserved.cnt  = 0x4
 reserved[0x0] [0x00000040080000-0x00000040792fff], 0x713000 bytes flags: 0x0
 reserved[0x1] [0x0000005fe00000-0x0000005fe0ffff], 0x10000 bytes flags: 0x0
 reserved[0x2] [0x000000b6338000-0x000000b6338fff], 0x1000 bytes flags: 0x0
 reserved[0x3] [0x000000be800000-0x000000bf7fffff], 0x1000000 bytes flags: 0x0
   memblock_free: [0x00000040792000-0x00000040792fff]
__create_mapping.isra.6+0x9c/0x2ac

>>
>> Are you happy with the other change in this patch, i.e., using
>> memblock_add() directly so that we don't have to deal with the
>> rounding?
>>
>
> Yeah, I think that's okay. Everything else seems to be working fine
> in my testing.
>

Cheers,
Ard.