[PATCH] efi_loader: eliminate efi_disk_obj structure
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.
Signed-off-by: Masahisa Kojima masahisa.kojima@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; + } + + return NULL; +}
/** * efi_disk_reset() - reset block device @@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, u32 media_id, u64 lba, unsigned long buffer_size, void *buffer, enum efi_disk_direction direction) { - struct efi_disk_obj *diskobj; + efi_handle_t handle; int blksz; int blocks; unsigned long n;
- diskobj = container_of(this, struct efi_disk_obj, ops); - blksz = diskobj->media.block_size; + handle = efi_blkio_find_obj(this); + if (!handle) + return EFI_INVALID_PARAMETER; + + blksz = this->media->block_size; blocks = buffer_size / blksz;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", @@ -124,18 +124,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (CONFIG_IS_ENABLED(PARTITIONS) && - device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) { + device_get_uclass_id(handle->dev) == UCLASS_PARTITION) { if (direction == EFI_DISK_READ) - n = disk_blk_read(diskobj->header.dev, lba, blocks, - buffer); + n = disk_blk_read(handle->dev, lba, blocks, buffer); else - n = disk_blk_write(diskobj->header.dev, lba, blocks, - buffer); + n = disk_blk_write(handle->dev, lba, blocks, buffer); } else { /* dev is a block device (UCLASS_BLK) */ struct blk_desc *desc;
- desc = dev_get_uclass_plat(diskobj->header.dev); + desc = dev_get_uclass_plat(handle->dev); if (direction == EFI_DISK_READ) n = blk_dread(desc, lba, blocks, buffer); else @@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part) * @part: partition * @disk: pointer to receive the created handle * @agent_handle: handle of the EFI block driver + * @dev: pointer to udevice * Return: disk object */ static efi_status_t efi_disk_add_dev( @@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev( int dev_index, struct disk_partition *part_info, unsigned int part, - struct efi_disk_obj **disk, - efi_handle_t agent_handle) + efi_handle_t *disk, + efi_handle_t agent_handle, + struct udevice *dev) { - struct efi_disk_obj *diskobj; - struct efi_object *handle; + efi_handle_t handle; const efi_guid_t *esp_guid = NULL; efi_status_t ret; + struct efi_block_io *blkio; + struct efi_block_io_media *media; + struct efi_device_path *dp = NULL; + struct efi_simple_file_system_protocol *volume = NULL;
/* Don't add empty devices */ if (!desc->lba) return EFI_NOT_READY;
- diskobj = calloc(1, sizeof(*diskobj)); - if (!diskobj) + ret = efi_create_handle(&handle); + if (ret != EFI_SUCCESS) + return ret; + + blkio = calloc(1, sizeof(*blkio)); + media = calloc(1, sizeof(*media)); + if (!blkio || !media) return EFI_OUT_OF_RESOURCES;
- /* Hook up to the device list */ - efi_add_handle(&diskobj->header); + *blkio = block_io_disk_template; + blkio->media = media;
/* Fill in object data */ if (part_info) { @@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev( * (controller). */ ret = efi_protocol_open(handler, &protocol_interface, NULL, - &diskobj->header, + handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER); if (ret != EFI_SUCCESS) { log_debug("prot open failed\n"); goto error; }
- diskobj->dp = efi_dp_append_node(dp_parent, node); + dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->media.last_block = part_info->size - 1; + blkio->media->last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) esp_guid = &efi_system_partition_guid; } else { - diskobj->dp = efi_dp_from_part(desc, part); - diskobj->media.last_block = desc->lba - 1; + dp = efi_dp_from_part(desc, part); + blkio->media->last_block = desc->lba - 1; } - diskobj->part = part;
/* * Install the device path and the block IO protocol. @@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev( * already installed on an other handle and returns EFI_ALREADY_STARTED * in this case. */ - handle = &diskobj->header; ret = efi_install_multiple_protocol_interfaces( &handle, - &efi_guid_device_path, diskobj->dp, - &efi_block_io_guid, &diskobj->ops, + &efi_guid_device_path, dp, + &efi_block_io_guid, blkio, /* * esp_guid must be last entry as it * can be NULL. Its interface is NULL. @@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev( */ if ((part || desc->part_type == PART_TYPE_UNKNOWN) && efi_fs_exists(desc, part)) { - ret = efi_create_simple_file_system(desc, part, diskobj->dp, - &diskobj->volume); + ret = efi_create_simple_file_system(desc, part, dp, &volume); if (ret != EFI_SUCCESS) goto error;
- ret = efi_add_protocol(&diskobj->header, + ret = efi_add_protocol(handle, &efi_simple_file_system_protocol_guid, - diskobj->volume); + volume); if (ret != EFI_SUCCESS) goto error; } - diskobj->ops = block_io_disk_template; - diskobj->dev_index = dev_index;
/* Fill in EFI IO Media info (for read/write callbacks) */ - diskobj->media.removable_media = desc->removable; - diskobj->media.media_present = 1; + blkio->media->removable_media = desc->removable; + blkio->media->media_present = 1; /* * MediaID is just an arbitrary counter. * We have to change it if the medium is removed or changed. */ - diskobj->media.media_id = 1; - diskobj->media.block_size = desc->blksz; - diskobj->media.io_align = desc->blksz; + blkio->media->media_id = 1; + blkio->media->block_size = desc->blksz; + blkio->media->io_align = desc->blksz; if (part) - diskobj->media.logical_partition = 1; - diskobj->ops.media = &diskobj->media; + blkio->media->logical_partition = 1; if (disk) - *disk = diskobj; + *disk = handle;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n", - diskobj->part, - diskobj->media.media_present, - diskobj->media.logical_partition, - diskobj->media.removable_media, - diskobj->media.last_block); + part, + blkio->media->media_present, + blkio->media->logical_partition, + blkio->media->removable_media, + blkio->media->last_block);
/* Store first EFI system partition */ if (part && efi_system_partition.uclass_id == UCLASS_INVALID) { @@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev( desc->devnum, part); } } + + if (efi_link_dev(handle, dev)) + goto error; + return EFI_SUCCESS; error: - efi_delete_handle(&diskobj->header); - free(diskobj->volume); - free(diskobj); + efi_delete_handle(handle); + efi_free_pool(dp); + free(blkio); + free(media); + free(volume); + return ret; }
@@ -557,7 +566,7 @@ error: */ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) { - struct efi_disk_obj *disk; + efi_handle_t disk; struct blk_desc *desc; int diskid; efi_status_t ret; @@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum;
ret = efi_disk_add_dev(NULL, NULL, desc, - diskid, NULL, 0, &disk, agent_handle); + diskid, NULL, 0, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { if (ret == EFI_NOT_READY) { log_notice("Disk %s not ready\n", dev->name); @@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
return ret; } - if (efi_link_dev(&disk->header, dev)) { - efi_free_pool(disk->dp); - efi_delete_handle(&disk->header); - - return -EINVAL; - }
return 0; } @@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) int diskid; struct efi_handler *handler; struct efi_device_path *dp_parent; - struct efi_disk_obj *disk; + efi_handle_t disk; efi_status_t ret;
if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent)) @@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) dp_parent = (struct efi_device_path *)handler->protocol_interface;
ret = efi_disk_add_dev(parent, dp_parent, desc, diskid, - info, part, &disk, agent_handle); + info, part, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { log_err("Adding partition for %s failed\n", dev->name); return -1; } - if (efi_link_dev(&disk->header, dev)) { - efi_free_pool(disk->dp); - efi_delete_handle(&disk->header); - - return -1; - }
return 0; } @@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event) return 0; }
+static void get_disk_resources(efi_handle_t handle, struct efi_device_path **dp, + struct efi_block_io **blkio, + struct efi_simple_file_system_protocol **volume) +{ + efi_status_t ret; + struct efi_handler *handler; + + ret = efi_search_protocol(handle, &efi_guid_device_path, &handler); + if (ret == EFI_SUCCESS) + *dp = handler->protocol_interface; + + ret = efi_search_protocol(handle, &efi_block_io_guid, &handler); + if (ret == EFI_SUCCESS) + *blkio = handler->protocol_interface; + + ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid, + &handler); + if (ret == EFI_SUCCESS) + *volume = handler->protocol_interface; +} + /** - * efi_disk_remove - delete an efi_disk object for a block device or partition + * efi_disk_remove - delete an efi handle for a block device or partition * * @ctx: event context: driver binding protocol * @event: EV_PM_PRE_REMOVE event * - * Delete an efi_disk object which is associated with the UCLASS_BLK or + * Delete an efi handle which is associated with the UCLASS_BLK or * UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised. * * Return: 0 on success, -1 otherwise @@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc; - struct efi_disk_obj *diskobj = NULL; efi_status_t ret; + struct efi_device_path *dp = NULL; + struct efi_block_io *blkio = NULL; + struct efi_simple_file_system_protocol *volume = NULL;
if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0; @@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER) - diskobj = container_of(handle, struct efi_disk_obj, - header); + get_disk_resources(handle, &dp, &blkio, &volume); + break; case UCLASS_PARTITION: - diskobj = container_of(handle, struct efi_disk_obj, header); + get_disk_resources(handle, &dp, &blkio, &volume); break; default: return 0; @@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event) if (ret != EFI_SUCCESS) return -1;
- if (diskobj) - efi_free_pool(diskobj->dp); + efi_free_pool(dp); + if (blkio) { + free(blkio->media); + free(blkio); + }
dev_tag_del(dev, DM_TAG_EFI);
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@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.
Best regards
Heinrich
- return NULL;
+}
/**
- efi_disk_reset() - reset block device
@@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, u32 media_id, u64 lba, unsigned long buffer_size, void *buffer, enum efi_disk_direction direction) {
- struct efi_disk_obj *diskobj;
- efi_handle_t handle; int blksz; int blocks; unsigned long n;
- diskobj = container_of(this, struct efi_disk_obj, ops);
- blksz = diskobj->media.block_size;
handle = efi_blkio_find_obj(this);
if (!handle)
return EFI_INVALID_PARAMETER;blksz = this->media->block_size; blocks = buffer_size / blksz;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",
@@ -124,18 +124,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (CONFIG_IS_ENABLED(PARTITIONS) &&
device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) {
device_get_uclass_id(handle->dev) == UCLASS_PARTITION) {
n = disk_blk_read(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_read(handle->dev, lba, blocks, buffer);
n = disk_blk_write(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_write(handle->dev, lba, blocks, buffer);
desc = dev_get_uclass_plat(diskobj->header.dev);
desc = dev_get_uclass_plat(handle->dev);@@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part)
- @part: partition
- @disk: pointer to receive the created handle
- @agent_handle: handle of the EFI block driver
*/ static efi_status_t efi_disk_add_dev(
- @dev: pointer to udevice
- Return: disk object
@@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev( int dev_index, struct disk_partition *part_info, unsigned int part,
struct efi_disk_obj **disk,efi_handle_t agent_handle)
efi_handle_t *disk,efi_handle_t agent_handle,struct udevice *dev)
- struct efi_disk_obj *diskobj;
- struct efi_object *handle;
efi_handle_t handle; const efi_guid_t *esp_guid = NULL; efi_status_t ret;
struct efi_block_io *blkio;
struct efi_block_io_media *media;
struct efi_device_path *dp = NULL;
struct efi_simple_file_system_protocol *volume = NULL;
/* Don't add empty devices */ if (!desc->lba) return EFI_NOT_READY;
- diskobj = calloc(1, sizeof(*diskobj));
- if (!diskobj)
- ret = efi_create_handle(&handle);
- if (ret != EFI_SUCCESS)
return ret;- blkio = calloc(1, sizeof(*blkio));
- media = calloc(1, sizeof(*media));
- if (!blkio || !media) return EFI_OUT_OF_RESOURCES;
- /* Hook up to the device list */
- efi_add_handle(&diskobj->header);
*blkio = block_io_disk_template;
blkio->media = media;
/* Fill in object data */ if (part_info) {
@@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev( * (controller). */ ret = efi_protocol_open(handler, &protocol_interface, NULL,
&diskobj->header,
handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
diskobj->dp = efi_dp_append_node(dp_parent, node);
dp = efi_dp_append_node(dp_parent, node);
diskobj->media.last_block = part_info->size - 1;
blkio->media->last_block = part_info->size - 1;
diskobj->dp = efi_dp_from_part(desc, part);diskobj->media.last_block = desc->lba - 1;
dp = efi_dp_from_part(desc, part);blkio->media->last_block = desc->lba - 1;
diskobj->part = part;
/*
- Install the device path and the block IO protocol.
@@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev( * already installed on an other handle and returns EFI_ALREADY_STARTED * in this case. */
- handle = &diskobj->header; ret = efi_install_multiple_protocol_interfaces( &handle,
&efi_guid_device_path, diskobj->dp,&efi_block_io_guid, &diskobj->ops,
&efi_guid_device_path, dp,&efi_block_io_guid, blkio, /* * esp_guid must be last entry as it * can be NULL. Its interface is NULL.@@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev( */ if ((part || desc->part_type == PART_TYPE_UNKNOWN) && efi_fs_exists(desc, part)) {
ret = efi_create_simple_file_system(desc, part, diskobj->dp,&diskobj->volume);
ret = efi_create_simple_file_system(desc, part, dp, &volume);
ret = efi_add_protocol(&diskobj->header,
ret = efi_add_protocol(handle, &efi_simple_file_system_protocol_guid,
diskobj->volume);
volume);
diskobj->ops = block_io_disk_template;
diskobj->dev_index = dev_index;
/* Fill in EFI IO Media info (for read/write callbacks) */
diskobj->media.removable_media = desc->removable;
diskobj->media.media_present = 1;
- blkio->media->removable_media = desc->removable;
- blkio->media->media_present = 1; /*
*/
- MediaID is just an arbitrary counter.
- We have to change it if the medium is removed or changed.
- diskobj->media.media_id = 1;
- diskobj->media.block_size = desc->blksz;
- diskobj->media.io_align = desc->blksz;
- blkio->media->media_id = 1;
- blkio->media->block_size = desc->blksz;
- blkio->media->io_align = desc->blksz; if (part)
diskobj->media.logical_partition = 1;- diskobj->ops.media = &diskobj->media;
blkio->media->logical_partition = 1;
*disk = diskobj;
*disk = handle;EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n",
diskobj->part,diskobj->media.media_present,diskobj->media.logical_partition,diskobj->media.removable_media,diskobj->media.last_block);
part,blkio->media->media_present,blkio->media->logical_partition,blkio->media->removable_media,blkio->media->last_block);/* Store first EFI system partition */ if (part && efi_system_partition.uclass_id == UCLASS_INVALID) {
@@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev( desc->devnum, part); } }
- if (efi_link_dev(handle, dev))
goto error;- return EFI_SUCCESS; error:
- efi_delete_handle(&diskobj->header);
- free(diskobj->volume);
- free(diskobj);
- efi_delete_handle(handle);
- efi_free_pool(dp);
- free(blkio);
- free(media);
- free(volume);
- return ret; }
@@ -557,7 +566,7 @@ error: */ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) {
- struct efi_disk_obj *disk;
- efi_handle_t disk; struct blk_desc *desc; int diskid; efi_status_t ret;
@@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum;
ret = efi_disk_add_dev(NULL, NULL, desc,
diskid, NULL, 0, &disk, agent_handle);
diskid, NULL, 0, &disk, agent_handle, dev);@@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
return ret;}
if (efi_link_dev(&disk->header, dev)) {
efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -EINVAL;}
return 0; }
@@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) int diskid; struct efi_handler *handler; struct efi_device_path *dp_parent;
- struct efi_disk_obj *disk;
efi_handle_t disk; efi_status_t ret;
if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
@@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) dp_parent = (struct efi_device_path *)handler->protocol_interface;
ret = efi_disk_add_dev(parent, dp_parent, desc, diskid,
info, part, &disk, agent_handle);
info, part, &disk, agent_handle, dev);
if (efi_link_dev(&disk->header, dev)) {
efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -1;}
return 0; }
@@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event) return 0; }
+static void get_disk_resources(efi_handle_t handle, struct efi_device_path **dp,
struct efi_block_io **blkio,struct efi_simple_file_system_protocol **volume)+{
- efi_status_t ret;
- struct efi_handler *handler;
- ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
- if (ret == EFI_SUCCESS)
*dp = handler->protocol_interface;- ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
- if (ret == EFI_SUCCESS)
*blkio = handler->protocol_interface;- ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid,
&handler);- if (ret == EFI_SUCCESS)
*volume = handler->protocol_interface;+}
- /**
- efi_disk_remove - delete an efi_disk object for a block device or partition
- efi_disk_remove - delete an efi handle for a block device or partition
- @ctx: event context: driver binding protocol
- @event: EV_PM_PRE_REMOVE event
- Delete an efi_disk object which is associated with the UCLASS_BLK or
- Delete an efi handle which is associated with the UCLASS_BLK or
- UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised.
- Return: 0 on success, -1 otherwise
@@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc;
- struct efi_disk_obj *diskobj = NULL; efi_status_t ret;
struct efi_device_path *dp = NULL;
struct efi_block_io *blkio = NULL;
struct efi_simple_file_system_protocol *volume = NULL;
if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0;
@@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
diskobj = container_of(handle, struct efi_disk_obj,header);
get_disk_resources(handle, &dp, &blkio, &volume);- break; case UCLASS_PARTITION:
diskobj = container_of(handle, struct efi_disk_obj, header);
get_disk_resources(handle, &dp, &blkio, &volume);@@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event) if (ret != EFI_SUCCESS) return -1;
- if (diskobj)
efi_free_pool(diskobj->dp);
efi_free_pool(dp);
if (blkio) {
free(blkio->media);free(blkio);}
dev_tag_del(dev, DM_TAG_EFI);
Hi Heinrich,
On Wed, 13 Dec 2023 at 07:23, Heinrich Schuchardt xypron.glpk@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@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
Hi Heinrich,
On Wed, 13 Dec 2023 at 23:22, Heinrich Schuchardt xypron.glpk@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@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.
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
struct efi_block_io is defined in UEFI spec, so probably option#2 is preferable and it is almost the same as the current implementation. I think we can proceed with the minor cleanup of struct efi_disk_obj as Akashi-san suggested.
[0] https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/9
Thanks, Masahisa Kojima
Best regards
Heinrich
return NULL;+}
/**
- efi_disk_reset() - reset block device
@@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, u32 media_id, u64 lba, unsigned long buffer_size, void *buffer, enum efi_disk_direction direction) {
struct efi_disk_obj *diskobj;
efi_handle_t handle; int blksz; int blocks; unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops);blksz = diskobj->media.block_size;
handle = efi_blkio_find_obj(this);if (!handle)return EFI_INVALID_PARAMETER;blksz = this->media->block_size; blocks = buffer_size / blksz; EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",@@ -124,18 +124,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (CONFIG_IS_ENABLED(PARTITIONS) &&
device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) {
device_get_uclass_id(handle->dev) == UCLASS_PARTITION) { if (direction == EFI_DISK_READ)
n = disk_blk_read(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_read(handle->dev, lba, blocks, buffer); else
n = disk_blk_write(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_write(handle->dev, lba, blocks, buffer); } else { /* dev is a block device (UCLASS_BLK) */ struct blk_desc *desc;
desc = dev_get_uclass_plat(diskobj->header.dev);
desc = dev_get_uclass_plat(handle->dev); if (direction == EFI_DISK_READ) n = blk_dread(desc, lba, blocks, buffer); else@@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part)
- @part: partition
- @disk: pointer to receive the created handle
- @agent_handle: handle of the EFI block driver
*/ static efi_status_t efi_disk_add_dev(
- @dev: pointer to udevice
- Return: disk object
@@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev( int dev_index, struct disk_partition *part_info, unsigned int part,
struct efi_disk_obj **disk,efi_handle_t agent_handle)
efi_handle_t *disk,efi_handle_t agent_handle,struct udevice *dev)
struct efi_disk_obj *diskobj;struct efi_object *handle;
efi_handle_t handle; const efi_guid_t *esp_guid = NULL; efi_status_t ret;struct efi_block_io *blkio;struct efi_block_io_media *media;struct efi_device_path *dp = NULL;struct efi_simple_file_system_protocol *volume = NULL; /* Don't add empty devices */ if (!desc->lba) return EFI_NOT_READY;
diskobj = calloc(1, sizeof(*diskobj));if (!diskobj)
ret = efi_create_handle(&handle);if (ret != EFI_SUCCESS)return ret;blkio = calloc(1, sizeof(*blkio));media = calloc(1, sizeof(*media));if (!blkio || !media) return EFI_OUT_OF_RESOURCES;
/* Hook up to the device list */efi_add_handle(&diskobj->header);
*blkio = block_io_disk_template;blkio->media = media; /* Fill in object data */ if (part_info) {@@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev( * (controller). */ ret = efi_protocol_open(handler, &protocol_interface, NULL,
&diskobj->header,
handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER); if (ret != EFI_SUCCESS) { log_debug("prot open failed\n"); goto error; }
diskobj->dp = efi_dp_append_node(dp_parent, node);
dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node);
diskobj->media.last_block = part_info->size - 1;
blkio->media->last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) esp_guid = &efi_system_partition_guid; } else {
diskobj->dp = efi_dp_from_part(desc, part);diskobj->media.last_block = desc->lba - 1;
dp = efi_dp_from_part(desc, part);blkio->media->last_block = desc->lba - 1; }
diskobj->part = part; /* * Install the device path and the block IO protocol.@@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev( * already installed on an other handle and returns EFI_ALREADY_STARTED * in this case. */
handle = &diskobj->header; ret = efi_install_multiple_protocol_interfaces( &handle,&efi_guid_device_path, diskobj->dp,&efi_block_io_guid, &diskobj->ops,
&efi_guid_device_path, dp,&efi_block_io_guid, blkio, /* * esp_guid must be last entry as it * can be NULL. Its interface is NULL.@@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev( */ if ((part || desc->part_type == PART_TYPE_UNKNOWN) && efi_fs_exists(desc, part)) {
ret = efi_create_simple_file_system(desc, part, diskobj->dp,&diskobj->volume);
ret = efi_create_simple_file_system(desc, part, dp, &volume); if (ret != EFI_SUCCESS) goto error;
ret = efi_add_protocol(&diskobj->header,
ret = efi_add_protocol(handle, &efi_simple_file_system_protocol_guid,
diskobj->volume);
volume); if (ret != EFI_SUCCESS) goto error; }
diskobj->ops = block_io_disk_template;diskobj->dev_index = dev_index; /* Fill in EFI IO Media info (for read/write callbacks) */diskobj->media.removable_media = desc->removable;diskobj->media.media_present = 1;
blkio->media->removable_media = desc->removable;blkio->media->media_present = 1; /* * MediaID is just an arbitrary counter. * We have to change it if the medium is removed or changed. */
diskobj->media.media_id = 1;diskobj->media.block_size = desc->blksz;diskobj->media.io_align = desc->blksz;
blkio->media->media_id = 1;blkio->media->block_size = desc->blksz;blkio->media->io_align = desc->blksz; if (part)
diskobj->media.logical_partition = 1;diskobj->ops.media = &diskobj->media;
blkio->media->logical_partition = 1; if (disk)
*disk = diskobj;
*disk = handle; EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n",
diskobj->part,diskobj->media.media_present,diskobj->media.logical_partition,diskobj->media.removable_media,diskobj->media.last_block);
part,blkio->media->media_present,blkio->media->logical_partition,blkio->media->removable_media,blkio->media->last_block); /* Store first EFI system partition */ if (part && efi_system_partition.uclass_id == UCLASS_INVALID) {@@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev( desc->devnum, part); } }
if (efi_link_dev(handle, dev))goto error;return EFI_SUCCESS;
efi_delete_handle(&diskobj->header);free(diskobj->volume);free(diskobj);
efi_delete_handle(handle);efi_free_pool(dp);free(blkio);free(media);free(volume);return ret;@@ -557,7 +566,7 @@ error: */ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) {
struct efi_disk_obj *disk;
efi_handle_t disk; struct blk_desc *desc; int diskid; efi_status_t ret;@@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum;
ret = efi_disk_add_dev(NULL, NULL, desc,
diskid, NULL, 0, &disk, agent_handle);
diskid, NULL, 0, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { if (ret == EFI_NOT_READY) { log_notice("Disk %s not ready\n", dev->name);@@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
return ret; }
if (efi_link_dev(&disk->header, dev)) {efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -EINVAL;} return 0;}
@@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) int diskid; struct efi_handler *handler; struct efi_device_path *dp_parent;
struct efi_disk_obj *disk;
efi_handle_t disk; efi_status_t ret; if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))@@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) dp_parent = (struct efi_device_path *)handler->protocol_interface;
ret = efi_disk_add_dev(parent, dp_parent, desc, diskid,
info, part, &disk, agent_handle);
info, part, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { log_err("Adding partition for %s failed\n", dev->name); return -1; }
if (efi_link_dev(&disk->header, dev)) {efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -1;} return 0;}
@@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event) return 0; }
+static void get_disk_resources(efi_handle_t handle, struct efi_device_path **dp,
struct efi_block_io **blkio,struct efi_simple_file_system_protocol **volume)+{
efi_status_t ret;struct efi_handler *handler;ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);if (ret == EFI_SUCCESS)*dp = handler->protocol_interface;ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);if (ret == EFI_SUCCESS)*blkio = handler->protocol_interface;ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid,&handler);if (ret == EFI_SUCCESS)*volume = handler->protocol_interface;+}
- /**
- efi_disk_remove - delete an efi_disk object for a block device or partition
- efi_disk_remove - delete an efi handle for a block device or partition
- @ctx: event context: driver binding protocol
- @event: EV_PM_PRE_REMOVE event
- Delete an efi_disk object which is associated with the UCLASS_BLK or
- Delete an efi handle which is associated with the UCLASS_BLK or
- UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised.
- Return: 0 on success, -1 otherwise
@@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc;
struct efi_disk_obj *diskobj = NULL; efi_status_t ret;
struct efi_device_path *dp = NULL;struct efi_block_io *blkio = NULL;struct efi_simple_file_system_protocol *volume = NULL; if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0;@@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
diskobj = container_of(handle, struct efi_disk_obj,header);
get_disk_resources(handle, &dp, &blkio, &volume);break; case UCLASS_PARTITION:
diskobj = container_of(handle, struct efi_disk_obj, header);
get_disk_resources(handle, &dp, &blkio, &volume); break; default: return 0;@@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event) if (ret != EFI_SUCCESS) return -1;
if (diskobj)efi_free_pool(diskobj->dp);
efi_free_pool(dp);if (blkio) {free(blkio->media);free(blkio);} dev_tag_del(dev, DM_TAG_EFI);
Am 14. Dezember 2023 02:55:27 MEZ schrieb Masahisa Kojima masahisa.kojima@linaro.org:
Hi Heinrich,
On Wed, 13 Dec 2023 at 23:22, Heinrich Schuchardt xypron.glpk@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@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.
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.
The UEFI specification prescribes which fields must be in the struct. In both 1) and 2) you have private fields at a known offset to the structure.
EDK II can (this is configurable) additionally add a magic value to verify that the overall structure was provided by EDK II.
Best regards
Heinrich
struct efi_block_io is defined in UEFI spec, so probably option#2 is preferable and it is almost the same as the current implementation. I think we can proceed with the minor cleanup of struct efi_disk_obj as Akashi-san suggested.
[0] https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/9
Thanks, Masahisa Kojima
Best regards
Heinrich
return NULL;+}
/**
- efi_disk_reset() - reset block device
@@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, u32 media_id, u64 lba, unsigned long buffer_size, void *buffer, enum efi_disk_direction direction) {
struct efi_disk_obj *diskobj;
efi_handle_t handle; int blksz; int blocks; unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops);blksz = diskobj->media.block_size;
handle = efi_blkio_find_obj(this);if (!handle)return EFI_INVALID_PARAMETER;blksz = this->media->block_size; blocks = buffer_size / blksz; EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",@@ -124,18 +124,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (CONFIG_IS_ENABLED(PARTITIONS) &&
device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) {
device_get_uclass_id(handle->dev) == UCLASS_PARTITION) { if (direction == EFI_DISK_READ)
n = disk_blk_read(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_read(handle->dev, lba, blocks, buffer); else
n = disk_blk_write(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_write(handle->dev, lba, blocks, buffer); } else { /* dev is a block device (UCLASS_BLK) */ struct blk_desc *desc;
desc = dev_get_uclass_plat(diskobj->header.dev);
desc = dev_get_uclass_plat(handle->dev); if (direction == EFI_DISK_READ) n = blk_dread(desc, lba, blocks, buffer); else@@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part)
- @part: partition
- @disk: pointer to receive the created handle
- @agent_handle: handle of the EFI block driver
*/ static efi_status_t efi_disk_add_dev(
- @dev: pointer to udevice
- Return: disk object
@@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev( int dev_index, struct disk_partition *part_info, unsigned int part,
struct efi_disk_obj **disk,efi_handle_t agent_handle)
efi_handle_t *disk,efi_handle_t agent_handle,struct udevice *dev)
struct efi_disk_obj *diskobj;struct efi_object *handle;
efi_handle_t handle; const efi_guid_t *esp_guid = NULL; efi_status_t ret;struct efi_block_io *blkio;struct efi_block_io_media *media;struct efi_device_path *dp = NULL;struct efi_simple_file_system_protocol *volume = NULL; /* Don't add empty devices */ if (!desc->lba) return EFI_NOT_READY;
diskobj = calloc(1, sizeof(*diskobj));if (!diskobj)
ret = efi_create_handle(&handle);if (ret != EFI_SUCCESS)return ret;blkio = calloc(1, sizeof(*blkio));media = calloc(1, sizeof(*media));if (!blkio || !media) return EFI_OUT_OF_RESOURCES;
/* Hook up to the device list */efi_add_handle(&diskobj->header);
*blkio = block_io_disk_template;blkio->media = media; /* Fill in object data */ if (part_info) {@@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev( * (controller). */ ret = efi_protocol_open(handler, &protocol_interface, NULL,
&diskobj->header,
handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER); if (ret != EFI_SUCCESS) { log_debug("prot open failed\n"); goto error; }
diskobj->dp = efi_dp_append_node(dp_parent, node);
dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node);
diskobj->media.last_block = part_info->size - 1;
blkio->media->last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) esp_guid = &efi_system_partition_guid; } else {
diskobj->dp = efi_dp_from_part(desc, part);diskobj->media.last_block = desc->lba - 1;
dp = efi_dp_from_part(desc, part);blkio->media->last_block = desc->lba - 1; }
diskobj->part = part; /* * Install the device path and the block IO protocol.@@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev( * already installed on an other handle and returns EFI_ALREADY_STARTED * in this case. */
handle = &diskobj->header; ret = efi_install_multiple_protocol_interfaces( &handle,&efi_guid_device_path, diskobj->dp,&efi_block_io_guid, &diskobj->ops,
&efi_guid_device_path, dp,&efi_block_io_guid, blkio, /* * esp_guid must be last entry as it * can be NULL. Its interface is NULL.@@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev( */ if ((part || desc->part_type == PART_TYPE_UNKNOWN) && efi_fs_exists(desc, part)) {
ret = efi_create_simple_file_system(desc, part, diskobj->dp,&diskobj->volume);
ret = efi_create_simple_file_system(desc, part, dp, &volume); if (ret != EFI_SUCCESS) goto error;
ret = efi_add_protocol(&diskobj->header,
ret = efi_add_protocol(handle, &efi_simple_file_system_protocol_guid,
diskobj->volume);
volume); if (ret != EFI_SUCCESS) goto error; }
diskobj->ops = block_io_disk_template;diskobj->dev_index = dev_index; /* Fill in EFI IO Media info (for read/write callbacks) */diskobj->media.removable_media = desc->removable;diskobj->media.media_present = 1;
blkio->media->removable_media = desc->removable;blkio->media->media_present = 1; /* * MediaID is just an arbitrary counter. * We have to change it if the medium is removed or changed. */
diskobj->media.media_id = 1;diskobj->media.block_size = desc->blksz;diskobj->media.io_align = desc->blksz;
blkio->media->media_id = 1;blkio->media->block_size = desc->blksz;blkio->media->io_align = desc->blksz; if (part)
diskobj->media.logical_partition = 1;diskobj->ops.media = &diskobj->media;
blkio->media->logical_partition = 1; if (disk)
*disk = diskobj;
*disk = handle; EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n",
diskobj->part,diskobj->media.media_present,diskobj->media.logical_partition,diskobj->media.removable_media,diskobj->media.last_block);
part,blkio->media->media_present,blkio->media->logical_partition,blkio->media->removable_media,blkio->media->last_block); /* Store first EFI system partition */ if (part && efi_system_partition.uclass_id == UCLASS_INVALID) {@@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev( desc->devnum, part); } }
if (efi_link_dev(handle, dev))goto error;return EFI_SUCCESS;
efi_delete_handle(&diskobj->header);free(diskobj->volume);free(diskobj);
efi_delete_handle(handle);efi_free_pool(dp);free(blkio);free(media);free(volume);return ret;@@ -557,7 +566,7 @@ error: */ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) {
struct efi_disk_obj *disk;
efi_handle_t disk; struct blk_desc *desc; int diskid; efi_status_t ret;@@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum;
ret = efi_disk_add_dev(NULL, NULL, desc,
diskid, NULL, 0, &disk, agent_handle);
diskid, NULL, 0, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { if (ret == EFI_NOT_READY) { log_notice("Disk %s not ready\n", dev->name);@@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
return ret; }
if (efi_link_dev(&disk->header, dev)) {efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -EINVAL;} return 0;}
@@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) int diskid; struct efi_handler *handler; struct efi_device_path *dp_parent;
struct efi_disk_obj *disk;
efi_handle_t disk; efi_status_t ret; if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))@@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) dp_parent = (struct efi_device_path *)handler->protocol_interface;
ret = efi_disk_add_dev(parent, dp_parent, desc, diskid,
info, part, &disk, agent_handle);
info, part, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { log_err("Adding partition for %s failed\n", dev->name); return -1; }
if (efi_link_dev(&disk->header, dev)) {efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -1;} return 0;}
@@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event) return 0; }
+static void get_disk_resources(efi_handle_t handle, struct efi_device_path **dp,
struct efi_block_io **blkio,struct efi_simple_file_system_protocol **volume)+{
efi_status_t ret;struct efi_handler *handler;ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);if (ret == EFI_SUCCESS)*dp = handler->protocol_interface;ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);if (ret == EFI_SUCCESS)*blkio = handler->protocol_interface;ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid,&handler);if (ret == EFI_SUCCESS)*volume = handler->protocol_interface;+}
- /**
- efi_disk_remove - delete an efi_disk object for a block device or partition
- efi_disk_remove - delete an efi handle for a block device or partition
- @ctx: event context: driver binding protocol
- @event: EV_PM_PRE_REMOVE event
- Delete an efi_disk object which is associated with the UCLASS_BLK or
- Delete an efi handle which is associated with the UCLASS_BLK or
- UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised.
- Return: 0 on success, -1 otherwise
@@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc;
struct efi_disk_obj *diskobj = NULL; efi_status_t ret;
struct efi_device_path *dp = NULL;struct efi_block_io *blkio = NULL;struct efi_simple_file_system_protocol *volume = NULL; if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0;@@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
diskobj = container_of(handle, struct efi_disk_obj,header);
get_disk_resources(handle, &dp, &blkio, &volume);break; case UCLASS_PARTITION:
diskobj = container_of(handle, struct efi_disk_obj, header);
get_disk_resources(handle, &dp, &blkio, &volume); break; default: return 0;@@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event) if (ret != EFI_SUCCESS) return -1;
if (diskobj)efi_free_pool(diskobj->dp);
efi_free_pool(dp);if (blkio) {free(blkio->media);free(blkio);} dev_tag_del(dev, DM_TAG_EFI);
Hi Heinrich,
On Thu, 14 Dec 2023 at 16:31, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 14. Dezember 2023 02:55:27 MEZ schrieb Masahisa Kojima masahisa.kojima@linaro.org:
Hi Heinrich,
On Wed, 13 Dec 2023 at 23:22, Heinrich Schuchardt xypron.glpk@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@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.
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?
[1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Disk/Ra...
Thanks, Masahisa Kojima
The UEFI specification prescribes which fields must be in the struct. In both 1) and 2) you have private fields at a known offset to the structure.
EDK II can (this is configurable) additionally add a magic value to verify that the overall structure was provided by EDK II.
Best regards
Heinrich
struct efi_block_io is defined in UEFI spec, so probably option#2 is preferable and it is almost the same as the current implementation. I think we can proceed with the minor cleanup of struct efi_disk_obj as Akashi-san suggested.
[0] https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/9
Thanks, Masahisa Kojima
Best regards
Heinrich
return NULL;+}
/**
- efi_disk_reset() - reset block device
@@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, u32 media_id, u64 lba, unsigned long buffer_size, void *buffer, enum efi_disk_direction direction) {
struct efi_disk_obj *diskobj;
efi_handle_t handle; int blksz; int blocks; unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops);blksz = diskobj->media.block_size;
handle = efi_blkio_find_obj(this);if (!handle)return EFI_INVALID_PARAMETER;blksz = this->media->block_size; blocks = buffer_size / blksz; EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",@@ -124,18 +124,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (CONFIG_IS_ENABLED(PARTITIONS) &&
device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) {
device_get_uclass_id(handle->dev) == UCLASS_PARTITION) { if (direction == EFI_DISK_READ)
n = disk_blk_read(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_read(handle->dev, lba, blocks, buffer); else
n = disk_blk_write(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_write(handle->dev, lba, blocks, buffer); } else { /* dev is a block device (UCLASS_BLK) */ struct blk_desc *desc;
desc = dev_get_uclass_plat(diskobj->header.dev);
desc = dev_get_uclass_plat(handle->dev); if (direction == EFI_DISK_READ) n = blk_dread(desc, lba, blocks, buffer); else@@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part)
- @part: partition
- @disk: pointer to receive the created handle
- @agent_handle: handle of the EFI block driver
*/ static efi_status_t efi_disk_add_dev(
- @dev: pointer to udevice
- Return: disk object
@@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev( int dev_index, struct disk_partition *part_info, unsigned int part,
struct efi_disk_obj **disk,efi_handle_t agent_handle)
efi_handle_t *disk,efi_handle_t agent_handle,struct udevice *dev)
struct efi_disk_obj *diskobj;struct efi_object *handle;
efi_handle_t handle; const efi_guid_t *esp_guid = NULL; efi_status_t ret;struct efi_block_io *blkio;struct efi_block_io_media *media;struct efi_device_path *dp = NULL;struct efi_simple_file_system_protocol *volume = NULL; /* Don't add empty devices */ if (!desc->lba) return EFI_NOT_READY;
diskobj = calloc(1, sizeof(*diskobj));if (!diskobj)
ret = efi_create_handle(&handle);if (ret != EFI_SUCCESS)return ret;blkio = calloc(1, sizeof(*blkio));media = calloc(1, sizeof(*media));if (!blkio || !media) return EFI_OUT_OF_RESOURCES;
/* Hook up to the device list */efi_add_handle(&diskobj->header);
*blkio = block_io_disk_template;blkio->media = media; /* Fill in object data */ if (part_info) {@@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev( * (controller). */ ret = efi_protocol_open(handler, &protocol_interface, NULL,
&diskobj->header,
handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER); if (ret != EFI_SUCCESS) { log_debug("prot open failed\n"); goto error; }
diskobj->dp = efi_dp_append_node(dp_parent, node);
dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node);
diskobj->media.last_block = part_info->size - 1;
blkio->media->last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) esp_guid = &efi_system_partition_guid; } else {
diskobj->dp = efi_dp_from_part(desc, part);diskobj->media.last_block = desc->lba - 1;
dp = efi_dp_from_part(desc, part);blkio->media->last_block = desc->lba - 1; }
diskobj->part = part; /* * Install the device path and the block IO protocol.@@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev( * already installed on an other handle and returns EFI_ALREADY_STARTED * in this case. */
handle = &diskobj->header; ret = efi_install_multiple_protocol_interfaces( &handle,&efi_guid_device_path, diskobj->dp,&efi_block_io_guid, &diskobj->ops,
&efi_guid_device_path, dp,&efi_block_io_guid, blkio, /* * esp_guid must be last entry as it * can be NULL. Its interface is NULL.@@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev( */ if ((part || desc->part_type == PART_TYPE_UNKNOWN) && efi_fs_exists(desc, part)) {
ret = efi_create_simple_file_system(desc, part, diskobj->dp,&diskobj->volume);
ret = efi_create_simple_file_system(desc, part, dp, &volume); if (ret != EFI_SUCCESS) goto error;
ret = efi_add_protocol(&diskobj->header,
ret = efi_add_protocol(handle, &efi_simple_file_system_protocol_guid,
diskobj->volume);
volume); if (ret != EFI_SUCCESS) goto error; }
diskobj->ops = block_io_disk_template;diskobj->dev_index = dev_index; /* Fill in EFI IO Media info (for read/write callbacks) */diskobj->media.removable_media = desc->removable;diskobj->media.media_present = 1;
blkio->media->removable_media = desc->removable;blkio->media->media_present = 1; /* * MediaID is just an arbitrary counter. * We have to change it if the medium is removed or changed. */
diskobj->media.media_id = 1;diskobj->media.block_size = desc->blksz;diskobj->media.io_align = desc->blksz;
blkio->media->media_id = 1;blkio->media->block_size = desc->blksz;blkio->media->io_align = desc->blksz; if (part)
diskobj->media.logical_partition = 1;diskobj->ops.media = &diskobj->media;
blkio->media->logical_partition = 1; if (disk)
*disk = diskobj;
*disk = handle; EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n",
diskobj->part,diskobj->media.media_present,diskobj->media.logical_partition,diskobj->media.removable_media,diskobj->media.last_block);
part,blkio->media->media_present,blkio->media->logical_partition,blkio->media->removable_media,blkio->media->last_block); /* Store first EFI system partition */ if (part && efi_system_partition.uclass_id == UCLASS_INVALID) {@@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev( desc->devnum, part); } }
if (efi_link_dev(handle, dev))goto error;return EFI_SUCCESS;
efi_delete_handle(&diskobj->header);free(diskobj->volume);free(diskobj);
efi_delete_handle(handle);efi_free_pool(dp);free(blkio);free(media);free(volume);return ret;@@ -557,7 +566,7 @@ error: */ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) {
struct efi_disk_obj *disk;
efi_handle_t disk; struct blk_desc *desc; int diskid; efi_status_t ret;@@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum;
ret = efi_disk_add_dev(NULL, NULL, desc,
diskid, NULL, 0, &disk, agent_handle);
diskid, NULL, 0, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { if (ret == EFI_NOT_READY) { log_notice("Disk %s not ready\n", dev->name);@@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
return ret; }
if (efi_link_dev(&disk->header, dev)) {efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -EINVAL;} return 0;}
@@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) int diskid; struct efi_handler *handler; struct efi_device_path *dp_parent;
struct efi_disk_obj *disk;
efi_handle_t disk; efi_status_t ret; if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))@@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) dp_parent = (struct efi_device_path *)handler->protocol_interface;
ret = efi_disk_add_dev(parent, dp_parent, desc, diskid,
info, part, &disk, agent_handle);
info, part, &disk, agent_handle, dev); if (ret != EFI_SUCCESS) { log_err("Adding partition for %s failed\n", dev->name); return -1; }
if (efi_link_dev(&disk->header, dev)) {efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -1;} return 0;}
@@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event) return 0; }
+static void get_disk_resources(efi_handle_t handle, struct efi_device_path **dp,
struct efi_block_io **blkio,struct efi_simple_file_system_protocol **volume)+{
efi_status_t ret;struct efi_handler *handler;ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);if (ret == EFI_SUCCESS)*dp = handler->protocol_interface;ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);if (ret == EFI_SUCCESS)*blkio = handler->protocol_interface;ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid,&handler);if (ret == EFI_SUCCESS)*volume = handler->protocol_interface;+}
- /**
- efi_disk_remove - delete an efi_disk object for a block device or partition
- efi_disk_remove - delete an efi handle for a block device or partition
- @ctx: event context: driver binding protocol
- @event: EV_PM_PRE_REMOVE event
- Delete an efi_disk object which is associated with the UCLASS_BLK or
- Delete an efi handle which is associated with the UCLASS_BLK or
- UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised.
- Return: 0 on success, -1 otherwise
@@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc;
struct efi_disk_obj *diskobj = NULL; efi_status_t ret;
struct efi_device_path *dp = NULL;struct efi_block_io *blkio = NULL;struct efi_simple_file_system_protocol *volume = NULL; if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0;@@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
diskobj = container_of(handle, struct efi_disk_obj,header);
get_disk_resources(handle, &dp, &blkio, &volume);break; case UCLASS_PARTITION:
diskobj = container_of(handle, struct efi_disk_obj, header);
get_disk_resources(handle, &dp, &blkio, &volume); break; default: return 0;@@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event) if (ret != EFI_SUCCESS) return -1;
if (diskobj)efi_free_pool(diskobj->dp);
efi_free_pool(dp);if (blkio) {free(blkio->media);free(blkio);} dev_tag_del(dev, DM_TAG_EFI);
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.
Best regards
Heinrich
[1]https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Disk/Ra...
Thanks, Masahisa Kojima
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
Hi Kojima-san,
On Wed, Dec 13, 2023 at 05:57:37PM +0900, 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.
I don't still understand what is an advantage of your approach.
efi_link_dev() call is moved inside of efi_disk_add_dev() function to simplify the cleanup process in case of error.
Signed-off-by: Masahisa Kojima masahisa.kojima@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;
This is a handy way of making it ease to dereference from an internal structure to an external reference as Heinrich mentioned.
- struct efi_block_io ops;
Along with "header" and "media", this allows us to congregate a couple of "malloc" calls, instead of your change, into one.
- int dev_index;
- struct efi_device_path *dp;
- unsigned int part;
- struct efi_simple_file_system_protocol *volume;
If we carefully look into the base code, we will no longer have to have those fields in this structure because they are mostly used in a single internal function (if I'm correct). So we can simply replace them with local variables (with some extra changes).
-Takahiro Akashi
-}; +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;- }
- return NULL;
+}
/**
- efi_disk_reset() - reset block device
@@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, u32 media_id, u64 lba, unsigned long buffer_size, void *buffer, enum efi_disk_direction direction) {
- struct efi_disk_obj *diskobj;
- efi_handle_t handle; int blksz; int blocks; unsigned long n;
- diskobj = container_of(this, struct efi_disk_obj, ops);
- blksz = diskobj->media.block_size;
handle = efi_blkio_find_obj(this);
if (!handle)
return EFI_INVALID_PARAMETER;blksz = this->media->block_size; blocks = buffer_size / blksz;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",
@@ -124,18 +124,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (CONFIG_IS_ENABLED(PARTITIONS) &&
device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) {
device_get_uclass_id(handle->dev) == UCLASS_PARTITION) {
n = disk_blk_read(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_read(handle->dev, lba, blocks, buffer);
n = disk_blk_write(diskobj->header.dev, lba, blocks,buffer);
n = disk_blk_write(handle->dev, lba, blocks, buffer);
desc = dev_get_uclass_plat(diskobj->header.dev);
desc = dev_get_uclass_plat(handle->dev);@@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part)
- @part: partition
- @disk: pointer to receive the created handle
- @agent_handle: handle of the EFI block driver
*/
- @dev: pointer to udevice
- Return: disk object
static efi_status_t efi_disk_add_dev( @@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev( int dev_index, struct disk_partition *part_info, unsigned int part,
struct efi_disk_obj **disk,efi_handle_t agent_handle)
efi_handle_t *disk,efi_handle_t agent_handle,struct udevice *dev){
- struct efi_disk_obj *diskobj;
- struct efi_object *handle;
efi_handle_t handle; const efi_guid_t *esp_guid = NULL; efi_status_t ret;
struct efi_block_io *blkio;
struct efi_block_io_media *media;
struct efi_device_path *dp = NULL;
struct efi_simple_file_system_protocol *volume = NULL;
/* Don't add empty devices */ if (!desc->lba) return EFI_NOT_READY;
- diskobj = calloc(1, sizeof(*diskobj));
- if (!diskobj)
- ret = efi_create_handle(&handle);
- if (ret != EFI_SUCCESS)
return ret;- blkio = calloc(1, sizeof(*blkio));
- media = calloc(1, sizeof(*media));
- if (!blkio || !media) return EFI_OUT_OF_RESOURCES;
- /* Hook up to the device list */
- efi_add_handle(&diskobj->header);
*blkio = block_io_disk_template;
blkio->media = media;
/* Fill in object data */ if (part_info) {
@@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev( * (controller). */ ret = efi_protocol_open(handler, &protocol_interface, NULL,
&diskobj->header,
handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
diskobj->dp = efi_dp_append_node(dp_parent, node);
dp = efi_dp_append_node(dp_parent, node);
diskobj->media.last_block = part_info->size - 1;
blkio->media->last_block = part_info->size - 1;
diskobj->dp = efi_dp_from_part(desc, part);diskobj->media.last_block = desc->lba - 1;
dp = efi_dp_from_part(desc, part);blkio->media->last_block = desc->lba - 1;
diskobj->part = part;
/*
- Install the device path and the block IO protocol.
@@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev( * already installed on an other handle and returns EFI_ALREADY_STARTED * in this case. */
- handle = &diskobj->header; ret = efi_install_multiple_protocol_interfaces( &handle,
&efi_guid_device_path, diskobj->dp,&efi_block_io_guid, &diskobj->ops,
&efi_guid_device_path, dp,&efi_block_io_guid, blkio, /* * esp_guid must be last entry as it * can be NULL. Its interface is NULL.@@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev( */ if ((part || desc->part_type == PART_TYPE_UNKNOWN) && efi_fs_exists(desc, part)) {
ret = efi_create_simple_file_system(desc, part, diskobj->dp,&diskobj->volume);
ret = efi_create_simple_file_system(desc, part, dp, &volume);
ret = efi_add_protocol(&diskobj->header,
ret = efi_add_protocol(handle, &efi_simple_file_system_protocol_guid,
diskobj->volume);
volume);
diskobj->ops = block_io_disk_template;
diskobj->dev_index = dev_index;
/* Fill in EFI IO Media info (for read/write callbacks) */
diskobj->media.removable_media = desc->removable;
diskobj->media.media_present = 1;
- blkio->media->removable_media = desc->removable;
- blkio->media->media_present = 1; /*
*/
- MediaID is just an arbitrary counter.
- We have to change it if the medium is removed or changed.
- diskobj->media.media_id = 1;
- diskobj->media.block_size = desc->blksz;
- diskobj->media.io_align = desc->blksz;
- blkio->media->media_id = 1;
- blkio->media->block_size = desc->blksz;
- blkio->media->io_align = desc->blksz; if (part)
diskobj->media.logical_partition = 1;- diskobj->ops.media = &diskobj->media;
blkio->media->logical_partition = 1;
*disk = diskobj;
*disk = handle;EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n",
diskobj->part,diskobj->media.media_present,diskobj->media.logical_partition,diskobj->media.removable_media,diskobj->media.last_block);
part,blkio->media->media_present,blkio->media->logical_partition,blkio->media->removable_media,blkio->media->last_block);/* Store first EFI system partition */ if (part && efi_system_partition.uclass_id == UCLASS_INVALID) {
@@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev( desc->devnum, part); } }
- if (efi_link_dev(handle, dev))
goto error;- return EFI_SUCCESS;
error:
- efi_delete_handle(&diskobj->header);
- free(diskobj->volume);
- free(diskobj);
- efi_delete_handle(handle);
- efi_free_pool(dp);
- free(blkio);
- free(media);
- free(volume);
- return ret;
}
@@ -557,7 +566,7 @@ error: */ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) {
- struct efi_disk_obj *disk;
- efi_handle_t disk; struct blk_desc *desc; int diskid; efi_status_t ret;
@@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum;
ret = efi_disk_add_dev(NULL, NULL, desc,
diskid, NULL, 0, &disk, agent_handle);
diskid, NULL, 0, &disk, agent_handle, dev);@@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
return ret;}
if (efi_link_dev(&disk->header, dev)) {
efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -EINVAL;}
return 0;
} @@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) int diskid; struct efi_handler *handler; struct efi_device_path *dp_parent;
- struct efi_disk_obj *disk;
efi_handle_t disk; efi_status_t ret;
if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
@@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) dp_parent = (struct efi_device_path *)handler->protocol_interface;
ret = efi_disk_add_dev(parent, dp_parent, desc, diskid,
info, part, &disk, agent_handle);
info, part, &disk, agent_handle, dev);
if (efi_link_dev(&disk->header, dev)) {
efi_free_pool(disk->dp);efi_delete_handle(&disk->header);return -1;}
return 0;
} @@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event) return 0; }
+static void get_disk_resources(efi_handle_t handle, struct efi_device_path **dp,
struct efi_block_io **blkio,struct efi_simple_file_system_protocol **volume)+{
- efi_status_t ret;
- struct efi_handler *handler;
- ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
- if (ret == EFI_SUCCESS)
*dp = handler->protocol_interface;- ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
- if (ret == EFI_SUCCESS)
*blkio = handler->protocol_interface;- ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid,
&handler);- if (ret == EFI_SUCCESS)
*volume = handler->protocol_interface;+}
/**
- efi_disk_remove - delete an efi_disk object for a block device or partition
- efi_disk_remove - delete an efi handle for a block device or partition
- @ctx: event context: driver binding protocol
- @event: EV_PM_PRE_REMOVE event
- Delete an efi_disk object which is associated with the UCLASS_BLK or
- Delete an efi handle which is associated with the UCLASS_BLK or
- UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised.
- Return: 0 on success, -1 otherwise
@@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc;
- struct efi_disk_obj *diskobj = NULL; efi_status_t ret;
struct efi_device_path *dp = NULL;
struct efi_block_io *blkio = NULL;
struct efi_simple_file_system_protocol *volume = NULL;
if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0;
@@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
diskobj = container_of(handle, struct efi_disk_obj,header);
get_disk_resources(handle, &dp, &blkio, &volume);- break; case UCLASS_PARTITION:
diskobj = container_of(handle, struct efi_disk_obj, header);
get_disk_resources(handle, &dp, &blkio, &volume);@@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event) if (ret != EFI_SUCCESS) return -1;
- if (diskobj)
efi_free_pool(diskobj->dp);
efi_free_pool(dp);
if (blkio) {
free(blkio->media);free(blkio);}
dev_tag_del(dev, DM_TAG_EFI);
-- 2.34.1
participants (4)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Masahisa Kojima -
Simon Glass