[RFC] efi_driver: fix a parent issue in efi-created block devices
Simon Glass
sjg at chromium.org
Thu Jul 27 02:49:54 CEST 2023
Hi,
On Wed, 19 Jul 2023 at 20:56, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> 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).
It looks like EFI_LOADER is for strange EFI things - it messes around
in the bowels of driver model in ways that it probably shouldn't. But
it seems to be used as a media device:
devnum = blk_next_free_devnum(UCLASS_EFI_LOADER)
For EFI_MEDIA, yes it is used in the app, although I don't see a
problem with it being used elsewhere.
But we should settle on one, I think.
>
> >
> > >
> > > > > > > > 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.
Really? We need a way to show meaningful names and not GUIDs.
>
> 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.
I very much want to avoid ever needing to read a GUID with driver
model. Since there must be a corresponding driver model device for
each EFI device, this should be possible. For example, we name MMC
devices based on their DT node name, we can name other devices based
on their uclass name and sequence number, block devices based on their
parent, etc.
Regards,
Simon
>
> -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?
> >
Regards,
Simon
More information about the U-Boot
mailing list