[U-Boot] [PATCH 0/3] efi_loader: correct media device paths

[U-Boot] [PATCH 0/3] efi_loader: correct media device paths

[Heinrich Schuchardt]

For each disk we need partition device path with partion number 0.
The device node texts should match the UEFI spec.

Heinrich Schuchardt (3):
  efi_loader: correctly determine if an MMC device is an SD-card
  efi_loader: correctly setup device paths for block devices
  efi_loader: correct DeviceNodeToText for media types

 lib/efi_loader/efi_device_path.c         | 35 +++++++++++++++++++++++++-------
 lib/efi_loader/efi_device_path_to_text.c | 30 +++++++++++++++++----------
 lib/efi_loader/efi_disk.c                |  7 +++++--
 3 files changed, 52 insertions(+), 20 deletions(-)

-- 
2.11.0

[U-Boot] [PATCH 1/3] efi_loader: correctly determine if an MMC device is an SD-card

[Heinrich Schuchardt]

The SD cards and eMMC devices have different device nodes.
The current coding interpretes all MMC devices as eMMC.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_device_path.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index b4e2f933cb..42fe6e1185 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -36,6 +36,24 @@ static const struct efi_device_path_vendor ROOT = {
 	.guid = U_BOOT_GUID,
 };
 
+#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC)
+/*
+ * Determine if an MMC device is an SD card.
+ *
+ * @desc	block device descriptor
+ * @return	true if the device is an SD card
+ */
+static bool is_sd(struct blk_desc *desc)
+{
+	struct mmc *mmc = find_mmc_device(desc->devnum);
+
+	if (!mmc)
+		return false;
+
+	return IS_SD(mmc) != 0U;
+}
+#endif
+
 static void *dp_alloc(size_t sz)
 {
 	void *buf;
@@ -298,9 +316,9 @@ static void *dp_fill(void *buf, struct udevice *dev)
 		struct blk_desc *desc = mmc_get_blk_desc(mmc);
 
 		sddp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-		sddp->dp.sub_type = (desc->if_type == IF_TYPE_MMC) ?
-			DEVICE_PATH_SUB_TYPE_MSG_MMC :
-			DEVICE_PATH_SUB_TYPE_MSG_SD;
+		sddp->dp.sub_type = is_sd(desc) ?
+			DEVICE_PATH_SUB_TYPE_MSG_SD :
+			DEVICE_PATH_SUB_TYPE_MSG_MMC;
 		sddp->dp.length   = sizeof(*sddp);
 		sddp->slot_number = dev->seq;
 
-- 
2.11.0

[U-Boot] [PATCH 2/3] efi_loader: correctly setup device paths for block devices

[Heinrich Schuchardt]

For each block device we have to setup:
- a device path for the block device
- a partition device path for the block device with
  partition number 0
- a partition device path for each partition

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_device_path.c | 11 +++++++----
 lib/efi_loader/efi_disk.c        |  7 +++++--
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 42fe6e1185..d0d62ff428 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -412,15 +412,18 @@ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part)
 	buf = &udp[1];
 #endif
 
-	if (part == 0) /* the actual disk, not a partition */
+	if (part == -1) /* the actual disk, not a partition */
 		return buf;
 
-	part_get_info(desc, part, &info);
+	if (part == 0)
+		part_get_info_whole_disk(desc, &info);
+	else
+		part_get_info(desc, part, &info);
 
 	if (desc->part_type == PART_TYPE_ISO) {
 		struct efi_device_path_cdrom_path *cddp = buf;
 
-		cddp->boot_entry = part - 1;
+		cddp->boot_entry = part;
 		cddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
 		cddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_CDROM_PATH;
 		cddp->dp.length = sizeof(*cddp);
@@ -434,7 +437,7 @@ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part)
 		hddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
 		hddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH;
 		hddp->dp.length = sizeof(*hddp);
