Re: [PATCH 13/35] efi: Add a media/block driver for EFI block devices

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 9/8/21 3:33 PM, Simon Glass wrote:
> Add a block driver which handles read/write for EFI block devices. This
> driver actually already exists ('efi_block') but is not really suitable
> for use as a real U-Boot driver:
>
> - The operations do not provide a udevice

efi_bl_bind() creates a udevice by calling blk_create_device() when an
EFI application calls ConnectController() for a handle with an
EFI_BLOCK_IO_PROTOCOL.

Please, explain in some detail what you think is wrong with the existing
code.

> - The code is designed for running as part of EFI loader, so uses
>      EFI_PRINT() and EFI_CALL().
> - It creates block devices for all the partitions too, which is not
>    somthing we want to support in this way
> - The bind method probes the device, which is not permitted
> - It uses 'EFI' as its parent device
>
> The new driver is more 'normal', just requiring its platform data be set
> up in advance.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Please, separate this series in two. One for U-Boot on EFI and one for
U-Boot's UEFI implementation.

Best regardss

Heinrich

> ---
>
>   drivers/block/Kconfig   |  10 ++++
>   drivers/block/Makefile  |   1 +
>   drivers/block/efi_blk.c | 115 ++++++++++++++++++++++++++++++++++++++++
>   include/efi.h           |  11 ++++
>   4 files changed, 137 insertions(+)
>   create mode 100644 drivers/block/efi_blk.c
>
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 058956ee8ee..ab450a52e9e 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -84,6 +84,16 @@ config EFI_MEDIA_SANDBOX
>   	  EFI_MEDIA uclass. It does not do anything useful, since sandbox does
>   	  not actually support running on top of UEFI.
>
> +config EFI_MEDIA_BLK
> +	bool "EFI media block driver"
> +	depends on EFI_APP
> +	default y
> +	help
> +	  Enables a block driver for providing access to UEFI devices. This
> +	  allows use of block devices detected by the underlying UEFI
> +	  implementation. With this it is possible to use filesystems on these
> +	  devices, for example.
> +
>   endif  # EFI_MEDIA
>
>   config IDE
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index 3778633da1d..b221a7c6eea 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_$(SPL_TPL_)BLOCK_CACHE) += blkcache.o
>
>   obj-$(CONFIG_EFI_MEDIA) += efi-media-uclass.o
>   obj-$(CONFIG_EFI_MEDIA_SANDBOX) += sb_efi_media.o
> +obj-$(CONFIG_EFI_MEDIA_BLK) += efi_blk.o
> diff --git a/drivers/block/efi_blk.c b/drivers/block/efi_blk.c
> new file mode 100644
> index 00000000000..c00b0cc0b1c
> --- /dev/null
> +++ b/drivers/block/efi_blk.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Block driver for EFI devices
> + * This supports a media driver of UCLASS_EFI with a child UCLASS_BLK
> + * It allows block-level access to EFI devices made available via EFI boot
> + * services
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#include <common.h>
> +#include <blk.h>
> +#include <dm.h>
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +struct efi_block_plat {
> +	struct efi_block_io *blkio;
> +};
> +
> +/**
> + * Read from block device
> + *
> + * @dev:	device
> + * @blknr:	first block to be read
> + * @blkcnt:	number of blocks to read
> + * @buffer:	output buffer
> + * Return:	number of blocks transferred
> + */
> +static ulong efi_bl_read(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> +			 void *buffer)
> +{
> +	struct efi_block_plat *plat = dev_get_plat(dev);
> +	struct efi_block_io *io = plat->blkio;
> +	efi_status_t ret;
> +
> +	log_debug("read buf=%p, block=%lx, count=%lx: ", buffer, (ulong)blknr,
> +		  (ulong)blkcnt);
> +	ret = io->read_blocks(io, io->media->media_id, blknr,
> +			      blkcnt * io->media->block_size, buffer);
> +	log_debug("ret=%lx (dec %ld)\n", ret & ~EFI_ERROR_MASK,
> +		  ret & ~EFI_ERROR_MASK);
> +	if (ret)
> +		return 0;
> +
> +	return blkcnt;
> +}
> +
> +/**
> + * Write to block device
> + *
> + * @dev:	device
> + * @blknr:	first block to be write
> + * @blkcnt:	number of blocks to write
> + * @buffer:	input buffer
> + * Return:	number of blocks transferred
> + */
> +static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> +			  const void *buffer)
> +{
> +	struct efi_block_plat *plat = dev_get_plat(dev);
> +	struct efi_block_io *io = plat->blkio;
> +	efi_status_t ret;
> +
> +	log_debug("write buf=%p, block=%lx, count=%lx: ", buffer, (ulong)blknr,
> +		  (ulong)blkcnt);
> +	ret = io->write_blocks(io, io->media->media_id, blknr,
> +			       blkcnt * io->media->block_size, (void *)buffer);
> +	log_debug("ret=%lx (dec %ld)\n", ret & ~EFI_ERROR_MASK,
> +		  ret & ~EFI_ERROR_MASK);
> +	if (ret)
> +		return 0;
> +
> +	return blkcnt;
> +}
> +
> +/* Block device driver operators */
> +static const struct blk_ops efi_blk_ops = {
> +	.read	= efi_bl_read,
> +	.write	= efi_bl_write,
> +};
> +
> +U_BOOT_DRIVER(efi_block) = {
> +	.name		= "efi_block",
> +	.id		= UCLASS_BLK,
> +	.ops		= &efi_blk_ops,
> +	.plat_auto	= sizeof(struct efi_block_plat),
> +};
> +
> +static int efi_media_bind(struct udevice *dev)
> +{
> +	struct efi_media_plat *plat = dev_get_plat(dev);
> +	struct efi_block_plat *blk_plat;
> +	struct udevice *blk;
> +	int ret;
> +
> +	ret = blk_create_devicef(dev, "efi_block", "blk", IF_TYPE_EFI,
> +				 dev_seq(dev), plat->blkio->media->block_size,
> +				 plat->blkio->media->last_block, &blk);
> +	if (ret) {
> +		debug("Cannot create block device\n");
> +		return ret;
> +	}
> +	blk_plat = dev_get_plat(blk);
> +	blk_plat->blkio = plat->blkio;
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(efi_media) = {
> +	.name		= "efi_media",
> +	.id		= UCLASS_EFI_MEDIA,
> +	.bind		= efi_media_bind,
> +	.plat_auto	= sizeof(struct efi_media_plat),
> +};
> diff --git a/include/efi.h b/include/efi.h
> index b5835422b95..0ec5913ddd1 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -414,6 +414,17 @@ struct efi_priv {
>   	void *next_hdr;
>   };
>
> +/*
> + * EFI attributes of the udevice handled by efi_media driver
> + *
> + * @handle: handle of the controller on which this driver is installed
> + * @blkio: block io protocol proxied by this driver
> + */
> +struct efi_media_plat {
> +	efi_handle_t		handle;
> +	struct efi_block_io	*blkio;
> +};
> +
>   /* Base address of the EFI image */
>   extern char image_base[];
>
>