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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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@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@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.
>>
>