[U-Boot] [PATCH] efi: device path for nvme

[U-Boot] [PATCH] efi: device path for nvme

[Patrick Wildt]

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.

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",

[U-Boot] [PATCH] efi: device path for nvme

[Bin Meng]

+Heinrich to review

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

Regards,
Bin

[U-Boot] [PATCH] efi: device path for nvme

[Heinrich Schuchardt]

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

>
> 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",
>

[U-Boot] [PATCH] efi: device path for nvme

[Patrick Wildt]

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",
> > 
> 

[U-Boot] [PATCH v2] efi: device path for nvme

[Patrick Wildt]

[PATCH v2] efi: device path for nvme

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
+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;
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 =

[U-Boot] [PATCH v2] efi: device path for nvme

[Bin Meng]

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

[U-Boot] [PATCH] nvme: add accessor to namespace id and eui64

[Patrick Wildt]

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
+ * @return:	namespace identifier
+ */
+u32 nvme_get_namespace_id(struct udevice *udev, u8 *eui64);
+
 #endif /* __NVME_H__ */

[U-Boot] [PATCH] efi: device path for nvme

[Patrick Wildt]

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;
+	u8 eui64[8];
+} __packed;
+
 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 =

[U-Boot] [PATCH] nvme: add accessor to namespace id and eui64

[Bin Meng]

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

[U-Boot] [PATCH] efi: device path for nvme

[Bin Meng]

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

[U-Boot] [PATCH] nvme: add accessor to namespace id and eui64

[Patrick Wildt]

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?

Best regards,
Patrick

> > + */
> > +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

[U-Boot] [PATCH] nvme: add accessor to namespace id and eui64

[Bin Meng]

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

[U-Boot] [PATCH 1/2] nvme: add accessor to namespace id and eui64

[Patrick Wildt]

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)
+{
+	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__ */
-- 
2.23.0

[U-Boot] [PATCH 2/2] efi: device path for nvme

[Patrick Wildt]

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 =
-- 
2.23.0

[U-Boot] [PATCH 1/2] nvme: add accessor to namespace id and eui64

[Bin Meng]

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>

[U-Boot] [PATCH v2 1/2] nvme: add accessor to namespace id and eui64

[Patrick Wildt]

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..ee6b581d9e 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;
 }
 
+int nvme_get_namespace_id(struct udevice *udev, u32 *ns_id, u8 *eui64)
+{
+	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__ */
-- 
2.23.0

[U-Boot] [PATCH v2 1/2] nvme: add accessor to namespace id and eui64

[Bin Meng]

On Thu, Oct 3, 2019 at 7:48 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(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

I see you only resent the v2 of the first patch. Normally all patches
in the same series should be resent that helps maintain the patch
inter-dependency.

Regards,
Bin

[U-Boot] [PATCH v2 2/2] efi: device path for nvme

[Patrick Wildt]

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 =
-- 
2.23.0

[U-Boot] [PATCH 2/2] efi: device path for nvme

[Heinrich Schuchardt]

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]);
       |                               ^~~~~~~~~~

> +			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 =
>

[U-Boot] [PATCH 2/2] efi: device path for nvme

[Patrick Wildt]

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 =
> > 
> 

[U-Boot] [PATCH v3 2/2] efi: device path for nvme

[Patrick Wildt]

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         | 20 ++++++++++++++++++++
 lib/efi_loader/efi_device_path_to_text.c | 15 +++++++++++++++
 3 files changed, 42 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..897fc1b2e8 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,20 @@ 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);
+			u32 ns_id;
+
+			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, &ns_id, dp->eui64);
+			memcpy(&dp->ns_id, &ns_id, sizeof(ns_id));
+			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..af1adbb71e 100644
--- a/lib/efi_loader/efi_device_path_to_text.c
+++ b/lib/efi_loader/efi_device_path_to_text.c
@@ -148,6 +148,21 @@ 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;
+		u32 ns_id;
+		int i;
+
+		memcpy(&ns_id, &ndp->ns_id, sizeof(ns_id));
+		s += sprintf(s, "NVMe(0x%x,", 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 =
-- 
2.23.0

[U-Boot] [PATCH v3 2/2] efi: device path for nvme

[Heinrich Schuchardt]

On 10/3/19 4:24 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>
> ---

I have tested with an NVMe drive mounted on a MACCHIATObin and saw
consistent device paths and GUIDs in U-Boot, UEFI Shell and Debian:

In U-Boot:

=> efidebug devices
Device           Device Path
================ ====================
000000007fb90380
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/NVMe(0x1,00-25-38-b5-81-e6-05-65)

In UEFI Shell (started from U-Boot):

     BLK1: Alias(s):

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/NVMe(0x1,00-25-38-b5-81-e6-05-65)

In Debian:

$ smartctl -a /dev/nvme0n1
Namespace 1 IEEE EUI-64:            002538 b581e60565

The first bytes match the brand of the drive:

http://standards-oui.ieee.org/oui.txt:
002538     Samsung Electronics Co., Ltd., Memory Division

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>