On Thu, Oct 03, 2019 at 08:48:59AM +0200, Heinrich Schuchardt wrote:

On 10/2/19 11:11 PM, Patrick Wildt wrote:

...

This adds a device path node for NVMe block devices. For that nvme_get_namespace_id() is added to return the privately stored namespace identifier.

Thanks a lot for looking into this.

The structures and constants that you define are correct. I would prefer if we could use the real value for the EUI instead of a dummy value of 0.

Both NVMe namespaces and EUIs can be discovered by issuing the 'Identify' command to the NVMe controller. See U-Boot function nvme_identify().

All NVMe revisions support 'Controller or Namespace Structure (CNS)' value 0x00. With this value you will retrieve the 'Identity Namespace' data structure. This call is already done in nvme_blk_probe().

Looking at the code I guess that you just have to add the EUI64 field to the NVMe private data (struct nvme_dev) and copy it from id->eui64 in nvme_blk_probe().

It would be great if you could also provide a patch adding the NVMe node to the device path to text protocol.

Unfortunately the patch was not addressed to me. You can use scripts/get_maintainer.pl to identify maintainers.

Best regards

Heinrich

Hi Heinrich,

that maintainer script is really useful, thank you!

I will follow up with a v2 that addresses your points in a second.

Best regards, Patrick

...

Signed-off-by: Patrick Wildt patrick@blueri.se

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 561f757772..0a72fe2b75 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -627,6 +627,13 @@ static int nvme_get_info_from_identify(struct nvme_dev *dev) return 0; }

+u32 +nvme_get_namespace_id(struct udevice *udev) +{

• struct nvme_ns *ns = dev_get_priv(udev);
• return ns->ns_id;

+}

• int nvme_scan_namespace(void) { struct uclass *uc;

diff --git a/include/efi_api.h b/include/efi_api.h index 37e56da460..0000b4ab1a 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

@@ -459,6 +460,12 @@ struct efi_device_path_usb_class { u8 device_protocol; } __packed;

+struct efi_device_path_nvme {

• struct efi_device_path dp;
• u32 nsid;
• u64 eui64;

+} __packed;

• struct efi_device_path_sd_mmc_path { struct efi_device_path dp; u8 slot_number;

diff --git a/include/nvme.h b/include/nvme.h index 2c3d14d241..95193c0334 100644 --- a/include/nvme.h +++ b/include/nvme.h @@ -78,4 +78,14 @@ int nvme_scan_namespace(void); */ int nvme_print_info(struct udevice *udev);

+/**

• • nvme_get_namespace_id - return namespace identifier
• • This returns the namespace identifier.
• • @udev: NVMe controller device
• • @return: namespace identifier
• */

+u32 nvme_get_namespace_id(struct udevice *udev);

• #endif /* __NVME_H__ */

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 86297bb7c1..89ad80c7bd 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -10,6 +10,7 @@ #include <dm.h> #include <usb.h> #include <mmc.h> +#include <nvme.h> #include <efi_loader.h> #include <part.h> #include <sandboxblockdev.h> @@ -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,19 @@ 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);
  

• 	dp->nsid        = nvme_get_namespace_id(dev);
  

• 	dp->eui64       = 0;
  

• 	return &dp[1];
  

• 	}
  

  #endif default: debug("%s(%u) %s: unhandled parent class: %s (%u)\n",

Hi Patrick,

On Thu, Oct 3, 2019 at 5:21 PM Patrick Wildt patrick@blueri.se wrote:

[PATCH v2] efi: device path for nvme

You don't need put the [PATCH v2] in the commit title.

This adds a device path node for NVMe block devices. For that nvme_get_namespace_id() is added to return the privately stored namespace identifier and optionally also the EUI64.

Signed-off-by: Patrick Wildt patrick@blueri.se

Changes for v2:

• the EUI64 is now also stored in the nvme device structure
• nvme_get_namespace_id() optionally returns the EUI64 as well
• EUI64 was changed from u64 to u8[8] for better handling
• added support for the device path to text protocol

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 47f101e280..b16ddc371a 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -621,6 +621,15 @@ static int nvme_get_info_from_identify(struct nvme_dev *dev) return 0; }

+u32

This should be put in the same line as the function name

+nvme_get_namespace_id(struct udevice *udev, u8 *eui64) +{

•   struct nvme_ns *ns = dev_get_priv(udev);
  

•   if (eui64)
  

•           memcpy(eui64, ns->eui64, sizeof(ns->eui64));
  

•   return ns->ns_id;
  

+}

