From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0C48CC0015E for ; Thu, 20 Jul 2023 02:56:26 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0E2A68677C; Thu, 20 Jul 2023 04:56:25 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="g6JpkOoo"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7FED486797; Thu, 20 Jul 2023 04:56:23 +0200 (CEST) Received: from mail-pl1-x636.google.com (mail-pl1-x636.google.com [IPv6:2607:f8b0:4864:20::636]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E9BF786719 for ; Thu, 20 Jul 2023 04:56:19 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pl1-x636.google.com with SMTP id d9443c01a7336-1bb106ad293so556825ad.0 for ; Wed, 19 Jul 2023 19:56:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1689821778; x=1690426578; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=7Y5DBh/P5AJR1LSBLhPy06R3T1R0tNpRrXnP08vTejQ=; b=g6JpkOooskpATJtq3VId66uHD7ZLaf731QWjbEo5Z9SgARJrIp6tM/uijygz2v723o o5BkLF7H1PTccMjV2O7DAmqhCpxbKTGbI8Zkzm7wkOqjwwa33dn88SWQ7XfXGAw+r16T sP4jSkoJpB5/pJDLKltWLWH4tOWqwk5zbQCjqKXv9C++2XghJm8kgnajfNN/ch5q5dBJ 2ph4lS8dl/2qgRIoQ4qKNpfnXc8yM2zJmZqWsXdq3m/7MHOlxM9td6YUVe0Z4WDV+MTx 3dW9/37xyv9NGxxgZNu+59o+Emr1Q41I6rqo+OQIi3PXWPD67jWSun7F78o15u+U6FNt 9kpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689821778; x=1690426578; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7Y5DBh/P5AJR1LSBLhPy06R3T1R0tNpRrXnP08vTejQ=; b=I5/VxyCn8xLC/tEhbXWvdFOydTsZZHJJGm/VJM4rOudxoygH/Lwl6KOyzgbXt9M1fS eV8Zi/3upKWG5RH5guKS7ZrVc0/0FLyDfwT/Y01GIodH97Dhvdk/fg2GPxC/REZw7oK1 iKp+PTWRxtAwlDediP880lYx8mdjjWEjE81NSYkCAdWuFCLQjuFB28/MNnvjz1/vnRon cY1mtJNo6hGu3pzpUJxoCEgNkhTYPzMT+YiC2RavD/VFH4f6jb0qwIAMI1lstLBACGN7 hr9oSfccwwWe28UCEsxROaEfI4caNU8DW/FVuaFD0TH8c77SAKWExnhi5bMK2sF265VJ RCHw== X-Gm-Message-State: ABy/qLZHIxV2AfvAZv8idbBIKrni3Wm37mkuiQlW2jdWvAiTsebEczjO 28bBUmIMDjxbT4SlxZ2KBWYgGw== X-Google-Smtp-Source: APBJJlHsHh1VLCOwKrfCuxklO8KzgJ8Hn5/sj3BR53HWa55RryT0PVWj+OZTOFIx65ML4Au3w84xlw== X-Received: by 2002:a17:902:f548:b0:1b8:17e8:5472 with SMTP id h8-20020a170902f54800b001b817e85472mr1973636plf.1.1689821778133; Wed, 19 Jul 2023 19:56:18 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:b19f:5d18:5b09:a6df]) by smtp.gmail.com with ESMTPSA id g15-20020a1709029f8f00b001b8b2a6c4a4sm4731751plq.172.2023.07.19.19.56.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Jul 2023 19:56:17 -0700 (PDT) Date: Thu, 20 Jul 2023 11:56:15 +0900 From: AKASHI Takahiro To: Simon Glass Cc: Heinrich Schuchardt , masahisa.kojima@linaro.org, u-boot@lists.denx.de Subject: Re: [RFC] efi_driver: fix a parent issue in efi-created block devices Message-ID: Mail-Followup-To: AKASHI Takahiro , Simon Glass , Heinrich Schuchardt , masahisa.kojima@linaro.org, u-boot@lists.denx.de References: <20230719002158.27004-1-takahiro.akashi@linaro.org> <30f22c59-c6ce-1cea-9ef0-bdc31f7d57c2@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On Wed, Jul 19, 2023 at 07:29:57PM -0600, Simon Glass wrote: > Hi, > > On Wed, 19 Jul 2023 at 18:14, AKASHI Takahiro > 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 > > > > 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 > > > > > > 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 > > > > > > > --- > > > > > > > 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