| Message ID | 20210908073355.11.Id572ddf5eb457096b95d30ae2d8675f539367fe8@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: > This code should never have been added as it builds a new feature on top > of legacy code. Drop it and add a dependency on BLK for this feature. > > Boards which want EFI_LOADER should migrate to driver model first. > > Signed-off-by: Simon Glass <sjg@chromium.org> This patch is not related to the rest of the series and the code has a different maintainer. So, please, separate it from the series. Best regards Heinrich > --- > > lib/efi_driver/Makefile | 2 +- > lib/efi_loader/Kconfig | 2 + > lib/efi_loader/efi_device_path.c | 96 +++++++------------------------- > lib/efi_loader/efi_disk.c | 48 ---------------- > 4 files changed, 24 insertions(+), 124 deletions(-) > > diff --git a/lib/efi_driver/Makefile b/lib/efi_driver/Makefile > index 83baa1c9a49..f2b6c05cc24 100644 > --- a/lib/efi_driver/Makefile > +++ b/lib/efi_driver/Makefile > @@ -6,6 +6,6 @@ > # object inclusion implicitly depends on it > > obj-y += efi_uclass.o > -ifeq ($(CONFIG_BLK)$(CONFIG_PARTITIONS),yy) > +ifeq ($(CONFIG_PARTITIONS),y) > obj-y += efi_block_device.o > endif > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index dacc3b58810..799aa1a7512 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -10,6 +10,8 @@ config EFI_LOADER > depends on !EFI_STUB || !X86_64 || EFI_STUB_64BIT > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > + depends on BLK > + depends on DM_ETH || !NET > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > select LIB_UUID > select PARTITION_UUIDS > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c > index cbdb466da41..a09090a32e4 100644 > --- a/lib/efi_loader/efi_device_path.c > +++ b/lib/efi_loader/efi_device_path.c > @@ -46,7 +46,7 @@ static const struct efi_device_path_vendor ROOT = { > .guid = U_BOOT_GUID, > }; > > -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) > +#if defined(CONFIG_MMC) > /* > * Determine if an MMC device is an SD card. > * > @@ -486,7 +486,6 @@ bool efi_dp_is_multi_instance(const struct efi_device_path *dp) > return p->sub_type == DEVICE_PATH_SUB_TYPE_INSTANCE_END; > } > > -#ifdef CONFIG_DM > /* size of device-path not including END node for device and all parents > * up to the root device. > */ > @@ -503,7 +502,6 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) > case UCLASS_ETH: > return dp_size(dev->parent) + > sizeof(struct efi_device_path_mac_addr); > -#ifdef CONFIG_BLK > case UCLASS_BLK: > switch (dev->parent->uclass->uc_drv->id) { > #ifdef CONFIG_IDE > @@ -511,12 +509,12 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) > return dp_size(dev->parent) + > sizeof(struct efi_device_path_atapi); > #endif > -#if defined(CONFIG_SCSI) && defined(CONFIG_DM_SCSI) > +#if defined(CONFIG_SCSI) > case UCLASS_SCSI: > return dp_size(dev->parent) + > sizeof(struct efi_device_path_scsi); > #endif > -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) > +#if defined(CONFIG_MMC) > case UCLASS_MMC: > return dp_size(dev->parent) + > sizeof(struct efi_device_path_sd_mmc_path); > @@ -554,8 +552,7 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) > default: > return dp_size(dev->parent); > } > -#endif > -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) > +#if defined(CONFIG_MMC) > case UCLASS_MMC: > return dp_size(dev->parent) + > sizeof(struct efi_device_path_sd_mmc_path); > @@ -590,7 +587,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > *vdp = ROOT; > return &vdp[1]; > } > -#ifdef CONFIG_DM_ETH > +#ifdef CONFIG_NET > case UCLASS_ETH: { > struct efi_device_path_mac_addr *dp = > dp_fill(buf, dev->parent); > @@ -607,7 +604,6 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > return &dp[1]; > } > #endif > -#ifdef CONFIG_BLK > case UCLASS_BLK: > switch (dev->parent->uclass->uc_drv->id) { > #ifdef CONFIG_SANDBOX > @@ -662,7 +658,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > return &dp[1]; > } > #endif > -#if defined(CONFIG_SCSI) && defined(CONFIG_DM_SCSI) > +#if defined(CONFIG_SCSI) > case UCLASS_SCSI: { > struct efi_device_path_scsi *dp = > dp_fill(buf, dev->parent); > @@ -676,7 +672,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > return &dp[1]; > } > #endif > -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) > +#if defined(CONFIG_MMC) > case UCLASS_MMC: { > struct efi_device_path_sd_mmc_path *sddp = > dp_fill(buf, dev->parent); > @@ -727,8 +723,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > dev->name, dev->parent->uclass->uc_drv->id); > return dp_fill(buf, dev->parent); > } > -#endif > -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) > +#if defined(CONFIG_MMC) > case UCLASS_MMC: { > struct efi_device_path_sd_mmc_path *sddp = > dp_fill(buf, dev->parent); > @@ -770,24 +765,18 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > return dp_fill(buf, dev->parent); > } > } > -#endif > > static unsigned dp_part_size(struct blk_desc *desc, int part) > { > unsigned dpsize; > + struct udevice *dev; > + int ret; > > -#ifdef CONFIG_BLK > - { > - struct udevice *dev; > - int ret = blk_find_device(desc->if_type, desc->devnum, &dev); > + ret = blk_find_device(desc->if_type, desc->devnum, &dev); > > - if (ret) > - dev = desc->bdev->parent; > - dpsize = dp_size(dev); > - } > -#else > - dpsize = sizeof(ROOT) + sizeof(struct efi_device_path_usb); > -#endif > + if (ret) > + dev = desc->bdev->parent; > + dpsize = dp_size(dev); > > if (part == 0) /* the actual disk, not a partition */ > return dpsize; > @@ -877,36 +866,14 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part) > */ > static void *dp_part_fill(void *buf, struct blk_desc *desc, int part) > { > -#ifdef CONFIG_BLK > - { > - struct udevice *dev; > - int ret = blk_find_device(desc->if_type, desc->devnum, &dev); > + struct udevice *dev; > + int ret; > > - if (ret) > - dev = desc->bdev->parent; > - buf = dp_fill(buf, dev); > - } > -#else > - /* > - * We *could* make a more accurate path, by looking at if_type > - * and handling all the different cases like we do for non- > - * legacy (i.e. CONFIG_BLK=y) case. But most important thing > - * is just to have a unique device-path for if_type+devnum. > - * So map things to a fictitious USB device. > - */ > - struct efi_device_path_usb *udp; > - > - memcpy(buf, &ROOT, sizeof(ROOT)); > - buf += sizeof(ROOT); > - > - udp = buf; > - udp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > - udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB; > - udp->dp.length = sizeof(*udp); > - udp->parent_port_number = desc->if_type; > - udp->usb_interface = desc->devnum; > - buf = &udp[1]; > -#endif > + ret = blk_find_device(desc->if_type, desc->devnum, &dev); > + > + if (ret) > + dev = desc->bdev->parent; > + buf = dp_fill(buf, dev); > > if (part == 0) /* the actual disk, not a partition */ > return buf; > @@ -1051,39 +1018,18 @@ struct efi_device_path *efi_dp_from_uart(void) > #ifdef CONFIG_NET > struct efi_device_path *efi_dp_from_eth(void) > { > -#ifndef CONFIG_DM_ETH > - struct efi_device_path_mac_addr *ndp; > -#endif > void *buf, *start; > unsigned dpsize = 0; > > assert(eth_get_dev()); > > -#ifdef CONFIG_DM_ETH > dpsize += dp_size(eth_get_dev()); > -#else > - dpsize += sizeof(ROOT); > - dpsize += sizeof(*ndp); > -#endif > > start = buf = dp_alloc(dpsize + sizeof(END)); > if (!buf) > return NULL; > > -#ifdef CONFIG_DM_ETH > buf = dp_fill(buf, eth_get_dev()); > -#else > - memcpy(buf, &ROOT, sizeof(ROOT)); > - buf += sizeof(ROOT); > - > - ndp = buf; > - ndp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > - ndp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR; > - ndp->dp.length = sizeof(*ndp); > - ndp->if_type = 1; /* Ethernet */ > - memcpy(ndp->mac.addr, eth_get_ethaddr(), ARP_HLEN); > - buf = &ndp[1]; > -#endif > > *((struct efi_device_path *)buf) = END; > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 988907ecb91..ef8b5c88ff9 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -555,7 +555,6 @@ efi_status_t efi_disk_register(void) > struct efi_disk_obj *disk; > int disks = 0; > efi_status_t ret; > -#ifdef CONFIG_BLK > struct udevice *dev; > > for (uclass_first_device_check(UCLASS_BLK, &dev); dev; > @@ -583,54 +582,7 @@ efi_status_t efi_disk_register(void) > &disk->header, desc, if_typename, > desc->devnum, dev->name); > } > -#else > - int i, if_type; > > - /* Search for all available disk devices */ > - for (if_type = 0; if_type < IF_TYPE_COUNT; if_type++) { > - const struct blk_driver *cur_drvr; > - const char *if_typename; > - > - cur_drvr = blk_driver_lookup_type(if_type); > - if (!cur_drvr) > - continue; > - > - if_typename = cur_drvr->if_typename; > - log_info("Scanning disks on %s...\n", if_typename); > - for (i = 0; i < 4; i++) { > - struct blk_desc *desc; > - char devname[32] = { 0 }; /* dp->str is u16[32] long */ > - > - desc = blk_get_devnum_by_type(if_type, i); > - if (!desc) > - continue; > - if (desc->type == DEV_TYPE_UNKNOWN) > - continue; > - > - snprintf(devname, sizeof(devname), "%s%d", > - if_typename, i); > - > - /* Add block device for the full device */ > - ret = efi_disk_add_dev(NULL, NULL, if_typename, desc, > - i, NULL, 0, &disk); > - if (ret == EFI_NOT_READY) { > - log_notice("Disk %s not ready\n", devname); > - continue; > - } > - if (ret) { > - log_err("ERROR: failure to add disk device %s, r = %lu\n", > - devname, ret & ~EFI_ERROR_MASK); > - return ret; > - } > - disks++; > - > - /* Partitions show up as block devices in EFI */ > - disks += efi_disk_create_partitions > - (&disk->header, desc, > - if_typename, i, devname); > - } > - } > -#endif > log_info("Found %d disks\n", disks); > > return EFI_SUCCESS; >
Hi Heinrich, On Wed, 8 Sept 2021 at 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 9/8/21 3:33 PM, Simon Glass wrote: > > This code should never have been added as it builds a new feature on top > > of legacy code. Drop it and add a dependency on BLK for this feature. > > > > Boards which want EFI_LOADER should migrate to driver model first. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > This patch is not related to the rest of the series and the code has a > different maintainer. > > So, please, separate it from the series. Who is the maintainer? I need this patch for this series to work. You can still review things for other maintainers and in this case it is common for one maintainer to pick up the series once the others are happy. Regards, Simon
On 9/9/21 10:57 AM, Simon Glass wrote: > Hi Heinrich, > > On Wed, 8 Sept 2021 at 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> >> >> On 9/8/21 3:33 PM, Simon Glass wrote: >>> This code should never have been added as it builds a new feature on top >>> of legacy code. Drop it and add a dependency on BLK for this feature. >>> >>> Boards which want EFI_LOADER should migrate to driver model first. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >> This patch is not related to the rest of the series and the code has a >> different maintainer. >> >> So, please, separate it from the series. > > Who is the maintainer? Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") there was no official maintainer for lib/efi/ but you were the main contributor. But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. I am happy if you would continue to care about U-Boot on EFI. > > I need this patch for this series to work. You can still review things > for other maintainers and in this case it is common for one maintainer > to pick up the series once the others are happy. The direction of this patch is completely correct. There are some things that will have to be changed, e.g we should not require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. I already added CONFIG_BLK as a requirement for CONFIG_EFI_LOADER in a submitted patch. Removing legacy code from lib/efi_loader/efi_disk.c and lib/efi_loader/efi_device_path.c could be done before all U-Boot on EFI patches. Therefore I still think it makes sense to split the series in two: 1) Cleanup of the UEFI implementation 2) Rework of U-Boot on EFI I hope merging in this sequence of patch series makes send to you. Best regards Heinrich
Hi Heinrich, On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> > >> > >> On 9/8/21 3:33 PM, Simon Glass wrote: > >>> This code should never have been added as it builds a new feature on top > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > >>> > >>> Boards which want EFI_LOADER should migrate to driver model first. > >>> > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > >> > >> This patch is not related to the rest of the series and the code has a > >> different maintainer. > >> > >> So, please, separate it from the series. > > > > Who is the maintainer? > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > there was no official maintainer for lib/efi/ but you were the main > contributor. > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > I am happy if you would continue to care about U-Boot on EFI. OK. > > > > > I need this patch for this series to work. You can still review things > > for other maintainers and in this case it is common for one maintainer > > to pick up the series once the others are happy. > > The direction of this patch is completely correct. > > There are some things that will have to be changed, e.g we should not > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. OK, but why not require DM_ETH? The deadline passed a year ago. > > I already added CONFIG_BLK as a requirement for CONFIG_EFI_LOADER in a > submitted patch. OK good. > > Removing legacy code from lib/efi_loader/efi_disk.c and > lib/efi_loader/efi_device_path.c could be done before all U-Boot on EFI > patches. > > Therefore I still think it makes sense to split the series in two: > > 1) Cleanup of the UEFI implementation > 2) Rework of U-Boot on EFI > > I hope merging in this sequence of patch series makes send to you. I am fine if you want to take on the clean-up stuff for BLK, etc. In that case we can just drop this patch when applying. But I do need some sort of patch here, for things to work. Regards, Simon
On Thu, Sep 09, 2021 at 01:57:39PM -0600, Simon Glass wrote: > Hi Heinrich, > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> > > >> > > >> > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > >>> This code should never have been added as it builds a new feature on top > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > >>> > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > >>> > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > >> > > >> This patch is not related to the rest of the series and the code has a > > >> different maintainer. > > >> > > >> So, please, separate it from the series. > > > > > > Who is the maintainer? > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > there was no official maintainer for lib/efi/ but you were the main > > contributor. > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > I am happy if you would continue to care about U-Boot on EFI. > > OK. > > > > > > > > > I need this patch for this series to work. You can still review things > > > for other maintainers and in this case it is common for one maintainer > > > to pick up the series once the others are happy. > > > > The direction of this patch is completely correct. > > > > There are some things that will have to be changed, e.g we should not > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > OK, but why not require DM_ETH? The deadline passed a year ago. Note that it's still a while (about a year) before I forcefully yank out unmigrated systems.
> From: Simon Glass <sjg@chromium.org> > Date: Thu, 9 Sep 2021 13:57:39 -0600 > > Hi Heinrich, > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> > > >> > > >> > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > >>> This code should never have been added as it builds a new feature on top > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > >>> > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > >>> > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > >> > > >> This patch is not related to the rest of the series and the code has a > > >> different maintainer. > > >> > > >> So, please, separate it from the series. > > > > > > Who is the maintainer? > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > there was no official maintainer for lib/efi/ but you were the main > > contributor. > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > I am happy if you would continue to care about U-Boot on EFI. > > OK. > > > > > > > > > I need this patch for this series to work. You can still review things > > > for other maintainers and in this case it is common for one maintainer > > > to pick up the series once the others are happy. > > > > The direction of this patch is completely correct. > > > > There are some things that will have to be changed, e.g we should not > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > OK, but why not require DM_ETH? The deadline passed a year ago. Because we support boards without network ports?
On Thu, Sep 09, 2021 at 10:15:44PM +0200, Mark Kettenis wrote: > > From: Simon Glass <sjg@chromium.org> > > Date: Thu, 9 Sep 2021 13:57:39 -0600 > > > > Hi Heinrich, > > > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > >> > > > >> > > > >> > > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > > >>> This code should never have been added as it builds a new feature on top > > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > > >>> > > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > > >>> > > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > >> > > > >> This patch is not related to the rest of the series and the code has a > > > >> different maintainer. > > > >> > > > >> So, please, separate it from the series. > > > > > > > > Who is the maintainer? > > > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > > there was no official maintainer for lib/efi/ but you were the main > > > contributor. > > > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > > > I am happy if you would continue to care about U-Boot on EFI. > > > > OK. > > > > > > > > > > > > > I need this patch for this series to work. You can still review things > > > > for other maintainers and in this case it is common for one maintainer > > > > to pick up the series once the others are happy. > > > > > > The direction of this patch is completely correct. > > > > > > There are some things that will have to be changed, e.g we should not > > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > > > OK, but why not require DM_ETH? The deadline passed a year ago. > > Because we support boards without network ports? Boards without networking should disable the relevant code, and as needed the EFI code return the proper error code?
> Date: Thu, 9 Sep 2021 16:23:08 -0400 > From: Tom Rini <trini@konsulko.com> > > On Thu, Sep 09, 2021 at 10:15:44PM +0200, Mark Kettenis wrote: > > > From: Simon Glass <sjg@chromium.org> > > > Date: Thu, 9 Sep 2021 13:57:39 -0600 > > > > > > Hi Heinrich, > > > > > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > >> > > > > >> > > > > >> > > > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > > > >>> This code should never have been added as it builds a new feature on top > > > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > > > >>> > > > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > > > >>> > > > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > > >> > > > > >> This patch is not related to the rest of the series and the code has a > > > > >> different maintainer. > > > > >> > > > > >> So, please, separate it from the series. > > > > > > > > > > Who is the maintainer? > > > > > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > > > there was no official maintainer for lib/efi/ but you were the main > > > > contributor. > > > > > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > > > > > I am happy if you would continue to care about U-Boot on EFI. > > > > > > OK. > > > > > > > > > > > > > > > > > I need this patch for this series to work. You can still review things > > > > > for other maintainers and in this case it is common for one maintainer > > > > > to pick up the series once the others are happy. > > > > > > > > The direction of this patch is completely correct. > > > > > > > > There are some things that will have to be changed, e.g we should not > > > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > > > > > OK, but why not require DM_ETH? The deadline passed a year ago. > > > > Because we support boards without network ports? > > Boards without networking should disable the relevant code, and as > needed the EFI code return the proper error code? Yes, but it means you can't make DM_ETH a (hard) requirement for EFI_LOADER support. What I mean is that it should still be possible to build U-Boot with EFI_LOADER support even if DM_EFI isn't set for a board. It should just result in a UEFI implementation with no network support instead.
On Thu, Sep 09, 2021 at 11:45:08PM +0200, Mark Kettenis wrote: > > Date: Thu, 9 Sep 2021 16:23:08 -0400 > > From: Tom Rini <trini@konsulko.com> > > > > On Thu, Sep 09, 2021 at 10:15:44PM +0200, Mark Kettenis wrote: > > > > From: Simon Glass <sjg@chromium.org> > > > > Date: Thu, 9 Sep 2021 13:57:39 -0600 > > > > > > > > Hi Heinrich, > > > > > > > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > >> > > > > > >> > > > > > >> > > > > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > > > > >>> This code should never have been added as it builds a new feature on top > > > > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > > > > >>> > > > > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > > > > >>> > > > > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > >> > > > > > >> This patch is not related to the rest of the series and the code has a > > > > > >> different maintainer. > > > > > >> > > > > > >> So, please, separate it from the series. > > > > > > > > > > > > Who is the maintainer? > > > > > > > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > > > > there was no official maintainer for lib/efi/ but you were the main > > > > > contributor. > > > > > > > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > > > > > > > I am happy if you would continue to care about U-Boot on EFI. > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > I need this patch for this series to work. You can still review things > > > > > > for other maintainers and in this case it is common for one maintainer > > > > > > to pick up the series once the others are happy. > > > > > > > > > > The direction of this patch is completely correct. > > > > > > > > > > There are some things that will have to be changed, e.g we should not > > > > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > > > > > > > OK, but why not require DM_ETH? The deadline passed a year ago. > > > > > > Because we support boards without network ports? > > > > Boards without networking should disable the relevant code, and as > > needed the EFI code return the proper error code? > > Yes, but it means you can't make DM_ETH a (hard) requirement for > EFI_LOADER support. What I mean is that it should still be possible > to build U-Boot with EFI_LOADER support even if DM_EFI isn't set for a > board. It should just result in a UEFI implementation with no network > support instead. Yes, agreed. I was just trying to say that in the context of what DM code EFI_LOADER can demand, the deadline for BLK has passed and everything that didn't support it has been removed, so that's a good requirement and area of code to clean up as needed. But DM_ETH-or-bust isn't there, yet.
Hi Mark, On Thu, 9 Sept 2021 at 15:45, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > Date: Thu, 9 Sep 2021 16:23:08 -0400 > > From: Tom Rini <trini@konsulko.com> > > > > On Thu, Sep 09, 2021 at 10:15:44PM +0200, Mark Kettenis wrote: > > > > From: Simon Glass <sjg@chromium.org> > > > > Date: Thu, 9 Sep 2021 13:57:39 -0600 > > > > > > > > Hi Heinrich, > > > > > > > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > >> > > > > > >> > > > > > >> > > > > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > > > > >>> This code should never have been added as it builds a new feature on top > > > > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > > > > >>> > > > > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > > > > >>> > > > > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > >> > > > > > >> This patch is not related to the rest of the series and the code has a > > > > > >> different maintainer. > > > > > >> > > > > > >> So, please, separate it from the series. > > > > > > > > > > > > Who is the maintainer? > > > > > > > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > > > > there was no official maintainer for lib/efi/ but you were the main > > > > > contributor. > > > > > > > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > > > > > > > I am happy if you would continue to care about U-Boot on EFI. > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > I need this patch for this series to work. You can still review things > > > > > > for other maintainers and in this case it is common for one maintainer > > > > > > to pick up the series once the others are happy. > > > > > > > > > > The direction of this patch is completely correct. > > > > > > > > > > There are some things that will have to be changed, e.g we should not > > > > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > > > > > > > OK, but why not require DM_ETH? The deadline passed a year ago. > > > > > > Because we support boards without network ports? > > > > Boards without networking should disable the relevant code, and as > > needed the EFI code return the proper error code? > > Yes, but it means you can't make DM_ETH a (hard) requirement for > EFI_LOADER support. What I mean is that it should still be possible > to build U-Boot with EFI_LOADER support even if DM_EFI isn't set for a > board. It should just result in a UEFI implementation with no network > support instead. I think you are misunderstanding my patch. I have: depends on DM_ETH || !NET which means that if NET is used, DM_ETH must be. I think that is reasonable. Regards, Simon
> From: Simon Glass <sjg@chromium.org> > Date: Thu, 23 Sep 2021 20:48:42 -0600 > > Hi Mark, > > On Thu, 9 Sept 2021 at 15:45, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > Date: Thu, 9 Sep 2021 16:23:08 -0400 > > > From: Tom Rini <trini@konsulko.com> > > > > > > On Thu, Sep 09, 2021 at 10:15:44PM +0200, Mark Kettenis wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > > > Date: Thu, 9 Sep 2021 13:57:39 -0600 > > > > > > > > > > Hi Heinrich, > > > > > > > > > > On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> On 9/8/21 3:33 PM, Simon Glass wrote: > > > > > > >>> This code should never have been added as it builds a new feature on top > > > > > > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > > > > > > >>> > > > > > > >>> Boards which want EFI_LOADER should migrate to driver model first. > > > > > > >>> > > > > > > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > >> > > > > > > >> This patch is not related to the rest of the series and the code has a > > > > > > >> different maintainer. > > > > > > >> > > > > > > >> So, please, separate it from the series. > > > > > > > > > > > > > > Who is the maintainer? > > > > > > > > > > > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > > > > > > there was no official maintainer for lib/efi/ but you were the main > > > > > > contributor. > > > > > > > > > > > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > > > > > > > > > > > I am happy if you would continue to care about U-Boot on EFI. > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > > > > > I need this patch for this series to work. You can still review things > > > > > > > for other maintainers and in this case it is common for one maintainer > > > > > > > to pick up the series once the others are happy. > > > > > > > > > > > > The direction of this patch is completely correct. > > > > > > > > > > > > There are some things that will have to be changed, e.g we should not > > > > > > require CONFIG_DM_ETH=y. I will work on reviewing this patch in detail. > > > > > > > > > > OK, but why not require DM_ETH? The deadline passed a year ago. > > > > > > > > Because we support boards without network ports? > > > > > > Boards without networking should disable the relevant code, and as > > > needed the EFI code return the proper error code? > > > > Yes, but it means you can't make DM_ETH a (hard) requirement for > > EFI_LOADER support. What I mean is that it should still be possible > > to build U-Boot with EFI_LOADER support even if DM_EFI isn't set for a > > board. It should just result in a UEFI implementation with no network > > support instead. > > I think you are misunderstanding my patch. I have: > > depends on DM_ETH || !NET > > which means that if NET is used, DM_ETH must be. I think that is reasonable. Yes, that should be fine (Kconfig stuff doesn't always make sense to me).
Hi Heinrich, On Thu, 9 Sept 2021 at 03:26, 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 11:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> > >> > >> On 9/8/21 3:33 PM, Simon Glass wrote: > >>> This code should never have been added as it builds a new feature on top > >>> of legacy code. Drop it and add a dependency on BLK for this feature. > >>> > >>> Boards which want EFI_LOADER should migrate to driver model first. > >>> > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > >> > >> This patch is not related to the rest of the series and the code has a > >> different maintainer. > >> > >> So, please, separate it from the series. > > > > Who is the maintainer? > > Until 623b3a57976 ("efi_selftest: provide an EFI selftest application") > there was no official maintainer for lib/efi/ but you were the main > contributor. > > But with that patch directory lib/efi/ was assigned to EFI PAYLOAD. > > I am happy if you would continue to care about U-Boot on EFI. I can add myself as maintainer of that part of EFI if you like. I will include it in the v2 series. But it is a little fiddling because there are shared files, so I will just add a few for now. Perhaps you could be co-maintainer for the app, given your EFI knowledge and that you review them? Also I expect they would still go through your tree. Regards, Simon
diff --git a/lib/efi_driver/Makefile b/lib/efi_driver/Makefile index 83baa1c9a49..f2b6c05cc24 100644 --- a/lib/efi_driver/Makefile +++ b/lib/efi_driver/Makefile @@ -6,6 +6,6 @@ # object inclusion implicitly depends on it obj-y += efi_uclass.o -ifeq ($(CONFIG_BLK)$(CONFIG_PARTITIONS),yy) +ifeq ($(CONFIG_PARTITIONS),y) obj-y += efi_block_device.o endif diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index dacc3b58810..799aa1a7512 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -10,6 +10,8 @@ config EFI_LOADER depends on !EFI_STUB || !X86_64 || EFI_STUB_64BIT # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT + depends on BLK + depends on DM_ETH || !NET default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select LIB_UUID select PARTITION_UUIDS diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index cbdb466da41..a09090a32e4 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -46,7 +46,7 @@ static const struct efi_device_path_vendor ROOT = { .guid = U_BOOT_GUID, }; -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) +#if defined(CONFIG_MMC) /* * Determine if an MMC device is an SD card. * @@ -486,7 +486,6 @@ bool efi_dp_is_multi_instance(const struct efi_device_path *dp) return p->sub_type == DEVICE_PATH_SUB_TYPE_INSTANCE_END; } -#ifdef CONFIG_DM /* size of device-path not including END node for device and all parents * up to the root device. */ @@ -503,7 +502,6 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) case UCLASS_ETH: return dp_size(dev->parent) + sizeof(struct efi_device_path_mac_addr); -#ifdef CONFIG_BLK case UCLASS_BLK: switch (dev->parent->uclass->uc_drv->id) { #ifdef CONFIG_IDE @@ -511,12 +509,12 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) return dp_size(dev->parent) + sizeof(struct efi_device_path_atapi); #endif -#if defined(CONFIG_SCSI) && defined(CONFIG_DM_SCSI) +#if defined(CONFIG_SCSI) case UCLASS_SCSI: return dp_size(dev->parent) + sizeof(struct efi_device_path_scsi); #endif -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) +#if defined(CONFIG_MMC) case UCLASS_MMC: return dp_size(dev->parent) + sizeof(struct efi_device_path_sd_mmc_path); @@ -554,8 +552,7 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) default: return dp_size(dev->parent); } -#endif -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) +#if defined(CONFIG_MMC) case UCLASS_MMC: return dp_size(dev->parent) + sizeof(struct efi_device_path_sd_mmc_path); @@ -590,7 +587,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) *vdp = ROOT; return &vdp[1]; } -#ifdef CONFIG_DM_ETH +#ifdef CONFIG_NET case UCLASS_ETH: { struct efi_device_path_mac_addr *dp = dp_fill(buf, dev->parent); @@ -607,7 +604,6 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) return &dp[1]; } #endif -#ifdef CONFIG_BLK case UCLASS_BLK: switch (dev->parent->uclass->uc_drv->id) { #ifdef CONFIG_SANDBOX @@ -662,7 +658,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) return &dp[1]; } #endif -#if defined(CONFIG_SCSI) && defined(CONFIG_DM_SCSI) +#if defined(CONFIG_SCSI) case UCLASS_SCSI: { struct efi_device_path_scsi *dp = dp_fill(buf, dev->parent); @@ -676,7 +672,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) return &dp[1]; } #endif -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) +#if defined(CONFIG_MMC) case UCLASS_MMC: { struct efi_device_path_sd_mmc_path *sddp = dp_fill(buf, dev->parent); @@ -727,8 +723,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) dev->name, dev->parent->uclass->uc_drv->id); return dp_fill(buf, dev->parent); } -#endif -#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC) +#if defined(CONFIG_MMC) case UCLASS_MMC: { struct efi_device_path_sd_mmc_path *sddp = dp_fill(buf, dev->parent); @@ -770,24 +765,18 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) return dp_fill(buf, dev->parent); } } -#endif static unsigned dp_part_size(struct blk_desc *desc, int part) { unsigned dpsize; + struct udevice *dev; + int ret; -#ifdef CONFIG_BLK - { - struct udevice *dev; - int ret = blk_find_device(desc->if_type, desc->devnum, &dev); + ret = blk_find_device(desc->if_type, desc->devnum, &dev); - if (ret) - dev = desc->bdev->parent; - dpsize = dp_size(dev); - } -#else - dpsize = sizeof(ROOT) + sizeof(struct efi_device_path_usb); -#endif + if (ret) + dev = desc->bdev->parent; + dpsize = dp_size(dev); if (part == 0) /* the actual disk, not a partition */ return dpsize; @@ -877,36 +866,14 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part) */ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part) { -#ifdef CONFIG_BLK - { - struct udevice *dev; - int ret = blk_find_device(desc->if_type, desc->devnum, &dev); + struct udevice *dev; + int ret; - if (ret) - dev = desc->bdev->parent; - buf = dp_fill(buf, dev); - } -#else - /* - * We *could* make a more accurate path, by looking at if_type - * and handling all the different cases like we do for non- - * legacy (i.e. CONFIG_BLK=y) case. But most important thing - * is just to have a unique device-path for if_type+devnum. - * So map things to a fictitious USB device. - */ - struct efi_device_path_usb *udp; - - memcpy(buf, &ROOT, sizeof(ROOT)); - buf += sizeof(ROOT); - - udp = buf; - udp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; - udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB; - udp->dp.length = sizeof(*udp); - udp->parent_port_number = desc->if_type; - udp->usb_interface = desc->devnum; - buf = &udp[1]; -#endif + ret = blk_find_device(desc->if_type, desc->devnum, &dev); + + if (ret) + dev = desc->bdev->parent; + buf = dp_fill(buf, dev); if (part == 0) /* the actual disk, not a partition */ return buf; @@ -1051,39 +1018,18 @@ struct efi_device_path *efi_dp_from_uart(void) #ifdef CONFIG_NET struct efi_device_path *efi_dp_from_eth(void) { -#ifndef CONFIG_DM_ETH - struct efi_device_path_mac_addr *ndp; -#endif void *buf, *start; unsigned dpsize = 0; assert(eth_get_dev()); -#ifdef CONFIG_DM_ETH dpsize += dp_size(eth_get_dev()); -#else - dpsize += sizeof(ROOT); - dpsize += sizeof(*ndp); -#endif start = buf = dp_alloc(dpsize + sizeof(END)); if (!buf) return NULL; -#ifdef CONFIG_DM_ETH buf = dp_fill(buf, eth_get_dev()); -#else - memcpy(buf, &ROOT, sizeof(ROOT)); - buf += sizeof(ROOT); - - ndp = buf; - ndp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; - ndp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR; - ndp->dp.length = sizeof(*ndp); - ndp->if_type = 1; /* Ethernet */ - memcpy(ndp->mac.addr, eth_get_ethaddr(), ARP_HLEN); - buf = &ndp[1]; -#endif *((struct efi_device_path *)buf) = END; diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 988907ecb91..ef8b5c88ff9 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -555,7 +555,6 @@ efi_status_t efi_disk_register(void) struct efi_disk_obj *disk; int disks = 0; efi_status_t ret; -#ifdef CONFIG_BLK struct udevice *dev; for (uclass_first_device_check(UCLASS_BLK, &dev); dev; @@ -583,54 +582,7 @@ efi_status_t efi_disk_register(void) &disk->header, desc, if_typename, desc->devnum, dev->name); } -#else - int i, if_type; - /* Search for all available disk devices */ - for (if_type = 0; if_type < IF_TYPE_COUNT; if_type++) { - const struct blk_driver *cur_drvr; - const char *if_typename; - - cur_drvr = blk_driver_lookup_type(if_type); - if (!cur_drvr) - continue; - - if_typename = cur_drvr->if_typename; - log_info("Scanning disks on %s...\n", if_typename); - for (i = 0; i < 4; i++) { - struct blk_desc *desc; - char devname[32] = { 0 }; /* dp->str is u16[32] long */ - - desc = blk_get_devnum_by_type(if_type, i); - if (!desc) - continue; - if (desc->type == DEV_TYPE_UNKNOWN) - continue; - - snprintf(devname, sizeof(devname), "%s%d", - if_typename, i); - - /* Add block device for the full device */ - ret = efi_disk_add_dev(NULL, NULL, if_typename, desc, - i, NULL, 0, &disk); - if (ret == EFI_NOT_READY) { - log_notice("Disk %s not ready\n", devname); - continue; - } - if (ret) { - log_err("ERROR: failure to add disk device %s, r = %lu\n", - devname, ret & ~EFI_ERROR_MASK); - return ret; - } - disks++; - - /* Partitions show up as block devices in EFI */ - disks += efi_disk_create_partitions - (&disk->header, desc, - if_typename, i, devname); - } - } -#endif log_info("Found %d disks\n", disks); return EFI_SUCCESS;
This code should never have been added as it builds a new feature on top of legacy code. Drop it and add a dependency on BLK for this feature. Boards which want EFI_LOADER should migrate to driver model first. Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/efi_driver/Makefile | 2 +- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_device_path.c | 96 +++++++------------------------- lib/efi_loader/efi_disk.c | 48 ---------------- 4 files changed, 24 insertions(+), 124 deletions(-)