-		hddp->partition_number = part - 1;
+		hddp->partition_number = part;
 		hddp->partition_start = info.start;
 		hddp->partition_end = info.size;
 		if (desc->part_type == PART_TYPE_EFI)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 4e457a841b..19f75aa919 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -274,6 +274,9 @@ static int efi_disk_create_partitions(struct blk_desc *desc,
 	disk_partition_t info;
 	int part;
 
+	/* Add devices for disk */
+	snprintf(devname, sizeof(devname), "%s", pdevname);
+	efi_disk_add_dev(devname, if_typename, desc, diskid, info.start, 0);
 	/* Add devices for each partition */
 	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
 		if (part_get_info(desc, part, &info))
@@ -315,7 +318,7 @@ int efi_disk_register(void)
 
 		/* Add block device for the full device */
 		efi_disk_add_dev(dev->name, if_typename, desc,
-				 desc->devnum, 0, 0);
+				 desc->devnum, 0, -1);
 
 		disks++;
 
@@ -351,7 +354,7 @@ int efi_disk_register(void)
 				 if_typename, i);
 
 			/* Add block device for the full device */
-			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
+			efi_disk_add_dev(devname, if_typename, desc, i, 0, -1);
 			disks++;
 
 			/* Partitions show up as block devices in EFI */
-- 
2.11.0

[U-Boot] [PATCH 3/3] efi_loader: correct DeviceNodeToText for media types

[Heinrich Schuchardt]

