[U-Boot] [PATCH 07/14] efi_loader: Add disk interfaces
Alexander Graf
agraf at suse.de
Tue Feb 2 01:32:11 CET 2016
On 01/31/2016 04:23 PM, Simon Glass wrote:
> On 14 January 2016 at 22:06, Alexander Graf <agraf at 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 at suse.de>
>> Reviewed-by: Simon Glass <sjg at 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.
What we have is a list of possible drivers. We don't know the list of
online devices. That's why we're looping from 0-3 to see whether behind
any of those doors there's an online device hiding :).
In an ideal world, we could just say "loop me through all storage
devices" without me caring about device drivers or numbering.
>
>> + *
>> + * 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)
As mentioned above, I'd like to see this generalized even more. Do you
think we can postpone the exact way this loops to the point in time when
that is sorted out?
>
>> + 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.
Sure. It's a define if anything, since it's really just an arbitrary
MAX_DISKS_PER_DRIVER.
>
>> + 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.
Ah, I didn't realize you could have DEV_TYPE_UNKNOWN.
>
>> +
>> + 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.
I'll add a comment in the array definition of devname. The str array
size is also 16.
Alex
More information about the U-Boot
mailing list