Re: [PATCH 0/4] introduce EFI_RAM_DISK_PROTOCOL

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Mon, Jul 10, 2023 at 11:13:12AM +0900, Masahisa Kojima wrote:
> On Fri, 7 Jul 2023 at 18:12, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Fri, Jul 07, 2023 at 05:19:33PM +0900, Masahisa Kojima wrote:
> > > Hi Akashi-san,
> > >
> > > On Fri, 7 Jul 2023 at 16:16, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Fri, Jul 07, 2023 at 08:29:12AM +0200, Heinrich Schuchardt wrote:
> > > > > On 7/7/23 06:00, Masahisa Kojima wrote:
> > > > > > This series introduces the EFI_RAM_DISK_PROTOCOL implementation.
> > > > > > The major purpose of this series is a preparation for EFI HTTP(S) boot.
> > > > > >
> > > > > > Now U-Boot can download the distro installer ISO image
> > > > > > via wget or tftpboot commands, but U-Boot can not mount
> > > > > > the downloaded ISO image.
> > > > > > By calling EFI_RAM_DISK_PROTOCOL->register API, user can
> > > > > > mount the ISO image and boot the distro installer.
> > > > >
> > > > > If I understand you correctly, with your design RAM disks will only
> > > > > exist in the EFI sub-system.
> > > >
> > > > Probably, not. As Kojima-san's "dm tree" shows, there is a U-Boot
> > > > block device (and associated partition devices) thanks to your
> > > > efi_driver framework.
> > >
> > > Read/Write the RAM disk is expected to be called from the EFI context, so
> >
> > An associated U-Boot block device should work as well on top of your
> > RAW_DISK_PROTOCOL via a generated RAM disk object (efi_disk).
> > That is why SYMPLE_FILE_SYSTEM_PROTOCOL, which solely relies on U-Boot's proper
> > filesystem subsystem, is installed to the RAM disk object.
> 
> I now realize that my RAM_DISK_PROTOCOL implementation happens to work
> thanks to the block cache.
> When I disable CONFIG_BLOCK_CACHE and load some EFI application(it
> does set  'app_gd')
> before calling RAM_DISK_PROTOCOL service, RAM_DISK_PROTOCOL does not work.
> ConnectController fails.
> 
> >
> > > native U-Boot can not access the RAM disk.
> >
> > I believe that, in theory, "fatls efidisk 0 1" (or efi_disk as an interface?)
> > should work even under your current implementation.
> 
> "fatls efiloader 0:2" does not work.
> I think "efiloader" device is designed to be accessed from EFI application only
> even if it is listed by "dm tree".

I don't believe so.
(the interface name may be "efi_blk" or "efiblk", though.)

If the command fails, it's a bug. Must be fixed.

> I understand that ramdisk as UCLASS_BLK driver is required as Heinrich said.

That said, I agree to starting with UCLASS_BLK from the viewpoint of
U-Boot driver model/UEFI integration.

-Takahiro Akashi

