[U-Boot] [PATCH 07/14] efi_loader: Add disk interfaces

Simon Glass
2016-01-31T15:23:02Z

On 14 January 2016 at 22:06, Alexander Graf <agraf@suse.de> wrote:
> A EFI applications usually want to access storage devices to load data from.
>
> This patch adds support for EFI disk interfaces. It loops through all block
> storage interfaces known to U-Boot and creates an EFI object for each existing
> one. EFI applications can then through these objects call U-Boot's read and
> write functions.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> ---
>
> v1 -> v2:
>
> - Move to block_drvr array
> - Move to GPLv2+
> - Fix header order
> - Document efi block object struct
> - Use calloc rather than malloc & memset
> ---
> include/efi_loader.h | 1 +
> lib/efi_loader/efi_disk.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 219 insertions(+)
> create mode 100644 lib/efi_loader/efi_disk.c
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0f821ff..9d6c322 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -63,6 +63,7 @@ struct efi_object {
> };
> extern struct list_head efi_obj_list;
>
> +int efi_disk_register(void);
> efi_status_t efi_return_handle(void *handle,
> efi_guid_t *protocol, void **protocol_interface,
> void *agent_handle, void *controller_handle,
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> new file mode 100644
> index 0000000..c589604
> --- /dev/null
> +++ b/lib/efi_loader/efi_disk.c
> @@ -0,0 +1,218 @@
> +/*
> + * EFI application disk support
> + *
> + * Copyright (c) 2016 Alexander Graf
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <efi_loader.h>
> +#include <inttypes.h>
> +#include <part.h>
> +#include <malloc.h>
> +
> +static const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
> +
> +struct efi_disk_obj {
> + /* Generic EFI object parent class data */
> + struct efi_object parent;
> + /* EFI Interface callback struct for block I/O */
> + struct efi_block_io ops;
> + /* U-Boot ifname for block device */
> + const char *ifname;
> + /* U-Boot dev_index for block device */
> + int dev_index;
> + /* EFI Interface Media descriptor struct, referenced by ops */
> + struct efi_block_io_media media;
> + /* EFI device path to this block device */
> + struct efi_device_path_file_path *dp;
> +};
> +
> +static void ascii2unicode(u16 *unicode, char *ascii)
> +{
> + while (*ascii)
> + *(unicode++) = *(ascii++);
> +}
> +
> +static efi_status_t efi_disk_open_block(void *handle, efi_guid_t *protocol,
> + void **protocol_interface, void *agent_handle,
> + void *controller_handle, uint32_t attributes)
> +{
> + struct efi_disk_obj *diskobj = handle;
> +
> + *protocol_interface = &diskobj->ops;
> +
> + return EFI_SUCCESS;
> +}
> +
> +static efi_status_t efi_disk_open_dp(void *handle, efi_guid_t *protocol,
> + void **protocol_interface, void *agent_handle,
> + void *controller_handle, uint32_t attributes)
> +{
> + struct efi_disk_obj *diskobj = handle;
> +
> + *protocol_interface = diskobj->dp;
> +
> + return EFI_SUCCESS;
> +}
> +
> +static efi_status_t efi_disk_reset(struct efi_block_io *this,
> + char extended_verification)
> +{
> + EFI_ENTRY("%p, %x", this, extended_verification);
> + return EFI_EXIT(EFI_DEVICE_ERROR);
> +}
> +
> +enum efi_disk_direction {
> + EFI_DISK_READ,
> + EFI_DISK_WRITE,
> +};
> +
> +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;
> + struct block_dev_desc *desc;
> + int blksz;
> + int blocks;
> + unsigned long n;
> +
> + EFI_ENTRY("%p, %x, %"PRIx64", %lx, %p", this, media_id, lba,
> + buffer_size, buffer);
> +
> + diskobj = container_of(this, struct efi_disk_obj, ops);
> + if (!(desc = get_dev(diskobj->ifname, diskobj->dev_index)))
> + return EFI_EXIT(EFI_DEVICE_ERROR);
> + blksz = desc->blksz;
> + blocks = buffer_size / blksz;
> +
> +#ifdef DEBUG_EFI
> + printf("EFI: %s:%d blocks=%x lba=%"PRIx64" blksz=%x dir=%d\n", __func__,
> + __LINE__, blocks, lba, blksz, direction);
> +#endif
> +
> + /* We only support full block access */
> + if (buffer_size & (blksz - 1))
> + return EFI_EXIT(EFI_DEVICE_ERROR);
> +
> + if (direction == EFI_DISK_READ)
> + n = desc->block_read(desc->dev, lba, blocks, buffer);

Shouldn't this be s/desc->dev/desc/?

> + else
> + n = desc->block_write(desc->dev, lba, blocks, buffer);

Same here.

