[PATCH v2 2/4] cmd: bootefi: Parse reserved-memory node from DT

[Heinrich Schuchardt]

On 3/14/20 8:14 AM, Heinrich Schuchardt wrote:
> On 3/14/20 1:55 AM, Atish Patra wrote:
>> On Fri, Mar 13, 2020 at 5:12 PM Atish Patra <atish.patra at wdc.com> wrote:
>>>
>>> Currently, bootefi only parses memory reservation block to setup
>>> EFI reserved memory mappings. However, it doesn't parse the
>>> reserved-memory[1] device tree node that also can contain the
>>> reserved memory regions.
>>>
>>> Add capability to parse reserved-memory node and update the EFI memory
>>> mappings accordingly.
> 
> Hello Atish,
> 
> thanks for reporting the problem.
> 
> I would appreciate a test to verify that the code really works. We
> should add both types of reserved-memory device tree node to the sandbox
> device-tree and parse the output of 'efidebug memmap' and compare it to
> the output of 'fdt print'. Could you, please, provide a first patch for
> sandbox.dts and sandbox64.dts. The next step then would be to build a
> Python test.
> 
> This patch series could be split into two. One for EFI and one for RISC-V.
> 
> (see comment below)
> 
>>>
>>> 1. <U-Boot 
>>> source>/doc/device-tree-bindings/reserved-memory/reserved-memory.txt]
>>>
>>> Signed-off-by: Atish Patra <atish.patra at wdc.com>
>>> ---
>>>   cmd/bootefi.c | 42 +++++++++++++++++++++++++++++++++---------
>>>   1 file changed, 33 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 24fc42ae898e..43b36fbacfcd 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -149,6 +149,20 @@ done:
>>>          return ret;
>>>   }
>>>
>>> +static void efi_reserve_memory(uint64_t addr, uint64_t size)
>>> +{
>>> +       uint64_t pages;
>>> +
>>> +       /* Convert from sandbox address space. */
>>> +       addr = (uintptr_t)map_sysmem(addr, 0);
>>> +       pages = efi_size_in_pages(size + (addr & EFI_PAGE_MASK));
>>> +       addr &= ~EFI_PAGE_MASK;
>>> +       if (efi_add_memory_map(addr, pages, EFI_RESERVED_MEMORY_TYPE,
>>> +                              false) != EFI_SUCCESS)
>>> +               printf("Reserved memory mapping failed addr %llx size 
>>> %llx\n",
>>> +                     (unsigned long long)addr, (unsigned long 
>>> long)size);
>>> +}
>>> +
>>>   /**
>>>    * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>>>    *
>>> @@ -161,7 +175,8 @@ done:
>>>   static void efi_carve_out_dt_rsv(void *fdt)
>>>   {
>>>          int nr_rsv, i;
>>> -       uint64_t addr, size, pages;
>>> +       uint64_t addr, size;
>>> +       int nodeoffset, subnode;
>>>
>>>          nr_rsv = fdt_num_mem_rsv(fdt);
>>>
>>> @@ -169,15 +184,24 @@ static void efi_carve_out_dt_rsv(void *fdt)
>>>          for (i = 0; i < nr_rsv; i++) {
>>>                  if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0)
>>>                          continue;
>>> +               efi_reserve_memory(addr, size);
>>> +       }
>>>
>>> -               /* Convert from sandbox address space. */
>>> -               addr = (uintptr_t)map_sysmem(addr, 0);
>>> -
>>> -               pages = efi_size_in_pages(size + (addr & 
>>> EFI_PAGE_MASK));
>>> -               addr &= ~EFI_PAGE_MASK;
>>> -               if (efi_add_memory_map(addr, pages, 
>>> EFI_RESERVED_MEMORY_TYPE,
>>> -                                      false) != EFI_SUCCESS)
>>> -                       printf("FDT memrsv map %d: Failed to add to 
>>> map\n", i);
>>> +       /* process reserved-memory */
>>> +       nodeoffset = fdt_subnode_offset(fdt, 0, "reserved-memory");
>>> +       if (nodeoffset >= 0) {
>>> +               subnode = fdt_first_subnode(fdt, nodeoffset);
>>> +               while (subnode >= 0) {
>>> +                       /* check if this subnode has a reg property */
>>> +                       addr = fdtdec_get_addr_size(fdt, subnode, "reg",
>>> +                                                   (fdt_size_t 
>>> *)&size);
>>> +                       if (addr == FDT_ADDR_T_NONE) {
>>> +                               debug("failed to read address/size\n");
> 
> Linux
> Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt has:
> 
> "Each child of the reserved-memory node specifies one or more regions of
> reserved memory. Each child node may either use a 'reg' property to
> specify a specific range of reserved memory, or a 'size' property with
> optional constraints to request a dynamically allocated block of memory.
> ... If both reg and size are present, then the reg property takes
> precedence and size is ignored."
> 
> So finding no reg property should not be considered a bug. I see no need
> for a debug statement here.
> 
> For the sandbox test we should have a node with 'size' and at least two
> nodes with 'reg'.
> 
>>> +                               continue;

You do not advance subnode here, creating an endless loop. So this 
should be:

         /*
          * The /reserved-memory node may have children with
          * a size instead of a reg property.
          */
         if (addr != FDT_ADDR_T_NONE)
                 efi_reserve_memory(addr, size);
         subnode = fdt_next_subnode(fdt, subnode);
}

Best regards

Heinrich

>>> +                       }
>>> +                       efi_reserve_memory(addr, size);
>>> +                       subnode = fdt_next_subnode(fdt, subnode);
>>> +               }
>>>          }
>>>   }
>>>
>>> -- 
>>> 2.25.1
>>>
>>
>> Fixed palmer's email address. Sorry for the spam.
>>
>