int nvme_scan_namespace(void) { struct uclass *uc; @@ -657,6 +666,7 @@ static int nvme_blk_probe(struct udevice *udev) if (nvme_identify(ndev, ns->ns_id, 0, (dma_addr_t)(long)id)) return -EIO;

•   memcpy(&ns->eui64, &id->eui64, sizeof(id->eui64));
    flbas = id->flbas & NVME_NS_FLBAS_LBA_MASK;
    ns->flbas = flbas;
    ns->lba_shift = id->lbaf[flbas].ds;
  

diff --git a/drivers/nvme/nvme.h b/drivers/nvme/nvme.h index 922f7abfe8..0e8cb221a7 100644 --- a/drivers/nvme/nvme.h +++ b/drivers/nvme/nvme.h @@ -637,6 +637,7 @@ struct nvme_ns { struct list_head list; struct nvme_dev *dev; unsigned ns_id;

•   u8 eui64[8];
    int devnum;
    int lba_shift;
    u8 flbas;
  

Please split your patch into 2 patches, one for NVMe driver changes, and the other one for EFI changes.

diff --git a/include/efi_api.h b/include/efi_api.h index 37e56da460..d2c2330f84 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

@@ -459,6 +460,12 @@ struct efi_device_path_usb_class { u8 device_protocol; } __packed;

+struct efi_device_path_nvme {

•   struct efi_device_path dp;
  

•   u32 nsid;
  

•   u8 eui64[8];
  

+} __packed;

struct efi_device_path_sd_mmc_path { struct efi_device_path dp; u8 slot_number; diff --git a/include/nvme.h b/include/nvme.h index 2c3d14d241..c8fdd44da0 100644 --- a/include/nvme.h +++ b/include/nvme.h @@ -78,4 +78,15 @@ int nvme_scan_namespace(void); */ int nvme_print_info(struct udevice *udev);

+/**

• • nvme_get_namespace_id - return namespace identifier
• • This returns the namespace identifier.
• • @udev: NVMe controller device
• • @eui64: IEEE Extended Unique Identifier
• • @return: namespace identifier
• */

+u32 nvme_get_namespace_id(struct udevice *udev, u8 *eui64);

#endif /* __NVME_H__ */ diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 86297bb7c1..6cb5b9c0d4 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -10,6 +10,7 @@ #include <dm.h> #include <usb.h> #include <mmc.h> +#include <nvme.h> #include <efi_loader.h> #include <part.h> #include <sandboxblockdev.h> @@ -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,19 @@ 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);
  

•                   dp->nsid        = nvme_get_namespace_id(dev,
  

•                                                           &dp->eui64[0]);
  

•                   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..9cfb7faa25 100644 --- a/lib/efi_loader/efi_device_path_to_text.c +++ b/lib/efi_loader/efi_device_path_to_text.c @@ -148,6 +148,20 @@ 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->nsid);
  

•           for (i = 0; i < sizeof(ndp->eui64); ++i)
  

•                   s += sprintf(s, "%s%02x", i ? "-" : "",
  

•                                ndp->eui64[i]);
  

•           s += sprintf(s, ")");
  

•           break;
  

•   }
    case DEVICE_PATH_SUB_TYPE_MSG_SD:
    case DEVICE_PATH_SUB_TYPE_MSG_MMC: {
            const char *typename =
  

Regards, Bin

On Thu, Oct 3, 2019 at 5:56 PM Patrick Wildt patrick@blueri.se wrote:

This adds a function which can be used by e.g. EFI to retrieve the namespace identifier and EUI64. For that it adds the EUI64 to its driver internal namespace structure and copies the EUI64 during namespace identification.

Signed-off-by: Patrick Wildt patrick@blueri.se

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 47f101e280..e9994ae5d4 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -621,6 +621,14 @@ static int nvme_get_info_from_identify(struct nvme_dev *dev) return 0; }

+u32 nvme_get_namespace_id(struct udevice *udev, u8 *eui64) +{

•   struct nvme_ns *ns = dev_get_priv(udev);
  

•   if (eui64)
  

•           memcpy(eui64, ns->eui64, sizeof(ns->eui64));
  

•   return ns->ns_id;
  

+}

int nvme_scan_namespace(void) { struct uclass *uc; @@ -657,6 +665,7 @@ static int nvme_blk_probe(struct udevice *udev) if (nvme_identify(ndev, ns->ns_id, 0, (dma_addr_t)(long)id)) return -EIO;

•   memcpy(&ns->eui64, &id->eui64, sizeof(id->eui64));
    flbas = id->flbas & NVME_NS_FLBAS_LBA_MASK;
    ns->flbas = flbas;
    ns->lba_shift = id->lbaf[flbas].ds;
  

diff --git a/drivers/nvme/nvme.h b/drivers/nvme/nvme.h index 922f7abfe8..0e8cb221a7 100644 --- a/drivers/nvme/nvme.h +++ b/drivers/nvme/nvme.h @@ -637,6 +637,7 @@ struct nvme_ns { struct list_head list; struct nvme_dev *dev; unsigned ns_id;

•   u8 eui64[8];
    int devnum;
    int lba_shift;
    u8 flbas;
  

diff --git a/include/nvme.h b/include/nvme.h index 2c3d14d241..c8fdd44da0 100644 --- a/include/nvme.h +++ b/include/nvme.h @@ -78,4 +78,15 @@ int nvme_scan_namespace(void); */ int nvme_print_info(struct udevice *udev);

+/**

• • nvme_get_namespace_id - return namespace identifier
• • This returns the namespace identifier.
• • @udev: NVMe controller device
• • @eui64: IEEE Extended Unique Identifier

It's actually an output parameter. Please add more descriptions to indicate.

• • @return: namespace identifier

Perhaps we should make ns_id an output parameter too, and make the return value be either 0 on success or -ve on error.

• */

+u32 nvme_get_namespace_id(struct udevice *udev, u8 *eui64);

#endif /* __NVME_H__ */

BTW: you can send this patch along with the EFI patch as a series.

Regards, Bin

Hi Patrick,

On Thu, Oct 3, 2019 at 6:33 PM Patrick Wildt patrick@blueri.se wrote:

On Thu, Oct 03, 2019 at 06:25:53PM +0800, Bin Meng wrote:

...

On Thu, Oct 3, 2019 at 5:56 PM Patrick Wildt patrick@blueri.se wrote:

...

This adds a function which can be used by e.g. EFI to retrieve the namespace identifier and EUI64. For that it adds the EUI64 to its driver internal namespace structure and copies the EUI64 during namespace identification.

Signed-off-by: Patrick Wildt patrick@blueri.se

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 47f101e280..e9994ae5d4 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -621,6 +621,14 @@ static int nvme_get_info_from_identify(struct nvme_dev *dev) return 0; }

+u32 nvme_get_namespace_id(struct udevice *udev, u8 *eui64) +{

•   struct nvme_ns *ns = dev_get_priv(udev);
  

•   if (eui64)
  

•           memcpy(eui64, ns->eui64, sizeof(ns->eui64));
  

•   return ns->ns_id;
  

+}

int nvme_scan_namespace(void) { struct uclass *uc; @@ -657,6 +665,7 @@ static int nvme_blk_probe(struct udevice *udev) if (nvme_identify(ndev, ns->ns_id, 0, (dma_addr_t)(long)id)) return -EIO;

•   memcpy(&ns->eui64, &id->eui64, sizeof(id->eui64));
    flbas = id->flbas & NVME_NS_FLBAS_LBA_MASK;
    ns->flbas = flbas;
    ns->lba_shift = id->lbaf[flbas].ds;
  

diff --git a/drivers/nvme/nvme.h b/drivers/nvme/nvme.h index 922f7abfe8..0e8cb221a7 100644 --- a/drivers/nvme/nvme.h +++ b/drivers/nvme/nvme.h @@ -637,6 +637,7 @@ struct nvme_ns { struct list_head list; struct nvme_dev *dev; unsigned ns_id;

•   u8 eui64[8];
    int devnum;
    int lba_shift;
    u8 flbas;
  

diff --git a/include/nvme.h b/include/nvme.h index 2c3d14d241..c8fdd44da0 100644 --- a/include/nvme.h +++ b/include/nvme.h @@ -78,4 +78,15 @@ int nvme_scan_namespace(void); */ int nvme_print_info(struct udevice *udev);

+/**

• • nvme_get_namespace_id - return namespace identifier
• • This returns the namespace identifier.
• • @udev: NVMe controller device
• • @eui64: IEEE Extended Unique Identifier

It's actually an output parameter. Please add more descriptions to indicate.

...

• • @return: namespace identifier

Perhaps we should make ns_id an output parameter too, and make the return value be either 0 on success or -ve on error.

In which case should the function return an error? Or should I specify that it can return an error, but let the function always return 0?

I think making it always return 0 is OK for now.

Regards, Bin

On Thu, Oct 3, 2019 at 7:01 PM Patrick Wildt patrick@blueri.se wrote:

This adds a function which can be used by e.g. EFI to retrieve the namespace identifier and EUI64. For that it adds the EUI64 to its driver internal namespace structure and copies the EUI64 during namespace identification.

Signed-off-by: Patrick Wildt patrick@blueri.se

drivers/nvme/nvme.c | 13 +++++++++++++ drivers/nvme/nvme.h | 1 + include/nvme.h | 12 ++++++++++++ 3 files changed, 26 insertions(+)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 47f101e280..99e859d1e9 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -621,6 +621,18 @@ static int nvme_get_info_from_identify(struct nvme_dev *dev) return 0; }

+u32 nvme_get_namespace_id(struct udevice *udev, u32 *ns_id, u8 *eui64)

This does not match the declaration. The return value should be int.

+{

•   struct nvme_ns *ns = dev_get_priv(udev);
  

•   if (ns_id)
  

•           *ns_id = ns->ns_id;
  

•   if (eui64)
  

•           memcpy(eui64, ns->eui64, sizeof(ns->eui64));
  

•   return 0;
  

+}

int nvme_scan_namespace(void) { struct uclass *uc; @@ -657,6 +669,7 @@ static int nvme_blk_probe(struct udevice *udev) if (nvme_identify(ndev, ns->ns_id, 0, (dma_addr_t)(long)id)) return -EIO;

•   memcpy(&ns->eui64, &id->eui64, sizeof(id->eui64));
    flbas = id->flbas & NVME_NS_FLBAS_LBA_MASK;
    ns->flbas = flbas;
    ns->lba_shift = id->lbaf[flbas].ds;
  

diff --git a/drivers/nvme/nvme.h b/drivers/nvme/nvme.h index 922f7abfe8..0e8cb221a7 100644 --- a/drivers/nvme/nvme.h +++ b/drivers/nvme/nvme.h @@ -637,6 +637,7 @@ struct nvme_ns { struct list_head list; struct nvme_dev *dev; unsigned ns_id;

•   u8 eui64[8];
    int devnum;
    int lba_shift;
    u8 flbas;
  

diff --git a/include/nvme.h b/include/nvme.h index 2c3d14d241..2cdf8ce320 100644 --- a/include/nvme.h +++ b/include/nvme.h @@ -78,4 +78,16 @@ int nvme_scan_namespace(void); */ int nvme_print_info(struct udevice *udev);

+/**

• • nvme_get_namespace_id - return namespace identifier
• • This returns the namespace identifier.
• • @udev: NVMe controller device
• • @ns_id: Place where to put the name space identifier
• • @eui64: Place where to put the IEEE Extended Unique Identifier
• • @return: 0 on success, -ve on error
• */

+int nvme_get_namespace_id(struct udevice *udev, u32 *ns_id, u8 *eui64);

#endif /* __NVME_H__ */

Other than that, Reviewed-by: Bin Meng bmeng.cn@gmail.com

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 patrick@blueri.se --- 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 <dm.h> #include <usb.h> #include <mmc.h> +#include <nvme.h> #include <efi_loader.h> #include <part.h> #include <sandboxblockdev.h> @@ -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]); + 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); + for (i = 0; i < sizeof(ndp->eui64); ++i) + s += sprintf(s, "%s%02x", i ? "-" : "", + ndp->eui64[i]); + s += sprintf(s, ")"); + + break; + } case DEVICE_PATH_SUB_TYPE_MSG_SD: case DEVICE_PATH_SUB_TYPE_MSG_MMC: { const char *typename =

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 patrick@blueri.se

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 <dm.h> #include <usb.h> #include <mmc.h> +#include <nvme.h> #include <efi_loader.h> #include <part.h> #include <sandboxblockdev.h> @@ -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 =

On Thu, Oct 3, 2019 at 5:57 PM Patrick Wildt patrick@blueri.se 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 patrick@blueri.se

diff --git a/include/efi_api.h b/include/efi_api.h index 37e56da460..d2c2330f84 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

@@ -459,6 +460,12 @@ struct efi_device_path_usb_class { u8 device_protocol; } __packed;

+struct efi_device_path_nvme {

•   struct efi_device_path dp;
  

•   u32 nsid;
  

nits: ns_id? for consistency with what we have in the nvme driver

•   u8 eui64[8];
  

+} __packed;

nits: this struct should be put after struct efi_device_path_sd_mmc_path

struct efi_device_path_sd_mmc_path { struct efi_device_path dp; u8 slot_number; diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 86297bb7c1..6cb5b9c0d4 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -10,6 +10,7 @@ #include <dm.h> #include <usb.h> #include <mmc.h> +#include <nvme.h> #include <efi_loader.h> #include <part.h> #include <sandboxblockdev.h> @@ -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,19 @@ 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);
  

•                   dp->nsid        = nvme_get_namespace_id(dev,
  

•                                                           &dp->eui64[0]);
  

•                   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..9cfb7faa25 100644 --- a/lib/efi_loader/efi_device_path_to_text.c +++ b/lib/efi_loader/efi_device_path_to_text.c @@ -148,6 +148,20 @@ 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->nsid);
  

•           for (i = 0; i < sizeof(ndp->eui64); ++i)
  

•                   s += sprintf(s, "%s%02x", i ? "-" : "",
  

•                                ndp->eui64[i]);
  

•           s += sprintf(s, ")");
  

•           break;
  

•   }
    case DEVICE_PATH_SUB_TYPE_MSG_SD:
    case DEVICE_PATH_SUB_TYPE_MSG_MMC: {
            const char *typename =
  

Regards, Bin