Hi Heinrich,
On Sun, 17 Dec 2023 at 19:26, Heinrich Schuchardt xypron.glpk@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.
- a private field for the handle appended to struct efi_block_io
- 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/Ra...
Thanks, Masahisa Kojima