Re: [PATCH v4 5/5] blkmap: add pmem nodes for blkmap memory mapped slices

[Tobias Waldekranz <tobias@waldekranz.com>]

On fre, feb 21, 2025 at 20:55, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> Hi Sughosh
>
> This generally looks ok, but I don't love the idea of unconditionally
> preserving all slices regardless of their usage.
> Basically, if a user doesn't unmap that slice it will end in kernel
> memory. My fear is that someone will forget device sensitive data in a
> blkmap....
>
> On Mon, 3 Feb 2025 at 12:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>>
>> The EFI HTTP boot puts the ISO installer image at some location in
>> memory which needs to be added to the devicetree as persistent
>> memory (pmem) node. The OS installer then gets information about the
>> presence of this ISO image through the pmem node and proceeds with the
>> installation.
>>
>> In U-Boot, this ISO image gets mounted as a blkmap device, with a
>> memory mapped slice. Add a helper function which iterates through all
>> such memory mapped blkmap slices, and calls the FDT fixup function to
>> add the pmem node. Invoke this helper function as part of the DT fixup
>> which happens before booting the OS.
>>
>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>> ---
>> Changes since V3:
>> * Move the definition of the helper function to the efi_helper.c
>> * Remove the region of the blkmap mem map device from the EFI memory
>>   map along with adding the pmem node
>>
>
> [...]
>
>> @@ -680,3 +683,52 @@ out:
>>
>>         return ret;
>>  }
>> +
>> +static int add_blkmap_pmem_nodes(void *fdt, struct blkmap *bm)
>> +{
>> +       int ret;
>> +       u32 size;
>> +       ulong addr;
>> +       efi_status_t status;
>> +       struct blkmap_mem *bmm;
>> +       struct blkmap_slice *bms;
>> +       struct blk_desc *bd = dev_get_uclass_plat(bm->blk);
>> +
>> +       list_for_each_entry(bms, &bm->slices, node) {
>> +               if (bms->type != BLKMAP_SLICE_MEM)
>> +                       continue;
>
> Can we convert the 'type' to 'preserve' and teach
> blkmap_create_ramdisk() to pass that flag?
> This way we can unconditionally pass it from EFI HTTP installers, and
> let the command line users decide if they want to preserve it.

This seems like the most reasonable approach to me as well.

Then we could add a single API like this:

int blkmap_foreach_pmem_slice(int (*cb)(void *ctx, void *addr, size_t size),
                              void *ctx);

Rather than exporting all internal details of every slice's
implementation in blkmap.h.

I.e., let the blkmap code deal with how to locate the slices of
interest, and keep the internal details away from the consumer of the
data.

With that added to blkmap.c, I think the rest of this patch reduces to
something like:

int add_pmem_node(void *fdt, void *addr, size_t size)
{
        return fdt_fixup_pmem_region(fdt, (ulong)addr, size);
}

int fdt_efi_pmem_setup(void *fdt)
{
        return blkmap_foreach_pmem_slice(add_pmem_node, fdt);
}


>
>> +
>> +               bmm = container_of(bms, struct blkmap_mem, slice);
>> +
>> +               addr = (ulong)(uintptr_t)bmm->addr;
>> +               size = (u32)bms->blkcnt << bd->log2blksz;
>> +
>> +               ret = fdt_fixup_pmem_region(fdt, addr, size);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               status = efi_remove_memory_map(addr, size,
>> +                                              EFI_CONVENTIONAL_MEMORY);
>> +               if (status != EFI_SUCCESS)
>> +                       return -1;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>
>
> Thanks
> /Ilias