When converting device nodes and paths to text we should
stick to the UEFI spec.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_device_path_to_text.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
index 7159c974d4..21c5678d20 100644
--- a/lib/efi_loader/efi_device_path_to_text.c
+++ b/lib/efi_loader/efi_device_path_to_text.c
@@ -90,7 +90,7 @@ static char *dp_msging(char *s, struct efi_device_path *dp)
 	case DEVICE_PATH_SUB_TYPE_MSG_USB: {
 		struct efi_device_path_usb *udp =
 			(struct efi_device_path_usb *)dp;
-		s += sprintf(s, "Usb(0x%x,0x%x)", udp->parent_port_number,
+		s += sprintf(s, "USB(0x%x,0x%x)", udp->parent_port_number,
 			     udp->usb_interface);
 		break;
 	}
@@ -124,10 +124,10 @@ static char *dp_msging(char *s, struct efi_device_path *dp)
 	case DEVICE_PATH_SUB_TYPE_MSG_MMC: {
 		const char *typename =
 			(dp->sub_type == DEVICE_PATH_SUB_TYPE_MSG_SD) ?
-					"SDCard" : "MMC";
+					"SD" : "eMMC";
 		struct efi_device_path_sd_mmc_path *sddp =
 			(struct efi_device_path_sd_mmc_path *)dp;
-		s += sprintf(s, "%s(Slot%u)", typename, sddp->slot_number);
+		s += sprintf(s, "%s(%u)", typename, sddp->slot_number);
 		break;
 	}
 	default:
@@ -147,18 +147,26 @@ static char *dp_media(char *s, struct efi_device_path *dp)
 
 		switch (hddp->signature_type) {
 		case SIG_TYPE_MBR:
-			s += sprintf(s, "HD(Part%d,Sig%08x)",
-				     hddp->partition_number,
-				     *(uint32_t *)sig);
+			s += sprintf(
+				s, "HD(%d,MBR,0x%08x,0x%llx,0x%llx)",
+				hddp->partition_number,
+				*(uint32_t *)sig,
+				(unsigned long long int)hddp->partition_start,
+				(unsigned long long int)hddp->partition_end);
 			break;
 		case SIG_TYPE_GUID:
-			s += sprintf(s, "HD(Part%d,Sig%pUl)",
-				     hddp->partition_number, sig);
+			s += sprintf(
+				s, "HD(%d,GPT,%pUl,0x%llx,0x%llx)",
+				hddp->partition_number, sig,
+				(unsigned long long int)hddp->partition_start,
+				(unsigned long long int)hddp->partition_end);
 			break;
 		default:
-			s += sprintf(s, "HD(Part%d,MBRType=%02x,SigType=%02x)",
-				     hddp->partition_number, hddp->partmap_type,
-				     hddp->signature_type);
+			s += sprintf(
+				s, "HD(%d,0x%02x,0,0x%llx,0x%llx)",
+				hddp->partition_number, hddp->partmap_type,
+				(unsigned long long int)hddp->partition_start,
+				(unsigned long long int)hddp->partition_end);
 			break;
 		}
 
-- 
2.11.0

[U-Boot] [PATCH 2/3] efi_loader: correctly setup device paths for block devices

[Mark Kettenis]

Heinrich Schuchardt schreef op 2017-12-02 13:42:
> For each block device we have to setup:
> - a device path for the block device
> - a partition device path for the block device with
>   partition number 0

Do x86 UEFI implementations actually generate these partition number 0 
paths?
The standard says that 0 can be used in tis fashion but doesn't seem to
prescribe their use.

> - a partition device path for each partition
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_device_path.c | 11 +++++++----
>  lib/efi_loader/efi_disk.c        |  7 +++++--
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_device_path.c 
> b/lib/efi_loader/efi_device_path.c
> index 42fe6e1185..d0d62ff428 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -412,15 +412,18 @@ static void *dp_part_fill(void *buf, struct
> blk_desc *desc, int part)
>  	buf = &udp[1];
>  #endif
> 
> -	if (part == 0) /* the actual disk, not a partition */
> +	if (part == -1) /* the actual disk, not a partition */
>  		return buf;
> 
> -	part_get_info(desc, part, &info);
> +	if (part == 0)
> +		part_get_info_whole_disk(desc, &info);
> +	else
> +		part_get_info(desc, part, &info);
> 
>  	if (desc->part_type == PART_TYPE_ISO) {
>  		struct efi_device_path_cdrom_path *cddp = buf;
> 
> -		cddp->boot_entry = part - 1;
> +		cddp->boot_entry = part;
>  		cddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
>  		cddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_CDROM_PATH;
>  		cddp->dp.length = sizeof(*cddp);
> @@ -434,7 +437,7 @@ static void *dp_part_fill(void *buf, struct
> blk_desc *desc, int part)
>  		hddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
>  		hddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH;
>  		hddp->dp.length = sizeof(*hddp);
> -		hddp->partition_number = part - 1;
> +		hddp->partition_number = part;
>  		hddp->partition_start = info.start;
>  		hddp->partition_end = info.size;
>  		if (desc->part_type == PART_TYPE_EFI)
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 4e457a841b..19f75aa919 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -274,6 +274,9 @@ static int efi_disk_create_partitions(struct 
> blk_desc *desc,
>  	disk_partition_t info;
>  	int part;
> 
> +	/* Add devices for disk */
> +	snprintf(devname, sizeof(devname), "%s", pdevname);
> +	efi_disk_add_dev(devname, if_typename, desc, diskid, info.start, 0);
>  	/* Add devices for each partition */
>  	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>  		if (part_get_info(desc, part, &info))
> @@ -315,7 +318,7 @@ int efi_disk_register(void)
> 
>  		/* Add block device for the full device */
>  		efi_disk_add_dev(dev->name, if_typename, desc,
> -				 desc->devnum, 0, 0);
> +				 desc->devnum, 0, -1);
> 
>  		disks++;
> 
> @@ -351,7 +354,7 @@ int efi_disk_register(void)
>  				 if_typename, i);
> 
>  			/* Add block device for the full device */
> -			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
> +			efi_disk_add_dev(devname, if_typename, desc, i, 0, -1);
>  			disks++;
> 
>  			/* Partitions show up as block devices in EFI */

[U-Boot] [PATCH 2/3] efi_loader: correctly setup device paths for block devices

[xypron.glpk at gmx.de]

On Sunday, December 3, 2017 8:24:39 PM CET Mark Kettenis wrote:
> Heinrich Schuchardt schreef op 2017-12-02 13:42:
> > For each block device we have to setup:
> > - a device path for the block device
> > - a partition device path for the block device with
> > 
> >   partition number 0
> 
> Do x86 UEFI implementations actually generate these partition number 0
> paths?

Thanks for reviewing.

I do not have access the an x86 computer with UEFI firmware.
Maybe somebody else on the list has.

Another reference point is EDK2.

Part of the relevant coding is in function PartitionInstallMbrChildHandles(),
MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c:47.

Another part is in PartitionInstallChildHandle(),
MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c.

I could not identify any line referring to partition 0.

> The standard says that 0 can be used in tis fashion but doesn't seem to
> prescribe their use.

<cite>A partition number of  zero can be used to represent the raw hard drive 
or a raw extended partition.</cite>

Do you think we should not generate this entry?

Regards

Heinrich

> 
> > - a partition device path for each partition
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> > 
> >  lib/efi_loader/efi_device_path.c | 11 +++++++----
> >  lib/efi_loader/efi_disk.c        |  7 +++++--
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_device_path.c
> > b/lib/efi_loader/efi_device_path.c
> > index 42fe6e1185..d0d62ff428 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -412,15 +412,18 @@ static void *dp_part_fill(void *buf, struct
> > blk_desc *desc, int part)
> > 
> >  	buf = &udp[1];
> >  
> >  #endif
> > 
> > -	if (part == 0) /* the actual disk, not a partition */
> > +	if (part == -1) /* the actual disk, not a partition */
> > 
> >  		return buf;
> > 
> > -	part_get_info(desc, part, &info);
> > +	if (part == 0)
> > +		part_get_info_whole_disk(desc, &info);
> > +	else
> > +		part_get_info(desc, part, &info);
> > 
> >  	if (desc->part_type == PART_TYPE_ISO) {
> >  	
> >  		struct efi_device_path_cdrom_path *cddp = buf;
> > 
> > -		cddp->boot_entry = part - 1;
> > +		cddp->boot_entry = part;
> > 
> >  		cddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
> >  		cddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_CDROM_PATH;
> >  		cddp->dp.length = sizeof(*cddp);
> > 
> > @@ -434,7 +437,7 @@ static void *dp_part_fill(void *buf, struct
> > blk_desc *desc, int part)
> > 
> >  		hddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
> >  		hddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH;
> >  		hddp->dp.length = sizeof(*hddp);
> > 
> > -		hddp->partition_number = part - 1;
> > +		hddp->partition_number = part;
> > 
> >  		hddp->partition_start = info.start;
> >  		hddp->partition_end = info.size;
> >  		if (desc->part_type == PART_TYPE_EFI)
> > 
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index 4e457a841b..19f75aa919 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -274,6 +274,9 @@ static int efi_disk_create_partitions(struct
> > blk_desc *desc,
> > 
> >  	disk_partition_t info;
> >  	int part;
> > 
> > +	/* Add devices for disk */
> > +	snprintf(devname, sizeof(devname), "%s", pdevname);
> > +	efi_disk_add_dev(devname, if_typename, desc, diskid, info.start, 0);
> > 
> >  	/* Add devices for each partition */
> >  	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> >  	
> >  		if (part_get_info(desc, part, &info))
> > 
> > @@ -315,7 +318,7 @@ int efi_disk_register(void)
> > 
> >  		/* Add block device for the full device */
> >  		efi_disk_add_dev(dev->name, if_typename, desc,
> > 
> > -				 desc->devnum, 0, 0);
> > +				 desc->devnum, 0, -1);
> > 
> >  		disks++;
> > 
> > @@ -351,7 +354,7 @@ int efi_disk_register(void)
> > 
> >  				 if_typename, i);
> >  			
> >  			/* Add block device for the full device */
> > 
> > -			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
> > +			efi_disk_add_dev(devname, if_typename, desc, i, 0, -1);
> > 
> >  			disks++;
> >  			
> >  			/* Partitions show up as block devices in EFI */

[U-Boot] [PATCH 2/3] efi_loader: correctly setup device paths for block devices

[Alexander Graf]

On 03.12.17 22:20, xypron.glpk at gmx.de wrote:
> On Sunday, December 3, 2017 8:24:39 PM CET Mark Kettenis wrote:
>> Heinrich Schuchardt schreef op 2017-12-02 13:42:
>>> For each block device we have to setup:
>>> - a device path for the block device
>>> - a partition device path for the block device with
>>>
>>>   partition number 0
>>
>> Do x86 UEFI implementations actually generate these partition number 0
>> paths?
> 
> Thanks for reviewing.
> 
> I do not have access the an x86 computer with UEFI firmware.
> Maybe somebody else on the list has.
> 
> Another reference point is EDK2.
> 
> Part of the relevant coding is in function PartitionInstallMbrChildHandles(),
> MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c:47.
> 
> Another part is in PartitionInstallChildHandle(),
> MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c.
> 
> I could not identify any line referring to partition 0.
> 
>> The standard says that 0 can be used in tis fashion but doesn't seem to
>> prescribe their use.
> 
> <cite>A partition number of  zero can be used to represent the raw hard drive 
> or a raw extended partition.</cite>
> 
> Do you think we should not generate this entry?

Given that basically all major UEFI implementations are based on edk2, I
think we should just do it exactly the same way :)

  https://wiki.linaro.org/LEG/UEFIforQEMU

