Hi Heinrich, On Sun, 17 Dec 2023 at 19:26, Heinrich Schuchardt <xypron.g...@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 >