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