To give you example grub lsefi output for a simple disk with 2 MBR
partitions:

== U-Boot w/o this patch set (SD) ==

  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/EndEntire
  block
  device path
Handle 0x9fef8f78
  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/HD(0,800,32000,2de808cb00000000,1,1)/EndEntire
  block
  device path
  simple FS
Handle 0x9fef90a0
  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/HD(1,32800,1cd800,2de808cb00000000,1,1)/EndEntire
  block
  device path
  simple FS

== U-Boot w/ this patch set (SD) ==

Handle 0x9fef7598
  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/EndEntire
  block
  device path
  simple FS
Handle 0x9fef76c0
  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/HD(0,0,200000,2de808cb00000000,1,1)/EndEntire
  block
  device path
Handle 0x9fef7020
  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/HD(1,800,32000,2de808cb00000000,1,1)/EndEntire
  block
  device path
  simple FS
Handle 0x9fef7228
  /HardwareVendor(e61d73b9-a384-4acc-aeab-82e828f3628b)[0:
]/USB(6,0)/HD(2,32800,1cd800,2de808cb00000000,1,1)/EndEntire
  block
  device path
  simple FS

== edk2 (virtio) ==

Handle 0x43250d10
  /ACPI(a0341d0,0)/PCI(0,1)/EndEntire
  disk
  block
  fa920010-6785-4941-b6ec-498c579f160a
  PCI
  device path
