| Message ID | 20210908073355.13.I9b48fe5f6b8c61348f16a1b5df114282240238c0@changeid |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: Improvements to U-Boot running on top of UEFI | expand |
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[]; > >
Hi Heinrich, On Wed, 8 Sept 2021 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > 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. See below: > > > - 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. Again I'm not sure what you mean here. Please point to something you don't want in this series, which is focussed on the app. > > 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 > > Regards, Simon
On 9/9/21 10:57 AM, Simon Glass wrote: > Hi Heinrich, > > On Wed, 8 Sept 2021 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> >> >> 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. > > See below: > >> >>> - 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 No, it creates a block device for the handle that was passed to ConnectController(). The handles for the partitions are created by U-Boot and these are not backed by separate block devices. >>> - The bind method probes the device, which is not permitted >>> - It uses 'EFI' as its parent device All devices that the UEFI sub-system uses must be probed. >>> >>> The new driver is more 'normal', just requiring its platform data be set >>> up in advance. No clue what you mean by "in advance". It is a software like iPXE that produces a handle at runtime and then calls ConnectController(). When that call is done we must create a block device which is in status probed, create handles for the partitions and install protocol interfaces on these partitions. Best regards Heinrich >>> >>> 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. > > Again I'm not sure what you mean here. Please point to something you > don't want in this series, which is focussed on the app. > >> >> 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 >>> > > Regards, > Simon >
Hi Heinrich, On Thu, 9 Sept 2021 at 04:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 9/9/21 10:57 AM, Simon Glass wrote: > > Hi Heinrich, > > > > On Wed, 8 Sept 2021 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> > >> > >> 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. > > > > See below: > > > >> > >>> - 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 > > No, it creates a block device for the handle that was passed to > ConnectController(). The handles for the partitions are created by > U-Boot and these are not backed by separate block devices. OK, that is hard to discover so thank you for explaining it. The other items stand, in any case. > > Best regards > > Heinrich > > >>> - The bind method probes the device, which is not permitted > >>> - It uses 'EFI' as its parent device > > All devices that the UEFI sub-system uses must be probed., It is fine to probe a device but this must not be done in the bind() method. Hopefully the reason for this is obvious? > > >>> > >>> The new driver is more 'normal', just requiring its platform data be set > >>> up in advance. > > No clue what you mean by "in advance". It is a software like iPXE that > produces a handle at runtime and then calls ConnectController(). When > that call is done we must create a block device which is in status > probed, create handles for the partitions and install protocol > interfaces on these partitions. In advance of using the device. Basically we can pass the platform data into the device_bind() call, so it has it right from the start. Please understand that, while the existing EFI uclass is wonky, for the reasons I have clearly explained in this patch, my intent in explaining this is to motivate adding a new uclass. I am not trying to change the existing uclass. It is just one of many wonky things in the original EFI implementation that predates your efforts. - Simon > > Best regards > > Heinrich > > >>> > >>> 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. > > > > Again I'm not sure what you mean here. Please point to something you > > don't want in this series, which is focussed on the app. > > > >> > >> 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 > >>> > > > > Regards, > > Simon > >
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[];
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 - 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> --- 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