[RFC] efi_driver: fix a parent issue in efi-created block devices
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu Jul 20 04:56:15 CEST 2023
On Wed, Jul 19, 2023 at 07:29:57PM -0600, Simon Glass wrote:
> Hi,
>
> On Wed, 19 Jul 2023 at 18:14, AKASHI Takahiro
> <takahiro.akashi at linaro.org> wrote:
> >
> > On Wed, Jul 19, 2023 at 03:15:10PM +0200, Heinrich Schuchardt wrote:
> > > On 19.07.23 15:04, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro
> > > > <takahiro.akashi at linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote:
> > > > > > Hi AKASHI,
> > > > > >
> > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro
> > > > > > <takahiro.akashi at linaro.org> wrote:
> > > > > > >
> > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in
> > > > > > > EFI world, which in turn generates a corresponding U-Boot block device based on
> > > > > > > U-Boot's Driver Model.
> > > > > > > The latter device, however, doesn't work as U-Boot proper block device
> > > > > > > due to an issue in efi_driver's implementation. We saw discussions in the past,
> > > > > > > most recently in [1].
> > > > > > >
> > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html
> > > > > > >
> > > > > > > This RFC patch tries to address (part of) the issue.
> > > > > > > If it is considered acceptable, I will create a formal patch.
> > > > > > >
> > > > > > > Withtout this patch,
> > > > > > > ===8<===
> > > > > > > => env set efi_selftest 'block device'
> > > > > > > => bootefi selftest
> > > > > > > ...
> > > > > > >
> > > > > > > Summary: 0 failures
> > > > > > >
> > > > > > > => dm tree
> > > > > > > Class Index Probed Driver Name
> > > > > > > -----------------------------------------------------------
> > > > > > > root 0 [ + ] root_driver root_driver
> > > > > > > ...
> > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple
> > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0
> > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1
> > > > > > > => ls efiloader 0:1
> > > > > > > ** Bad device specification efiloader 0 **
> > > > > > > Couldn't find partition efiloader 0:1
> > > > > > > ===>8===
> > > > > > >
> > > > > > > With this patch applied, efiblk#0(:1) now gets accessible.
> > > > > > >
> > > > > > > ===8<===
> > > > > > > => env set efi_selftest 'block device'
> > > > > > > => bootefi selftest
> > > > > > > ...
> > > > > > >
> > > > > > > Summary: 0 failures
> > > > > > >
> > > > > > > => dm tree
> > > > > > > Class Index Probed Driver Name
> > > > > > > -----------------------------------------------------------
> > > > > > > root 0 [ + ] root_driver root_driver
> > > > > > > ...
> > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple
> > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)
> > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0
> > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1
> > > > > > > => ls efiloader 0:1
> > > > > > > 13 hello.txt
> > > > > > > 7 u-boot.txt
> > > > > > >
> > > > > > > 2 file(s), 0 dir(s)
> > > > > > > ===>8===
> > > > > > >
> > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > > > > > ---
> > > > > > > include/efi_driver.h | 2 +-
> > > > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++-----
> > > > > > > lib/efi_driver/efi_uclass.c | 8 +++++++-
> > > > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++
> > > > > > > 4 files changed, 22 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h
> > > > > > > index 63a95e4cf800..ed66796f3519 100644
> > > > > > > --- a/include/efi_driver.h
> > > > > > > +++ b/include/efi_driver.h
> > > > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops {
> > > > > > > const efi_guid_t *child_protocol;
> > > > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this);
> > > > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this,
> > > > > > > - efi_handle_t handle, void *interface);
> > > > > > > + efi_handle_t handle, void *interface, char *name);
> > > > > > > };
> > > > > > >
> > > > > > > #endif /* _EFI_DRIVER_H */
> > > > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> > > > > > > index add00eeebbea..43b7ed7c973c 100644
> > > > > > > --- a/lib/efi_driver/efi_block_device.c
> > > > > > > +++ b/lib/efi_driver/efi_block_device.c
> > > > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> > > > > > > * Return: status code
> > > > > > > */
> > > > > > > static efi_status_t
> > > > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface)
> > > > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent)
> > > > > > > {
> > > > > > > - struct udevice *bdev = NULL, *parent = dm_root();
> > > > > > > + struct udevice *bdev = NULL;
> > > > > > > efi_status_t ret;
> > > > > > > int devnum;
> > > > > > > char *name;
> > > > > > > @@ -181,7 +181,7 @@ err:
> > > > > > > */
> > > > > > > static efi_status_t efi_bl_bind(
> > > > > > > struct efi_driver_binding_extended_protocol *this,
> > > > > > > - efi_handle_t handle, void *interface)
> > > > > > > + efi_handle_t handle, void *interface, char *name)
> > > > > > > {
> > > > > > > efi_status_t ret = EFI_SUCCESS;
> > > > > > > struct efi_object *obj = efi_search_obj(handle);
> > > > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind(
> > > > > > > if (!obj || !interface)
> > > > > > > return EFI_INVALID_PARAMETER;
> > > > > > >
> > > > > > > - if (!handle->dev)
> > > > > > > - ret = efi_bl_create_block_device(handle, interface);
> > > > > > > + if (!handle->dev) {
> > > > > > > + struct udevice *parent;
> > > > > > > +
> > > > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent);
> > > > > >
> > > > > > Can you use a non-root device as the parent?
> > > > >
> > > > > I have no idea what else can be the parent in this case.
> > > >
> > > > I suggest an EFI_MEDIA device.
> > > >
> > > > >
> > > > > Please note:
> > > > > > > => dm tree
> > > > > > > Class Index Probed Driver Name
> > > > > > > -----------------------------------------------------------
> > > > > > > root 0 [ + ] root_driver root_driver
> > > > > > > ...
> > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple
> > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)
> > > > >
> > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c)
> > > > > and don't have any practical parent.
> > > >
> > > > Block devices must have a media device as their parent. This seems to
> > > > be a persistent area of confusion...probably when the uclass ID goes
> > > > away from blk_desc it will be more obvious.
> > >
> > > Dear Simon,
> > >
> > > The only reason why you request to add an otherwise parent device is
> > > that you use it to determine the device class name used in the CLI (mmc,
> > > usb, nvme, ...).
>
> Yes and also (at present) we number the devices within their uclass,
> so that we can have a block device 0 for both mmc and nvme, for
> example.
>
> > >
> > > That concept worked fine when all devices had physical parents from
> > > which such an information could be derived.
> > >
> > > This is not the case UCLASS_EFI block devices. We should not introduce
> > > any DM devices which have no meaning in the EFI world.
>
> Actually I feel the opposite. EFI should just be using DM devices. If
> they don't exist, create them. EFI cannot be a parallel universe.
>
> >
> > Regarding my RFC patch, I have not invented any new DM device, instead
> > I reuse the existing one, UCLASS_EFI_LOADER, which strangely never appears
> > in DM tree under the current implementation.
> >
> > With my patch, a new instance (device) is created and associated with
> > a "controller handle" (in UEFI jargon) which is passed on to
> > EFI_DRIVER_BINDING_PROTOCOL.start() by a UEFI app.
> > So the hierarchy looks like:
> > ROOT
> > UCLASS_EFI_LOADER - controller
> > UCLASS_BLK - raw device
> > UCLASS_PARTITION - partition
> >
> > It seems to me that it perfectly matches to DM concept.
> > It has nothing different from other ordinary block devices.
>
> Yes that seems fine to me. I'm sorry that I misunderstood that. Should
> we use EFI_MEDIA instead of EFI_LOADER?
My understanding is as follows:
UCLASS_EFI_MEDIA is usable when U-Boot is invoked as an UEFI application against
the underlying UEFI firmware. On the other hand, UCLASS_EFI_LOADER serves an UEFI
application loaded via U-Boot UEFI when U-Boot works as UEFI firmware.
So I don't think that the two uclasses may not co-exist (or unified).
>
> >
> > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)
> >
> > The guid here is exactly what you gave to the controller handle
> > (disk_handle) in your lib/efi_selftest/efi_selftest_block_device.c.
>
> What does it mean? We need to use names instead of GUIDs. I never want
> a guid to be shown to the poor, hard-pressed, confused user. It would
> be like using a sha256 hash instead of a filename.
Well, what matters here is not a guid, but a UEFI device path which
represents an UEFI object uniquely in UEFI world.
"/VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)" is a human-readable
presentation for the device path given to the controller handle
by an UEFI app (again, this is efi_selftest_block_device.c).
Since it may be in another form (possibly without guid?) in another
application, we have no control over the naming here.
I even believe that using a device path as a udevice's name
is sane because it is the only way to interwine the two worlds.
-Takahiro Akashi
>
> >
> > -Takahiro Akashi
> >
> > > If there is no other benefit, we should do the reasonable and keep a
> > > field in blk_desc and use it to derive the CLI name of the block device.
>
> I don't see any down-side to having a parent device like EFI_MEDIA. Is
> there one?
>
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
>
> Regards,
> Simon
More information about the U-Boot
mailing list