Handle 0x4223af90
  /ACPI(a0341d0,0)/PCI(0,1)/HD(1,800,32000,2de808cb00000000,1,1)/EndEntire
  simple FS
  disk
  c12a7328-f81f-11d2-ba4b-00a0c93ec93b
  8cf2f62c-bc9b-4821-808d-ec9ec421a1a0
  block
  device path
Handle 0x4223ac90

/ACPI(a0341d0,0)/PCI(0,1)/HD(2,32800,1cd800,2de808cb00000000,1,1)/EndEntire
  simple FS
  disk
  8cf2f62c-bc9b-4821-808d-ec9ec421a1a0
  block
  device path


=====

So I think the only patch we really need is something like this:

diff --git a/lib/efi_loader/efi_device_path.c
b/lib/efi_loader/efi_device_path.c
index b4e2f933cb..9ba6b04e5e 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -416,7 +416,7 @@ static void *dp_part_fill(void *buf, struct blk_desc
*desc, int part)
 		hddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
 		hddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH;
 		hddp->dp.length = sizeof(*hddp);
-		hddp->partition_number = part - 1;
+		hddp->partition_number = part;
 		hddp->partition_start = info.start;
 		hddp->partition_end = info.size;
 		if (desc->part_type == PART_TYPE_EFI)

But this can wait until after rc1, so I'll send the pull request as is
out now :).


Alex

[U-Boot] [PATCH 1/3] efi_loader: correctly determine if an MMC device is an SD-card

[Alexander Graf]