> 
> Thanks,
> Masahisa Kojima
> 
> >
> > -Takahiro Akashi
> >
> > >  # Honestly speaking, I'm not sure how U-Boot prohibits the access to
> > > the EFI RAM disk
> > >     because the udevices are created for the RAM disk.
> > >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > > >
> > > > > We strive to synchronize what is happening on the driver model level and
> > > > > on the UEFI level. I would prefer a design where we have a UCLASS_BLK
> > > > > driver ram disks and the EFI_RAM_DISK_PROTOCOL is a one method of
> > > > > managing ram disk devices.
> > > >
> > > > That said, I agree to starting with U-Boot block device implementation,
> > > > UEFI_DISK comes automatically. It benefits both U-Boot proper and
> > > > UEFI subsystem equally as well.
> > > > (That is also what I meant to say in my first response.)
> > > >
> > > > > A big issue we have is RAM management. malloc() can only assign limited
> > > > > amount of memory which is probably too small for the RAM disk you are
> > > > > looking at. We manage page sized memory in the EFI sub-system but this
> > > > > is not integrated with the LMB memory checks.
> > > >
> > > > Not sure, is it enough simply to add some restrictions on the start size
> > > > and the size when a memory region is specified for a raw disk?
> > > >
> > > > -Takahiro Akashi
> > > >
> > > > > The necessary sequence of development is probably:
> > > > >
> > > > > * Rework memory management
> > > > > * Implement ramdisks as UCLASS_BLK driver
> > > > > * Implement the EFI_RAM_DISK_PROTOCOL based on the UCLASS_BLK driver.
> > > > >
> > > > > Best regards
> > > > >
> > > > > Heinrich
> > > > >
> > > > > >
> > > > > > Note that the installation process may not proceed
> > > > > > after the distro installer calls ExitBootServices()
> > > > > > since there is no hand-off process for the block device of
> > > > > > memory mapped ISO image.
> > > > > >
> > > > > > The EFI_RAM_DISK_PROTOCOL was tested with the
> > > > > > debian network installer[1] on qemu_arm64 platform.
> > > > > > The example procedure is as follows.
> > > > > >   => tftpboot 41000000 mini.iso
> > > > > >   => efidebug disk load 41000000 $filesize
> > > > > >
> > > > > > After these commands, ISO image is mounted like:
> > > > > >
> > > > > >   => efidebug dh
> > > > > >
> > > > > >      000000007eec5910 (efiblk#0)
> > > > > >        /RamDisk(0x41000000,4974afff,3d5abd30-4175-87ce-6d64-d2ade523c4bb,0x0)
> > > > > >        Block IO
> > > > > >
> > > > > >      000000007eec6140 (efiblk#0:1)
> > > > > >        /RamDisk(0x41000000,4974afff,3d5abd30-4175-87ce-6d64-d2ade523c4bb,0x0)/HD(1,0x01,0,0x0,0x33800)
> > > > > >        Block IO
> > > > > >
> > > > > >      000000007eec62b0 (efiblk#0:2)
> > > > > >        /RamDisk(0x41000000,4974afff,3d5abd30-4175-87ce-6d64-d2ade523c4bb,0x0)/HD(2,0x01,0,0x33800,0x10000)
> > > > > >        Block IO
> > > > > >        System Partition
> > > > > >        Simple File System
> > > > > >
> > > > > >    => dm tree
> > > > > >
> > > > > >      blk           0  [ + ]   efi_blk               `-- efiblk#0
> > > > > >      partition     0  [ + ]   blk_partition             |-- efiblk#0:1
> > > > > >      partition     1  [ + ]   blk_partition             `-- efiblk#0:2
> > > > > >
> > > > > > Debian can be successfully installed with this RAM disk on QEMU.
> > > > > >
> > > > > > [TODO]
> > > > > > - udevices created in ./lib/efi_driver/efi_block_device.c::efi_bl_bind()
> > > > > >    are not removed when the efi_handle is removed.
> > > > > >    So after unload the RAM disk, udevices still exist.
> > > > > >    I plan to add a udevice removal process in ./lib/efi_driver/efi_uclass.c::efi_uc_stop().
> > > > > >    In addition, I also plan to add unbind() callback in struct efi_driver_ops.
> > > > > >
> > > > > >
> > > > > > [1] https://deb.debian.org/debian/dists/bookworm/main/installer-arm64/current/images/netboot/mini.iso
> > > > > >
> > > > > > Masahisa Kojima (4):
> > > > > >    efi_loader: add RAM disk device path
> > > > > >    efi_loader: add EFI_RAM_DISK_PROTOCOL implementation
> > > > > >    cmd: efidebug: add RAM disk mount command
> > > > > >    efi_selftest: add EFI_RAM_DISK_PROTOCOL selftest
> > > > > >
> > > > > >   cmd/efidebug.c                           | 117 ++++++
> > > > > >   include/efi_api.h                        |  32 ++
> > > > > >   include/efi_loader.h                     |   4 +
> > > > > >   lib/efi_driver/efi_uclass.c              |   7 +-
> > > > > >   lib/efi_loader/Kconfig                   |   6 +
> > > > > >   lib/efi_loader/Makefile                  |   1 +
> > > > > >   lib/efi_loader/efi_device_path_to_text.c |  14 +
> > > > > >   lib/efi_loader/efi_ram_disk.c            | 334 +++++++++++++++
> > > > > >   lib/efi_loader/efi_setup.c               |   6 +
> > > > > >   lib/efi_selftest/Makefile                |   1 +
> > > > > >   lib/efi_selftest/efi_selftest_ram_disk.c | 511 +++++++++++++++++++++++
> > > > > >   lib/uuid.c                               |   4 +
> > > > > >   12 files changed, 1035 insertions(+), 2 deletions(-)
> > > > > >   create mode 100644 lib/efi_loader/efi_ram_disk.c
> > > > > >   create mode 100644 lib/efi_selftest/efi_selftest_ram_disk.c
> > > > > >
> > > > > >
> > > > > > base-commit: e2e2aea5733f0d23cd9593bbefe5c803c552dcb9
> > > > >