Hi, On Wed, 19 Jul 2023 at 18:14, AKASHI Takahiro <takahiro.aka...@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.aka...@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.aka...@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.aka...@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? > > > > > > > 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. > > -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