On 02.12.17 13:42, Heinrich Schuchardt wrote:
> The SD cards and eMMC devices have different device nodes.
> The current coding interpretes all MMC devices as eMMC.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_device_path.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index b4e2f933cb..42fe6e1185 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -36,6 +36,24 @@ static const struct efi_device_path_vendor ROOT = {
>  	.guid = U_BOOT_GUID,
>  };
>  
> +#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC)
> +/*
> + * Determine if an MMC device is an SD card.
> + *
> + * @desc	block device descriptor
> + * @return	true if the device is an SD card
> + */
> +static bool is_sd(struct blk_desc *desc)
> +{
> +	struct mmc *mmc = find_mmc_device(desc->devnum);
> +
> +	if (!mmc)
> +		return false;
> +
> +	return IS_SD(mmc) != 0U;
> +}
> +#endif
> +
>  static void *dp_alloc(size_t sz)
>  {
>  	void *buf;
> @@ -298,9 +316,9 @@ static void *dp_fill(void *buf, struct udevice *dev)
>  		struct blk_desc *desc = mmc_get_blk_desc(mmc);
>  
>  		sddp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> -		sddp->dp.sub_type = (desc->if_type == IF_TYPE_MMC) ?
> -			DEVICE_PATH_SUB_TYPE_MSG_MMC :
> -			DEVICE_PATH_SUB_TYPE_MSG_SD;
> +		sddp->dp.sub_type = is_sd(desc) ?
> +			DEVICE_PATH_SUB_TYPE_MSG_SD :
> +			DEVICE_PATH_SUB_TYPE_MSG_MMC;

So in general, MMC != SD != eMMC. Or rather eMMC "is like" SD "is like"
MMC maybe?

I really don't know how edk2 distinguishes between them. In general,
eMMC is an SD card on steroids. Can you double-check in the edk2 code
and then we just do whatever they do?


Alex

[U-Boot] [PATCH 1/3] efi_loader: correctly determine if an MMC device is an SD-card

[Heinrich Schuchardt]

On 12/04/2017 12:16 AM, Alexander Graf wrote:
> 
> 
> On 02.12.17 13:42, Heinrich Schuchardt wrote:
>> The SD cards and eMMC devices have different device nodes.
>> The current coding interpretes all MMC devices as eMMC.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   lib/efi_loader/efi_device_path.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>> index b4e2f933cb..42fe6e1185 100644
>> --- a/lib/efi_loader/efi_device_path.c
>> +++ b/lib/efi_loader/efi_device_path.c
>> @@ -36,6 +36,24 @@ static const struct efi_device_path_vendor ROOT = {
>>   	.guid = U_BOOT_GUID,
>>   };
>>   
>> +#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC)
>> +/*
>> + * Determine if an MMC device is an SD card.
>> + *
>> + * @desc	block device descriptor
>> + * @return	true if the device is an SD card
>> + */
>> +static bool is_sd(struct blk_desc *desc)
>> +{
>> +	struct mmc *mmc = find_mmc_device(desc->devnum);
>> +
>> +	if (!mmc)
>> +		return false;
>> +
>> +	return IS_SD(mmc) != 0U;
>> +}
>> +#endif
>> +
>>   static void *dp_alloc(size_t sz)
>>   {
>>   	void *buf;
>> @@ -298,9 +316,9 @@ static void *dp_fill(void *buf, struct udevice *dev)
>>   		struct blk_desc *desc = mmc_get_blk_desc(mmc);
>>   
>>   		sddp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>> -		sddp->dp.sub_type = (desc->if_type == IF_TYPE_MMC) ?
>> -			DEVICE_PATH_SUB_TYPE_MSG_MMC :
>> -			DEVICE_PATH_SUB_TYPE_MSG_SD;
>> +		sddp->dp.sub_type = is_sd(desc) ?
>> +			DEVICE_PATH_SUB_TYPE_MSG_SD :
>> +			DEVICE_PATH_SUB_TYPE_MSG_MMC;
> 
> So in general, MMC != SD != eMMC. Or rather eMMC "is like" SD "is like"
> MMC maybe?
> 
> I really don't know how edk2 distinguishes between them. In general,
> eMMC is an SD card on steroids. Can you double-check in the edk2 code
> and then we just do whatever they do?
> 
> 
> Alex
> 

EDK2 defines the following conversion functions:
   {L"SD",                      DevPathFromTextSd                      },
   {L"eMMC",                    DevPathFromTextEmmc                    },
in MdePkg/Library/UefiDevicePathLib/DevicePathFromText.c:3465
and
   {MESSAGING_DEVICE_PATH, MSG_SD_DP, DevPathToTextSd             },
   {MESSAGING_DEVICE_PATH, MSG_EMMC_DP, DevPathToTextEmmc         },
in MdePkg/Library/UefiDevicePathLib/DevicePathToText.c:2256

Regards

Heinrich

[U-Boot] [PATCH 2/3] efi_loader: correctly setup device paths for block devices

[Mark Kettenis]

xypron.glpk at gmx.de schreef op 2017-12-03 22:20:
> On Sunday, December 3, 2017 8:24:39 PM CET Mark Kettenis wrote:
>> Heinrich Schuchardt schreef op 2017-12-02 13:42:
>> > For each block device we have to setup:
>> > - a device path for the block device
>> > - a partition device path for the block device with
>> >
>> >   partition number 0
>> 
>> Do x86 UEFI implementations actually generate these partition number 0
>> paths?
> 
> Thanks for reviewing.
> 
> I do not have access the an x86 computer with UEFI firmware.
> Maybe somebody else on the list has.
> 
> Another reference point is EDK2.
> 
> Part of the relevant coding is in function 
> PartitionInstallMbrChildHandles(),
> MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c:47.
> 
> Another part is in PartitionInstallChildHandle(),
> MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c.
> 
> I could not identify any line referring to partition 0.
> 
>> The standard says that 0 can be used in tis fashion but doesn't seem 
>> to
>> prescribe their use.
> 
> <cite>A partition number of  zero can be used to represent the raw hard 
> drive
> or a raw extended partition.</cite>
> 
> Do you think we should not generate this entry?

I think it would be better to not generate this entry if other UEFI 
implementations don't generate it either.
Your analysis of the EDK2 code suggests that they don't, so U-Boot 
probably shouldn't do this either.

Was going to check whether the EDK2-based firmware on my Overdrive 1000 
provides a partition 0, but I managed
to hang it in the process and I'll have to power-cycle it when I get 
home later today...

>> > - a partition device path for each partition
>> >
>> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> > ---
>> >
>> >  lib/efi_loader/efi_device_path.c | 11 +++++++----
>> >  lib/efi_loader/efi_disk.c        |  7 +++++--
>> >  2 files changed, 12 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/lib/efi_loader/efi_device_path.c
>> > b/lib/efi_loader/efi_device_path.c
>> > index 42fe6e1185..d0d62ff428 100644
>> > --- a/lib/efi_loader/efi_device_path.c
>> > +++ b/lib/efi_loader/efi_device_path.c
>> > @@ -412,15 +412,18 @@ static void *dp_part_fill(void *buf, struct
>> > blk_desc *desc, int part)
>> >
>> >  	buf = &udp[1];
>> >
>> >  #endif
>> >
>> > -	if (part == 0) /* the actual disk, not a partition */
>> > +	if (part == -1) /* the actual disk, not a partition */
>> >
>> >  		return buf;
>> >
>> > -	part_get_info(desc, part, &info);
>> > +	if (part == 0)
>> > +		part_get_info_whole_disk(desc, &info);
>> > +	else
>> > +		part_get_info(desc, part, &info);
>> >
>> >  	if (desc->part_type == PART_TYPE_ISO) {
>> >
>> >  		struct efi_device_path_cdrom_path *cddp = buf;
>> >
>> > -		cddp->boot_entry = part - 1;
>> > +		cddp->boot_entry = part;
>> >
>> >  		cddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
>> >  		cddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_CDROM_PATH;
>> >  		cddp->dp.length = sizeof(*cddp);
>> >
>> > @@ -434,7 +437,7 @@ static void *dp_part_fill(void *buf, struct
>> > blk_desc *desc, int part)
>> >
>> >  		hddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
>> >  		hddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH;
>> >  		hddp->dp.length = sizeof(*hddp);
>> >
>> > -		hddp->partition_number = part - 1;
>> > +		hddp->partition_number = part;
>> >
>> >  		hddp->partition_start = info.start;
>> >  		hddp->partition_end = info.size;
>> >  		if (desc->part_type == PART_TYPE_EFI)
>> >
>> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> > index 4e457a841b..19f75aa919 100644
>> > --- a/lib/efi_loader/efi_disk.c
>> > +++ b/lib/efi_loader/efi_disk.c
>> > @@ -274,6 +274,9 @@ static int efi_disk_create_partitions(struct
>> > blk_desc *desc,
>> >
>> >  	disk_partition_t info;
>> >  	int part;
>> >
>> > +	/* Add devices for disk */
>> > +	snprintf(devname, sizeof(devname), "%s", pdevname);
>> > +	efi_disk_add_dev(devname, if_typename, desc, diskid, info.start, 0);
>> >
>> >  	/* Add devices for each partition */
>> >  	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>> >
>> >  		if (part_get_info(desc, part, &info))
>> >
>> > @@ -315,7 +318,7 @@ int efi_disk_register(void)
>> >
>> >  		/* Add block device for the full device */
>> >  		efi_disk_add_dev(dev->name, if_typename, desc,
>> >
>> > -				 desc->devnum, 0, 0);
>> > +				 desc->devnum, 0, -1);
>> >
>> >  		disks++;
>> >
>> > @@ -351,7 +354,7 @@ int efi_disk_register(void)
>> >
>> >  				 if_typename, i);
>> >
>> >  			/* Add block device for the full device */
>> >
>> > -			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
>> > +			efi_disk_add_dev(devname, if_typename, desc, i, 0, -1);
>> >
>> >  			disks++;
>> >
>> >  			/* Partitions show up as block devices in EFI */