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 4CC25C00528 for ; Fri, 28 Jul 2023 09:14:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4190D86898; Fri, 28 Jul 2023 11:14:37 +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="qeUbCRRz"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 275498689A; Fri, 28 Jul 2023 11:14:35 +0200 (CEST) Received: from mail-wr1-x434.google.com (mail-wr1-x434.google.com [IPv6:2a00:1450:4864:20::434]) (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 F00A586895 for ; Fri, 28 Jul 2023 11:14:31 +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=ilias.apalodimas@linaro.org Received: by mail-wr1-x434.google.com with SMTP id ffacd0b85a97d-3090d3e9c92so1942905f8f.2 for ; Fri, 28 Jul 2023 02:14:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1690535671; x=1691140471; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ozYPMMx8HQy57TOPllf0Nbl+l5Ul8aIu3pYnE4M+6/o=; b=qeUbCRRzOLEZbrzRNitRtpGaZYTJ3l6ktWt79FOZDbqtNnlxlRpA67UQljnQ7wjUUZ qS4RHcuDWl3pSllwQdUolt+b8PNsF4yLAoFeEp3pYbQjUZWEfSAyuzZ8k/qP3/KtFDd3 lwIdoKl9dPCEv0qs2dcndJxVv2hNcikSOGT8kERpoic0lIffxgrGAqbw6oNRUZAGgeHB 4VC/IhG68pkxPZDxGfdAMyDU+F2DTmMhsxxSk68A0F6XOTMaFFIolwmeQqfqkmLePcHO i/Yud3GDW4LVvGVCjfHnQNv9mOV/n0m9W/b9thl2+iDB7h76OsAFp1Mv/9NXFDyEGlRN nr6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690535671; x=1691140471; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ozYPMMx8HQy57TOPllf0Nbl+l5Ul8aIu3pYnE4M+6/o=; b=g8nmiCxLGwnh424CA6Q0x+GifgErOz/f1X3CXG1CWznUeALA9IcToSgIKXaELAhfMf Kbm3yQ23AVlwo4c64KbtmOa1t+jNz1s+6luu3SVx3Fr2M/vH70USxyDnwvd25i4+ZzVI g5CQ083dHPGTlAv8np3EMSSRp3STpv/rWJ4O/WeskM0xDOmK1D60dDJh9M4Cktsa8wLa 4ZEYMh8PM6IDBFSQqsH04GHByIqvtJ2/20TvnmU0DvOCfWDJK2HsPb7pwt+lf1ub2JeG ovAEtEWamB5INEq7+wTihwo05Rlb8TBM+AdcfI2ygT3jCBgETQkbUGkjh+kgwuZ80SyX pLPg== X-Gm-Message-State: ABy/qLZ1taIYRkPkw5uZ2z5BaJEuXehNYMZQCHeLbYhlQ9WziR8pnFLR vqUFrc1M10pI9klAv5Q/0RMMuQiBpwC1KM7wCZA= X-Google-Smtp-Source: APBJJlEQLzCLbTqMC1GOGBpqj4Im3oB8u0ITGE9h9glJxc83PhYRXGQN73SlUHcLpXNwSFoP5qiEuw== X-Received: by 2002:a05:6000:120a:b0:317:54e2:26ca with SMTP id e10-20020a056000120a00b0031754e226camr1570105wrx.50.1690535671074; Fri, 28 Jul 2023 02:14:31 -0700 (PDT) Received: from hades (ppp089210088142.access.hol.gr. [89.210.88.142]) by smtp.gmail.com with ESMTPSA id x1-20020a5d54c1000000b003176f2d9ce5sm4272335wrv.71.2023.07.28.02.14.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Jul 2023 02:14:30 -0700 (PDT) Date: Fri, 28 Jul 2023 12:14:28 +0300 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: u-boot@lists.denx.de Subject: Re: [PATCH 1/1] efi_loader: device paths for special block devices Message-ID: References: <20230720220346.48642-1-heinrich.schuchardt@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230720220346.48642-1-heinrich.schuchardt@canonical.com> 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 Hi Heinrich, On Fri, Jul 21, 2023 at 12:03:46AM +0200, Heinrich Schuchardt wrote: > The UEFI specification does not provide node types matching UCLASS_BLKMAP, > UCLASS_HOST, UCLASS_VIRTIO block devices. > > The current implementation uses VenHw() nodes with uclass specific GUIDs > and a single byte for the device number appended. This leads to unaligned > integers is succeeding device path nodes. in succeeding > > The current implementation fails to create unique device paths for block > devices based on other uclasses like UCLASS_PVBLOCK. Is this fixed as well with this patch because we have a sane default switch now? > > Let's use a VenHw() node with the U-Boot GUID with a length dividable by > four and encoding blkdesc->uclass_id as well as blkdesc->devnum. > > Signed-off-by: Heinrich Schuchardt > --- > lib/efi_loader/efi_device_path.c | 114 ++++++------------------------- > 1 file changed, 21 insertions(+), 93 deletions(-) > > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c > index 19e8861ef4..5b050fa17c 100644 > --- a/lib/efi_loader/efi_device_path.c > +++ b/lib/efi_loader/efi_device_path.c > @@ -22,16 +22,6 @@ > #include > #include /* U16_MAX */ > > -#ifdef CONFIG_BLKMAP > -const efi_guid_t efi_guid_blkmap_dev = U_BOOT_BLKMAP_DEV_GUID; > -#endif > -#ifdef CONFIG_SANDBOX > -const efi_guid_t efi_guid_host_dev = U_BOOT_HOST_DEV_GUID; > -#endif > -#ifdef CONFIG_VIRTIO_BLK > -const efi_guid_t efi_guid_virtio_dev = U_BOOT_VIRTIO_DEV_GUID; > -#endif > - > /* template END node: */ > const struct efi_device_path END = { > .type = DEVICE_PATH_TYPE_END, > @@ -524,43 +514,15 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) > return dp_size(dev->parent) + > sizeof(struct efi_device_path_nvme); > #endif > -#ifdef CONFIG_SANDBOX > - case UCLASS_HOST: > - /* > - * Sandbox's host device will be represented > - * as vendor device with extra one byte for > - * device number > - */ > - return dp_size(dev->parent) > - + sizeof(struct efi_device_path_vendor) + 1; > -#endif > #ifdef CONFIG_USB > case UCLASS_MASS_STORAGE: > return dp_size(dev->parent) > + sizeof(struct efi_device_path_controller); > -#endif > -#ifdef CONFIG_VIRTIO_BLK > - case UCLASS_VIRTIO: > - /* > - * Virtio devices will be represented as a vendor > - * device node with an extra byte for the device > - * number. > - */ > - return dp_size(dev->parent) > - + sizeof(struct efi_device_path_vendor) + 1; > -#endif > -#ifdef CONFIG_BLKMAP > - case UCLASS_BLKMAP: > - /* > - * blkmap devices will be represented as a vendor > - * device node with an extra byte for the device > - * number. > - */ > - return dp_size(dev->parent) > - + sizeof(struct efi_device_path_vendor) + 1; > #endif > default: > - return dp_size(dev->parent); > + /* UCLASS_BLKMAP, UCLASS_HOST, UCLASS_VIRTIO */ > + return dp_size(dev->parent) + > + sizeof(struct efi_device_path_udevice); We are handling these for now , but this ends up being a catch all for all UCLASS devices that arent explicitly handled but I guess this is ok? > } > #if defined(CONFIG_MMC) > case UCLASS_MMC: > @@ -608,53 +570,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > } > #endif > case UCLASS_BLK: > - switch (dev->parent->uclass->uc_drv->id) { > -#ifdef CONFIG_BLKMAP > - case UCLASS_BLKMAP: { > - struct efi_device_path_vendor *dp; > - struct blk_desc *desc = dev_get_uclass_plat(dev); > - > - dp = dp_fill(buf, dev->parent); > - dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; > - dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR; > - dp->dp.length = sizeof(*dp) + 1; > - memcpy(&dp->guid, &efi_guid_blkmap_dev, > - sizeof(efi_guid_t)); > - dp->vendor_data[0] = desc->devnum; > - return &dp->vendor_data[1]; > - } > -#endif > -#ifdef CONFIG_SANDBOX > - case UCLASS_HOST: { > - /* stop traversing parents at this point: */ > - struct efi_device_path_vendor *dp; > - struct blk_desc *desc = dev_get_uclass_plat(dev); > - > - dp = dp_fill(buf, dev->parent); > - dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; > - dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR; > - dp->dp.length = sizeof(*dp) + 1; > - memcpy(&dp->guid, &efi_guid_host_dev, > - sizeof(efi_guid_t)); > - dp->vendor_data[0] = desc->devnum; > - return &dp->vendor_data[1]; > - } > -#endif > -#ifdef CONFIG_VIRTIO_BLK > - case UCLASS_VIRTIO: { > - struct efi_device_path_vendor *dp; > - struct blk_desc *desc = dev_get_uclass_plat(dev); > - > - dp = dp_fill(buf, dev->parent); > - dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; > - dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR; > - dp->dp.length = sizeof(*dp) + 1; > - memcpy(&dp->guid, &efi_guid_virtio_dev, > - sizeof(efi_guid_t)); > - dp->vendor_data[0] = desc->devnum; > - return &dp->vendor_data[1]; > - } > -#endif > + switch (device_get_uclass_id(dev->parent)) { > #ifdef CONFIG_IDE > case UCLASS_IDE: { > struct efi_device_path_atapi *dp = > @@ -744,12 +660,24 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) > return &dp[1]; > } > #endif > - default: > - debug("%s(%u) %s: unhandled parent class: %s (%u)\n", > - __FILE__, __LINE__, __func__, > - dev->name, dev->parent->uclass->uc_drv->id); > - return dp_fill(buf, dev->parent); > + default: { > + /* UCLASS_BLKMAP, UCLASS_HOST, UCLASS_VIRTIO */ > + struct efi_device_path_udevice *dp; > + struct blk_desc *desc = dev_get_uclass_plat(dev); > + > + dp = dp_fill(buf, dev->parent); > + dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; > + dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR; > + dp->dp.length = sizeof(*dp); > + memcpy(&dp->guid, &efi_u_boot_guid, > + sizeof(efi_guid_t)); > + dp->uclass_id = (UCLASS_BLK & 0xffff) | > + (desc->uclass_id << 16); > + dp->dev_number = desc->devnum; > + > + return &dp[1]; > } > + } > #if defined(CONFIG_MMC) > case UCLASS_MMC: { > struct efi_device_path_sd_mmc_path *sddp = > -- > 2.40.1 > In any case the patch LGTM Reviewed-by: Ilias Apalodimas