> +
> + /* We don't do interrupts, so check for timers cooperatively */
> + efi_timer_check();
> +
> +#ifdef DEBUG_EFI
> + printf("EFI: %s:%d n=%lx blocks=%x\n", __func__, __LINE__, n, blocks);
> +#endif
> + if (n != blocks)
> + return EFI_EXIT(EFI_DEVICE_ERROR);
> +
> + return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +static efi_status_t efi_disk_read_blocks(struct efi_block_io *this,
> + u32 media_id, u64 lba, unsigned long buffer_size,
> + void *buffer)
> +{
> + return efi_disk_rw_blocks(this, media_id, lba, buffer_size, buffer,
> + EFI_DISK_READ);
> +}
> +
> +static efi_status_t efi_disk_write_blocks(struct efi_block_io *this,
> + u32 media_id, u64 lba, unsigned long buffer_size,
> + void *buffer)
> +{
> + return efi_disk_rw_blocks(this, media_id, lba, buffer_size, buffer,
> + EFI_DISK_WRITE);
> +}
> +
> +static efi_status_t efi_disk_flush_blocks(struct efi_block_io *this)
> +{
> + /* We always write synchronously */
> + return EFI_SUCCESS;
> +}
> +
> +static const struct efi_block_io block_io_disk_template = {
> + .reset = &efi_disk_reset,
> + .read_blocks = &efi_disk_read_blocks,
> + .write_blocks = &efi_disk_write_blocks,
> + .flush_blocks = &efi_disk_flush_blocks,
> +};
> +
> +/*
> + * U-Boot doesn't have a list of all online disk devices. So when running our
> + * EFI payload, we scan through all of the potentially available ones and
> + * store them in our object pool.

This isn't quite true - you are actually using its list below. You
could modify this to work without the table I suspect. But the way you
have it seems good to me.

> + *
> + * This gets called from do_bootefi_exec().
> + */
> +int efi_disk_register(void)
> +{
> + const struct block_drvr *cur_drvr;

I think it would be better to use a function call to return the
driver. Then we can implement it with driver model later.

How about:

block_dev_first(&cur_drvr)
block_dev_next(&cur_drvr)

> + int i;
> + int disks = 0;
> +
> + /* Search for all available disk devices */
> + for (cur_drvr = block_drvr; cur_drvr->name; cur_drvr++) {
> + printf("Scanning disks on %s...\n", cur_drvr->name);
> + for (i = 0; i < 4; i++) {

Please add a comment about the 4, or an enum/#define.

> + block_dev_desc_t *desc;
> + struct efi_disk_obj *diskobj;
> + struct efi_device_path_file_path *dp;
> + int objlen = sizeof(*diskobj) + (sizeof(*dp) * 2);
> + char devname[16];
> +
> + desc = get_dev(cur_drvr->name, i);
> + if (!desc)
> + continue;

It's not clear how this checks that the device is actually valid. I
think if desc->type == DEV_TYPE_UNKNOWN then you should ignore the
device. For example an MMC card slot might be empty.

> +
> + diskobj = calloc(1, objlen);
> +
> + /* Fill in object data */

drop extra blank line, and below

> +
> + diskobj->parent.protocols[0].guid = &efi_block_io_guid;
> + diskobj->parent.protocols[0].open = efi_disk_open_block;
> + diskobj->parent.protocols[1].guid = &efi_guid_device_path;
> + diskobj->parent.protocols[1].open = efi_disk_open_dp;
> + diskobj->parent.handle = diskobj;
> + diskobj->ops = block_io_disk_template;
> + diskobj->ifname = cur_drvr->name;
> + diskobj->dev_index = i;
> +
> + /* Fill in EFI IO Media info (for read/write callbacks) */
> +
> + diskobj->media.removable_media = desc->removable;
> + diskobj->media.media_present = 1;
> + diskobj->media.block_size = desc->blksz;
> + diskobj->media.io_align = desc->blksz;
> + diskobj->media.last_block = desc->lba;
> + diskobj->ops.media = &diskobj->media;
> +
> + /* Fill in device path */
> +
> + dp = (void*)&diskobj[1];
> + diskobj->dp = dp;
> + dp[0].dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
> + dp[0].dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
> + dp[0].dp.length = sizeof(*dp);
> + sprintf(devname, "%s%d", cur_drvr->name, i);

I'd suggest snprintf(devname, ARRAY_SIZE(dp[0].str), ...) or perhaps a check.

> + ascii2unicode(dp[0].str, devname);
> +
> + dp[1].dp.type = DEVICE_PATH_TYPE_END;
> + dp[1].dp.sub_type = DEVICE_PATH_SUB_TYPE_END;
> + dp[1].dp.length = sizeof(*dp);
> +
> + /* Hook up to the device list */
> +
> + list_add_tail(&diskobj->parent.link, &efi_obj_list);
> + disks++;
> + }
> + }
> + printf("Found %d disks\n", disks);
> +
> + return 0;
> +}
> --
> 2.1.4
>

Regards,
Simon