[PATCH] efi_loader: eliminate efi_disk_obj structure

[Masahisa Kojima]

Hi Heinrich,

On Sun, 17 Dec 2023 at 19:26, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 12/14/23 09:23, Masahisa Kojima wrote:
> >>>> 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.
> >>> This patch tries to address this issue[0].
> >>>
> >>> If I understand correctly, two options are suggested here.
> >>> 1) a private field for the handle appended to struct efi_block_io
> >>> 2) keep the private struct something like current struct
> >>> efi_disk_obj, same as EDK II does
> >> Edk II uses 1) as I indicated above.
> > Probably I misunderstand your suggestion, let me clarify again.
> >
> > EDK II RamDisk implementation defines the private structure
> > RAM_DISK_PRIVATE_DATA[1].
> > RAM_DISK_PRIVATE_FROM_BLKIO macro returns the pointer to the
> > RAM_DISK_PRIVATE_DATA structure from the BLKIO protocol interface.
> > EFI_BLOCK_IO_PROTOCOL does not have a private field.
> >
> > It is the same as the current U-Boot implementation of efi_disk.c using
> > struct efi_disk_obj and following code.
> >    diskobj = container_of(this, struct efi_disk_obj, ops);
> >
> > Do you suggest modifying struct efi_block_io to add private fields?
>
> efi_block_io currently is what is defined in the UEFI specification.
>
> We could define a new structure that includes efi_block_io and
> additional private fields:
>
> struct efi_block_io_plus_private {
>         struct efi_block_io block_io;
>         efi_handle_t handle;
> }
>
> Or we can change the definition of efi_block_io by adding private fields:
>
> struct efi_block_io {
>          u64 revision;
>          struct efi_block_io_media *media;
> ...
>          efi_status_t (EFIAPI *flush_blocks)(struct efi_block_io *this);
>          # U-Boot private fields start here
>         efi_handle_t handle;
> };
>
> In either case we must not try to access the private fields if the
> structure is not allocated by us.
>
> The second option will require less code changes.
>
> I would try to avoid using container_of() for accessing private fields
> as it static code analysis and reading the code more difficult.

Thank you for the detailed explanation.

Avoiding container_of() means using cast instead?
Probably we define the following struct, cast the pointer of struct efi_block_io
to struct efi_block_io_plus_private pointer and check the signature before use.
struct efi_block_io_plus_private {
        struct efi_block_io block_io;
        u32 signature;
        efi_handle_t handle;
}

I still hesitate to modify struct efi_block_io.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > [1]https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h#L95
> >
> > Thanks,
> > Masahisa Kojima
>