[PATCH] efi_loader: eliminate efi_disk_obj structure

[Simon Glass]

Hi Heinrich,

On Wed, 13 Dec 2023 at 07:23, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 13.12.23 09:57, Masahisa Kojima wrote:
> > Current code uses struct efi_disk_obj to keep information
> > about block devices and partitions. As the efi handle already
> > has a field with the udevice, we should eliminate
> > struct efi_disk_obj and use an pointer to struct efi_object
> > for the handle.
> >
> > efi_link_dev() call is moved inside of efi_disk_add_dev() function
> > to simplify the cleanup process in case of error.
>
> I agree that using struct efi_disk_obj as a container for protocols of a
> block IO device is a bit messy.
>
> But we should keep looking up the handle easy. Currently we use
>
> diskobj = container_of(this, struct efi_disk_obj, ops);
>
> Instead we could append a private field with the handle to the
> EFI_BLOCK_IO_PROTOCOL structure.
>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> >   lib/efi_loader/efi_disk.c | 209 +++++++++++++++++++++-----------------
> >   1 file changed, 116 insertions(+), 93 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index f0d76113b0..cfb7ace551 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -27,27 +27,24 @@ struct efi_system_partition efi_system_partition = {
> >   const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> >   const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID;
> >
> > -/**
> > - * struct efi_disk_obj - EFI disk object
> > - *
> > - * @header:  EFI object header
> > - * @ops:     EFI disk I/O protocol interface
> > - * @dev_index:       device index of block device
> > - * @media:   block I/O media information
> > - * @dp:              device path to the block device
> > - * @part:    partition
> > - * @volume:  simple file system protocol of the partition
> > - * @dev:     associated DM device
> > - */
> > -struct efi_disk_obj {
> > -     struct efi_object header;
> > -     struct efi_block_io ops;
> > -     int dev_index;
> > -     struct efi_block_io_media media;
> > -     struct efi_device_path *dp;
> > -     unsigned int part;
> > -     struct efi_simple_file_system_protocol *volume;
> > -};
> > +static efi_handle_t efi_blkio_find_obj(struct efi_block_io *blkio)
> > +{
> > +     efi_handle_t handle;
> > +
> > +     list_for_each_entry(handle, &efi_obj_list, link) {
> > +             efi_status_t ret;
> > +             struct efi_handler *handler;
> > +
> > +             ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
> > +             if (ret != EFI_SUCCESS)
> > +                     continue;
> > +
> > +             if (blkio == handler->protocol_interface)
> > +                     return handle;
> > +     }
>
> Depending on the number of handles and pointers this will take a
> considerable time. A private field for the handle appended to struct
> efi_block_io would allow a fast lookup.
>
> EDK II does the same. See the definition of RAM_DISK_PRIVATE_FROM_BLKIO
> which uses macro BASE_CR() to find the private fields.

That seems pretty ugly to me, but if it really is that slow, I suppose it is OK.

Can we attach efi_block_io to the driver model BLK device?

Regards,
Simon