From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick Wildt Date: Thu, 3 Oct 2019 15:32:44 +0200 Subject: [U-Boot] [PATCH 2/2] efi: device path for nvme In-Reply-To: <2e625188-b53e-cf19-e25e-9b6cdc7122e7@gmx.de> References: <20191002211131.GA44383@nox.fritz.box> <5756478a-a091-be62-1f84-1b5efa59f9fd@gmx.de> <20191003092127.GD47163@nox.fritz.box> <20191003095617.GA48482@nox.fritz.box> <20191003103336.GA49768@nox.fritz.box> <20191003110216.GC49768@nox.fritz.box> <2e625188-b53e-cf19-e25e-9b6cdc7122e7@gmx.de> Message-ID: <20191003133244.GA50411@nox.fritz.box> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Thu, Oct 03, 2019 at 03:12:45PM +0200, Heinrich Schuchardt wrote: > On 10/3/19 1:02 PM, Patrick Wildt wrote: > > This allows our EFI API to create a device path node for NVMe > > devices. It adds the necessary device path struct, uses the > > nvme namespace accessor to retrieve the id and eui64, and also > > provides support for the device path text protocol. > > > > Signed-off-by: Patrick Wildt > > --- > > include/efi_api.h | 7 +++++++ > > lib/efi_loader/efi_device_path.c | 18 ++++++++++++++++++ > > lib/efi_loader/efi_device_path_to_text.c | 13 +++++++++++++ > > 3 files changed, 38 insertions(+) > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > index 37e56da460..22396172e1 100644 > > --- a/include/efi_api.h > > +++ b/include/efi_api.h > > @@ -422,6 +422,7 @@ struct efi_device_path_acpi_path { > > # define DEVICE_PATH_SUB_TYPE_MSG_USB 0x05 > > # define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR 0x0b > > # define DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS 0x0f > > +# define DEVICE_PATH_SUB_TYPE_MSG_NVME 0x17 > > # define DEVICE_PATH_SUB_TYPE_MSG_SD 0x1a > > # define DEVICE_PATH_SUB_TYPE_MSG_MMC 0x1d > > > > @@ -464,6 +465,12 @@ struct efi_device_path_sd_mmc_path { > > u8 slot_number; > > } __packed; > > > > +struct efi_device_path_nvme { > > + struct efi_device_path dp; > > + u32 ns_id; > > + u8 eui64[8]; > > +} __packed; > > + > > #define DEVICE_PATH_TYPE_MEDIA_DEVICE 0x04 > > # define DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH 0x01 > > # define DEVICE_PATH_SUB_TYPE_CDROM_PATH 0x02 > > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c > > index 86297bb7c1..6a18add269 100644 > > --- a/lib/efi_loader/efi_device_path.c > > +++ b/lib/efi_loader/efi_device_path.c > > @@ -10,6 +10,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -451,6 +452,11 @@ static unsigned dp_size(struct udevice *dev) > > return dp_size(dev->parent) + > > sizeof(struct efi_device_path_sd_mmc_path); > > #endif > > +#if defined(CONFIG_NVME) > > + case UCLASS_NVME: > > + return dp_size(dev->parent) + > > + sizeof(struct efi_device_path_nvme); > > +#endif > > #ifdef CONFIG_SANDBOX > > case UCLASS_ROOT: > > /* > > @@ -583,6 +589,18 @@ static void *dp_fill(void *buf, struct udevice *dev) > > sddp->slot_number = dev->seq; > > return &sddp[1]; > > } > > +#endif > > +#if defined(CONFIG_NVME) > > + case UCLASS_NVME: { > > + struct efi_device_path_nvme *dp = > > + dp_fill(buf, dev->parent); > > + > > + dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > > + dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_NVME; > > + dp->dp.length = sizeof(*dp); > > + nvme_get_namespace_id(dev, &dp->ns_id, &dp->eui64[0]); > > Instead of &dp->eui64[0] you could simply write dp->eui64. > To me that is easier to read. > > Your code does not compile with GCC 9.2.1 (as available in Debian Bullseye): > > lib/efi_loader/efi_device_path.c: In function ‘dp_fill’: > lib/efi_loader/efi_device_path.c:601:31: error: taking address of packed > member of ‘struct efi_device_path_nvme’ may result in an unaligned > pointer value [-Werror=address-of-packed-member] > 601 | nvme_get_namespace_id(dev, &dp->ns_id, &dp->eui64[0]); > | ^~~~~~~~~~ Uh, that sounds bad. How can we fix/work around this? Maybe something like.. u32 ns_id; nvme_get_namespace_id(dev, &ns_id, &dp->eui64[0]); memcpy(&dp->ns_id, &ns_id, sizeof(ns_id)); ? Best regards, Patrick > > + return &dp[1]; > > + } > > #endif > > default: > > debug("%s(%u) %s: unhandled parent class: %s (%u)\n", > > diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c > > index 0f3796b373..e8a608bd07 100644 > > --- a/lib/efi_loader/efi_device_path_to_text.c > > +++ b/lib/efi_loader/efi_device_path_to_text.c > > @@ -148,6 +148,19 @@ static char *dp_msging(char *s, struct efi_device_path *dp) > > > > break; > > } > > + case DEVICE_PATH_SUB_TYPE_MSG_NVME: { > > + struct efi_device_path_nvme *ndp = > > + (struct efi_device_path_nvme *)dp; > > + int i; > > + > > + s += sprintf(s, "NVME(0x%x,", ndp->ns_id); > > In the spec this is "NVMe(", i.e. lower case 'e' at the end. > > > + for (i = 0; i < sizeof(ndp->eui64); ++i) > > + s += sprintf(s, "%s%02x", i ? "-" : "", > > The example in the spec uses capitalized hexadecimals in the example. > But EDK2 uses lower case. So that should be ok. > > Best regards > > Heinrich > > > + ndp->eui64[i]); > > + s += sprintf(s, ")"); > > + > > + break; > > + } > > case DEVICE_PATH_SUB_TYPE_MSG_SD: > > case DEVICE_PATH_SUB_TYPE_MSG_MMC: { > > const char *typename = > > >