diff mbox series

efi_loader: Fix partition offsets

Message ID 20171201151033.88189-1-agraf@suse.de
State Accepted
Commit c034b7fd419010116f2b6ac3b8e3b725f425d92c
Headers show
Series efi_loader: Fix partition offsets | expand

Commit Message

Alexander Graf Dec. 1, 2017, 3:10 p.m. UTC
Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
to introduce the el torito scheme to all partition table types: Spawn
individual disk objects for each partition on a disk.

Unfortunately, that code ended up creating partitions with offset=0 which meant
that anyone accessing these objects gets data from the raw block device instead
of the partition.

Furthermore, all the el torito logic to spawn devices for partitions was
duplicated. So let's merge the two code paths and give partition disk objects
good offsets to work from, so that payloads can actually make use of them.

Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_disk.c | 60 ++++++++++-------------------------------------
 1 file changed, 13 insertions(+), 47 deletions(-)

Comments

Heinrich Schuchardt Dec. 2, 2017, 4:53 a.m. UTC | #1
On 12/01/2017 04:10 PM, Alexander Graf wrote:
> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> to introduce the el torito scheme to all partition table types: Spawn
> individual disk objects for each partition on a disk.
> 
> Unfortunately, that code ended up creating partitions with offset=0 which meant
> that anyone accessing these objects gets data from the raw block device instead
> of the partition.
> 
> Furthermore, all the el torito logic to spawn devices for partitions was
> duplicated. So let's merge the two code paths and give partition disk objects
> good offsets to work from, so that payloads can actually make use of them.
> 
> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>   lib/efi_loader/efi_disk.c | 60 ++++++++++-------------------------------------
>   1 file changed, 13 insertions(+), 47 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 68ba2cf7b2..4e457a841b 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -264,21 +264,17 @@ out_of_memory:
>   	printf("ERROR: Out of memory\n");
>   }
>   
> -static int efi_disk_create_eltorito(struct blk_desc *desc,
> -				    const char *if_typename,
> -				    int diskid,
> -				    const char *pdevname)
> +static int efi_disk_create_partitions(struct blk_desc *desc,
> +				      const char *if_typename,
> +				      int diskid,
> +				      const char *pdevname)
>   {
>   	int disks = 0;
> -#if CONFIG_IS_ENABLED(ISO_PARTITION)
>   	char devname[32] = { 0 }; /* dp->str is u16[32] long */
>   	disk_partition_t info;
>   	int part;
>   
> -	if (desc->part_type != PART_TYPE_ISO)
> -		return 0;
> -
> -	/* and devices for each partition: */
> +	/* Add devices for each partition */
>   	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>   		if (part_get_info(desc, part, &info))
>   			continue;
> @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc,
>   		disks++;
>   	}
>   
> -	/* ... and add block device: */
> -	efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0);
> -#endif
> -
>   	return disks;
>   }
>   
> @@ -318,31 +310,18 @@ int efi_disk_register(void)
>   	     uclass_next_device_check(&dev)) {
>   		struct blk_desc *desc = dev_get_uclass_platdata(dev);
>   		const char *if_typename = dev->driver->name;
> -		disk_partition_t info;
> -		int part;
>   
>   		printf("Scanning disk %s...\n", dev->name);
>   
> -		/* add devices for each partition: */
> -		for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> -			if (part_get_info(desc, part, &info))
> -				continue;
> -			efi_disk_add_dev(dev->name, if_typename, desc,
> -					 desc->devnum, 0, part);
> -		}
> -
> -		/* ... and add block device: */
> +		/* Add block device for the full device */
>   		efi_disk_add_dev(dev->name, if_typename, desc,
>   				 desc->devnum, 0, 0);
>   
>   		disks++;
>   
> -		/*
> -		* El Torito images show up as block devices in an EFI world,
> -		* so let's create them here
> -		*/
> -		disks += efi_disk_create_eltorito(desc, if_typename,
> -						  desc->devnum, dev->name);
> +		/* Partitions show up as block devices in EFI */
> +		disks += efi_disk_create_partitions(desc, if_typename,
> +						    desc->devnum, dev->name);
>   	}
>   #else >   	int i, if_type;
> @@ -361,8 +340,6 @@ int efi_disk_register(void)
>   		for (i = 0; i < 4; i++) {

include/configs/highbank.h allows 5 SCSI drives.
include/configs/qemu-arm.h allows 6 SCSI drives.

I guess you want to use drv->max_devs as upper limit.

>   			struct blk_desc *desc;
>   			char devname[32] = { 0 }; /* dp->str is u16[32] long */
> -			disk_partition_t info;
> -			int part;
>   
>   			desc = blk_get_devnum_by_type(if_type, i);
>   			if (!desc)
> @@ -373,24 +350,13 @@ int efi_disk_register(void)
>   			snprintf(devname, sizeof(devname), "%s%d",
>   				 if_typename, i);
>   
> -			/* add devices for each partition: */
> -			for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> -				if (part_get_info(desc, part, &info))
> -					continue;
> -				efi_disk_add_dev(devname, if_typename, desc,
> -						 i, 0, part);
> -			}
> -
> -			/* ... and add block device: */
> +			/* Add block device for the full device */
>   			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
>   			disks++;
>   
> -			/*
> -			 * El Torito images show up as block devices
> -			 * in an EFI world, so let's create them here
> -			 */
> -			disks += efi_disk_create_eltorito(desc, if_typename,
> -							  i, devname);
> +			/* Partitions show up as block devices in EFI */
> +			disks += efi_disk_create_partitions(desc, if_typename,
> +							    i, devname);
>   		}
>   	}
>   #endif
> 

I tested on an Odroid C2 with four partitions on an SD card (device 
mmc0). Device mmc1 is not installed but defined in the dtb.

Bootefi selftest (with some debug output added) showed:

=> bootefi selftest 

lib/efi_loader/efi_disk.c (311) efi_disk_register, dev = 
000000007df72600 

Scanning disk mmc@72000.blk...
lib/efi_loader/efi_disk.c (320) efi_disk_register, disks = 0
lib/efi_loader/efi_disk.c (323) efi_disk_register, disks = 1
lib/efi_loader/efi_disk.c (287) efi_disk_create_partitions, disks = 4
lib/efi_loader/efi_disk.c (328) efi_disk_register, disks = 5

Up to this point everything is ok.
We created one device of the complete disk and four devices for the 
partions.
Now it starts going wrong:

Card did not respond to voltage select! 

mmc_init: -95, time 9
lib/efi_loader/efi_disk.c (311) efi_disk_register, dev = 
000000007df72930 

Scanning disk mmc@74000.blk... 

lib/efi_loader/efi_disk.c (320) efi_disk_register, disks = 5 

lib/efi_loader/efi_disk.c (323) efi_disk_register, disks = 6 

lib/efi_loader/efi_disk.c (287) efi_disk_create_partitions, disks = 0 

lib/efi_loader/efi_disk.c (328) efi_disk_register, disks = 6 

Found 6 disks

So here we are creating a disk for an MMC device that does not 
physically exist. Can't we check the existence? I wonder why 
uclass_next_device_check does not skip the drive.

Only the following MMC related device paths are created:

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part0,Sig6fe30000)

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part1,Sig6fe30000)

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part2,Sig6fe30000)

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part3,Sig6fe30000)

The device paths with partition number 0 for the disk devices are 
missing and the partitions incorrectly numbered 0 to 3 instead of 1 to 4.

But I guess this problem is in another part of the code.

Best regards

Heinrich
Alexander Graf Dec. 4, 2017, 8:59 a.m. UTC | #2
> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> to introduce the el torito scheme to all partition table types: Spawn
> individual disk objects for each partition on a disk.
> 
> Unfortunately, that code ended up creating partitions with offset=0 which meant
> that anyone accessing these objects gets data from the raw block device instead
> of the partition.
> 
> Furthermore, all the el torito logic to spawn devices for partitions was
> duplicated. So let's merge the two code paths and give partition disk objects
> good offsets to work from, so that payloads can actually make use of them.
> 
> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex
Jonathan Gray Dec. 7, 2017, 7 a.m. UTC | #3
On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> to introduce the el torito scheme to all partition table types: Spawn
> individual disk objects for each partition on a disk.
> 
> Unfortunately, that code ended up creating partitions with offset=0 which meant
> that anyone accessing these objects gets data from the raw block device instead
> of the partition.
> 
> Furthermore, all the el torito logic to spawn devices for partitions was
> duplicated. So let's merge the two code paths and give partition disk objects
> good offsets to work from, so that payloads can actually make use of them.
> 
> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>

This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
node with the loaded image protocol on rpi_3 with mmc/usb.

broken
## Starting EFI application at 01000000 ...
Scanning disk sdhci@7e300000.blk...
efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1
efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4
Scanning disk usb_mass_storage.lun0...
efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1
efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4
Found 6 disks
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 11 type 3 b: len 11 type: 3
efi_dp_match a: len 11 type 3 b: len 11 type: 3
efi_dp_match a: len 11 type 3 b: len 11 type: 3
efi_diskprobe
efi_device_path_depth looking for type 4 i=0 type 1
efi_device_path_depth looking for type 4 i=1 type 3
efi_device_path_depth looking for type 4 i=2 type 3
efi_device_path_depth looking for type 4 i=3 type 3
efi_device_path_depth no match for type 4
depth=-1

working (this commit reverted)
## Starting EFI application at 01000000 ...
Scanning disk sdhci@7e300000.blk...
efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=1
efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=4
efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
Scanning disk usb_mass_storage.lun0...
efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=1
efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=4
efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
Found 2 disks
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 20 type 1 b: len 20 type: 1
efi_dp_match a: len 11 type 3 b: len 11 type: 3
efi_dp_match a: len 11 type 3 b: len 11 type: 3
efi_dp_match a: len 11 type 3 b: len 11 type: 3
efi_dp_match a: len 42 type 4 b: len 42 type: 4
efi_diskprobe
efi_device_path_depth looking for type 4 i=0 type 1
efi_device_path_depth looking for type 4 i=1 type 3
efi_device_path_depth looking for type 4 i=2 type 3
efi_device_path_depth looking for type 4 i=3 type 3
efi_device_path_depth looking for type 4 i=4 type 4
depth=4

> ---
>  lib/efi_loader/efi_disk.c | 60 ++++++++++-------------------------------------
>  1 file changed, 13 insertions(+), 47 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 68ba2cf7b2..4e457a841b 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -264,21 +264,17 @@ out_of_memory:
>  	printf("ERROR: Out of memory\n");
>  }
>  
> -static int efi_disk_create_eltorito(struct blk_desc *desc,
> -				    const char *if_typename,
> -				    int diskid,
> -				    const char *pdevname)
> +static int efi_disk_create_partitions(struct blk_desc *desc,
> +				      const char *if_typename,
> +				      int diskid,
> +				      const char *pdevname)
>  {
>  	int disks = 0;
> -#if CONFIG_IS_ENABLED(ISO_PARTITION)
>  	char devname[32] = { 0 }; /* dp->str is u16[32] long */
>  	disk_partition_t info;
>  	int part;
>  
> -	if (desc->part_type != PART_TYPE_ISO)
> -		return 0;
> -
> -	/* and devices for each partition: */
> +	/* Add devices for each partition */
>  	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>  		if (part_get_info(desc, part, &info))
>  			continue;
> @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc,
>  		disks++;
>  	}
>  
> -	/* ... and add block device: */
> -	efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0);
> -#endif
> -
>  	return disks;
>  }
>  
> @@ -318,31 +310,18 @@ int efi_disk_register(void)
>  	     uclass_next_device_check(&dev)) {
>  		struct blk_desc *desc = dev_get_uclass_platdata(dev);
>  		const char *if_typename = dev->driver->name;
> -		disk_partition_t info;
> -		int part;
>  
>  		printf("Scanning disk %s...\n", dev->name);
>  
> -		/* add devices for each partition: */
> -		for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> -			if (part_get_info(desc, part, &info))
> -				continue;
> -			efi_disk_add_dev(dev->name, if_typename, desc,
> -					 desc->devnum, 0, part);
> -		}
> -
> -		/* ... and add block device: */
> +		/* Add block device for the full device */
>  		efi_disk_add_dev(dev->name, if_typename, desc,
>  				 desc->devnum, 0, 0);
>  
>  		disks++;
>  
> -		/*
> -		* El Torito images show up as block devices in an EFI world,
> -		* so let's create them here
> -		*/
> -		disks += efi_disk_create_eltorito(desc, if_typename,
> -						  desc->devnum, dev->name);
> +		/* Partitions show up as block devices in EFI */
> +		disks += efi_disk_create_partitions(desc, if_typename,
> +						    desc->devnum, dev->name);
>  	}
>  #else
>  	int i, if_type;
> @@ -361,8 +340,6 @@ int efi_disk_register(void)
>  		for (i = 0; i < 4; i++) {
>  			struct blk_desc *desc;
>  			char devname[32] = { 0 }; /* dp->str is u16[32] long */
> -			disk_partition_t info;
> -			int part;
>  
>  			desc = blk_get_devnum_by_type(if_type, i);
>  			if (!desc)
> @@ -373,24 +350,13 @@ int efi_disk_register(void)
>  			snprintf(devname, sizeof(devname), "%s%d",
>  				 if_typename, i);
>  
> -			/* add devices for each partition: */
> -			for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> -				if (part_get_info(desc, part, &info))
> -					continue;
> -				efi_disk_add_dev(devname, if_typename, desc,
> -						 i, 0, part);
> -			}
> -
> -			/* ... and add block device: */
> +			/* Add block device for the full device */
>  			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
>  			disks++;
>  
> -			/*
> -			 * El Torito images show up as block devices
> -			 * in an EFI world, so let's create them here
> -			 */
> -			disks += efi_disk_create_eltorito(desc, if_typename,
> -							  i, devname);
> +			/* Partitions show up as block devices in EFI */
> +			disks += efi_disk_create_partitions(desc, if_typename,
> +							    i, devname);
>  		}
>  	}
>  #endif
> -- 
> 2.12.3
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Alexander Graf Dec. 7, 2017, 10:27 a.m. UTC | #4
On 07.12.17 08:00, Jonathan Gray wrote:
> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
>> to introduce the el torito scheme to all partition table types: Spawn
>> individual disk objects for each partition on a disk.
>>
>> Unfortunately, that code ended up creating partitions with offset=0 which meant
>> that anyone accessing these objects gets data from the raw block device instead
>> of the partition.
>>
>> Furthermore, all the el torito logic to spawn devices for partitions was
>> duplicated. So let's merge the two code paths and give partition disk objects
>> good offsets to work from, so that payloads can actually make use of them.
>>
>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> node with the loaded image protocol on rpi_3 with mmc/usb.

Hooray :). Do you think you could somehow assemble a test that runs
inside Travis-CI to make sure we catch your loader? I still need to do a
better one for grub to ensure that that one's happy too.

> 
> broken
> ## Starting EFI application at 01000000 ...
> Scanning disk sdhci@7e300000.blk...
> efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
> efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1
> efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4
> Scanning disk usb_mass_storage.lun0...
> efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
> efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1
> efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4
> Found 6 disks
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_diskprobe
> efi_device_path_depth looking for type 4 i=0 type 1
> efi_device_path_depth looking for type 4 i=1 type 3
> efi_device_path_depth looking for type 4 i=2 type 3
> efi_device_path_depth looking for type 4 i=3 type 3
> efi_device_path_depth no match for type 4
> depth=-1

I can reproduce this with grub here as well - the device path passed in
via loaded image seems to have something to it that's wrong. I can't
quite figure out what - printing it with our own helpers seems to show a
correct path:

 do_bootefi_exec:
dp=/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Usb(0x6,0x0)/HD(Part1,Sigcb08e82d)

I'll need to dig a bit further into this to understand where things are
going wrong...


Alex
Heinrich Schuchardt Dec. 7, 2017, 10:57 a.m. UTC | #5
On 12/07/2017 08:00 AM, Jonathan Gray wrote:
> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
>> to introduce the el torito scheme to all partition table types: Spawn
>> individual disk objects for each partition on a disk.
>>
>> Unfortunately, that code ended up creating partitions with offset=0 which meant
>> that anyone accessing these objects gets data from the raw block device instead
>> of the partition.
>>
>> Furthermore, all the el torito logic to spawn devices for partitions was
>> duplicated. So let's merge the two code paths and give partition disk objects
>> good offsets to work from, so that payloads can actually make use of them.
>>
>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> node with the loaded image protocol on rpi_3 with mmc/usb.

Could you, please, specify which software you are trying to call:
Linux EFI stub, Grub, or anything else?

Which patches did you consider?
Did you apply these patch series that are not yet in efi-next?
efi_loader: correct media device paths
efi_loader: avoid use after free

Regards

Heinrich
Jonathan Gray Dec. 7, 2017, 11:45 a.m. UTC | #6
On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote:
> On 12/07/2017 08:00 AM, Jonathan Gray wrote:
> > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
> >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> >> to introduce the el torito scheme to all partition table types: Spawn
> >> individual disk objects for each partition on a disk.
> >>
> >> Unfortunately, that code ended up creating partitions with offset=0 which meant
> >> that anyone accessing these objects gets data from the raw block device instead
> >> of the partition.
> >>
> >> Furthermore, all the el torito logic to spawn devices for partitions was
> >> duplicated. So let's merge the two code paths and give partition disk objects
> >> good offsets to work from, so that payloads can actually make use of them.
> >>
> >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> > 
> > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> > node with the loaded image protocol on rpi_3 with mmc/usb.
> 
> Could you, please, specify which software you are trying to call:
> Linux EFI stub, Grub, or anything else?

https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/
Disk image with fat/ffs filesystems
https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs

though it would likely show up on other archs as well

armv7 equivalents of the above
https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/
https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs

> 
> Which patches did you consider?
> Did you apply these patch series that are not yet in efi-next?
> efi_loader: correct media device paths
> efi_loader: avoid use after free

just master
c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path

with a small patch to force calling gnu sed for some non-portable
sed use in check-config.sh

'efi_loader: avoid use after free' doesn't help
'efi_loader: correct media device paths' doesn't either

> 
> Regards
> 
> Heinrich
Alexander Graf Dec. 8, 2017, 5:55 a.m. UTC | #7
On 07.12.17 12:45, Jonathan Gray wrote:
> On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote:
>> On 12/07/2017 08:00 AM, Jonathan Gray wrote:
>>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
>>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
>>>> to introduce the el torito scheme to all partition table types: Spawn
>>>> individual disk objects for each partition on a disk.
>>>>
>>>> Unfortunately, that code ended up creating partitions with offset=0 which meant
>>>> that anyone accessing these objects gets data from the raw block device instead
>>>> of the partition.
>>>>
>>>> Furthermore, all the el torito logic to spawn devices for partitions was
>>>> duplicated. So let's merge the two code paths and give partition disk objects
>>>> good offsets to work from, so that payloads can actually make use of them.
>>>>
>>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
>>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
>>> node with the loaded image protocol on rpi_3 with mmc/usb.
>>
>> Could you, please, specify which software you are trying to call:
>> Linux EFI stub, Grub, or anything else?
> 
> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/
> Disk image with fat/ffs filesystems
> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs
> 
> though it would likely show up on other archs as well
> 
> armv7 equivalents of the above
> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/
> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs
> 
>>
>> Which patches did you consider?
>> Did you apply these patch series that are not yet in efi-next?
>> efi_loader: correct media device paths
>> efi_loader: avoid use after free
> 
> just master
> c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path
> 
> with a small patch to force calling gnu sed for some non-portable
> sed use in check-config.sh
> 
> 'efi_loader: avoid use after free' doesn't help
> 'efi_loader: correct media device paths' doesn't either

As a quick heads-up: The device path matching is broken. The patch below
should fix it, but I want to create a travis-ci case around that first
and also wrap it up more nicely.

Alex


diff --git a/lib/efi_loader/efi_device_path.c
b/lib/efi_loader/efi_device_path.c
index b4e2f933cb..d12a38c450 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -126,6 +126,7 @@ static struct efi_object *find_obj(struct
efi_device_path *dp, bool short_path,
 				   struct efi_device_path **rem)
 {
 	struct efi_object *efiobj;
+	unsigned int dp_size = efi_dp_size(dp);

 	list_for_each_entry(efiobj, &efi_obj_list, link) {
 		struct efi_handler *handler;
@@ -141,10 +142,18 @@ static struct efi_object *find_obj(struct
efi_device_path *dp, bool short_path,
 		do {
 			if (efi_dp_match(dp, obj_dp) == 0) {
 				if (rem) {
+					/*
+					 * Allow partial matches, but inform
+					 * the caller.
+					 */
 					*rem = ((void *)dp) +
 						efi_dp_size(obj_dp);
+					return efiobj;
+				} else {
+					/* Only return on exact matches */
+					if (efi_dp_size(obj_dp) == dp_size)
+						return efiobj;
 				}
-				return efiobj;
 			}

 			obj_dp = shorten_path(efi_dp_next(obj_dp));
@@ -164,8 +173,14 @@ struct efi_object *efi_dp_find_obj(struct
efi_device_path *dp,
 {
 	struct efi_object *efiobj;

-	efiobj = find_obj(dp, false, rem);
+	/* Search for an exact match first */
+	efiobj = find_obj(dp, false, NULL);

+	/* Then for a fuzzy match */
+	if (!efiobj)
+		efiobj = find_obj(dp, false, rem);
+
+	/* And now for a fuzzy short match */
 	if (!efiobj)
 		efiobj = find_obj(dp, true, rem);
Jonathan Gray Dec. 8, 2017, 6:13 a.m. UTC | #8
On Fri, Dec 08, 2017 at 06:55:02AM +0100, Alexander Graf wrote:
> 
> 
> On 07.12.17 12:45, Jonathan Gray wrote:
> > On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote:
> >> On 12/07/2017 08:00 AM, Jonathan Gray wrote:
> >>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
> >>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> >>>> to introduce the el torito scheme to all partition table types: Spawn
> >>>> individual disk objects for each partition on a disk.
> >>>>
> >>>> Unfortunately, that code ended up creating partitions with offset=0 which meant
> >>>> that anyone accessing these objects gets data from the raw block device instead
> >>>> of the partition.
> >>>>
> >>>> Furthermore, all the el torito logic to spawn devices for partitions was
> >>>> duplicated. So let's merge the two code paths and give partition disk objects
> >>>> good offsets to work from, so that payloads can actually make use of them.
> >>>>
> >>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> >>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>
> >>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> >>> node with the loaded image protocol on rpi_3 with mmc/usb.
> >>
> >> Could you, please, specify which software you are trying to call:
> >> Linux EFI stub, Grub, or anything else?
> > 
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI
> > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/
> > Disk image with fat/ffs filesystems
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs
> > 
> > though it would likely show up on other archs as well
> > 
> > armv7 equivalents of the above
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI
> > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs
> > 
> >>
> >> Which patches did you consider?
> >> Did you apply these patch series that are not yet in efi-next?
> >> efi_loader: correct media device paths
> >> efi_loader: avoid use after free
> > 
> > just master
> > c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path
> > 
> > with a small patch to force calling gnu sed for some non-portable
> > sed use in check-config.sh
> > 
> > 'efi_loader: avoid use after free' doesn't help
> > 'efi_loader: correct media device paths' doesn't either
> 
> As a quick heads-up: The device path matching is broken. The patch below
> should fix it, but I want to create a travis-ci case around that first
> and also wrap it up more nicely.
> 
> Alex

Yes, this does fix it.  Thanks.

Your mail client wrapped lines so I had to adjust it to apply.  Including
it again with that fixed:

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index b4e2f933cb..24a4f40c00 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -126,6 +126,7 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
 				   struct efi_device_path **rem)
 {
 	struct efi_object *efiobj;
+	unsigned int dp_size = efi_dp_size(dp);
 
 	list_for_each_entry(efiobj, &efi_obj_list, link) {
 		struct efi_handler *handler;
@@ -141,10 +142,18 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
 		do {
 			if (efi_dp_match(dp, obj_dp) == 0) {
 				if (rem) {
+					/*
+					 * Allow partial matches, but inform
+					 * the caller.
+					 */
 					*rem = ((void *)dp) +
 						efi_dp_size(obj_dp);
+					return efiobj;
+				} else {
+					/* Only return on exact matches */
+					if (efi_dp_size(obj_dp) == dp_size)
+						return efiobj;
 				}
-				return efiobj;
 			}
 
 			obj_dp = shorten_path(efi_dp_next(obj_dp));
@@ -164,8 +173,14 @@ struct efi_object *efi_dp_find_obj(struct efi_device_path *dp,
 {
 	struct efi_object *efiobj;
 
-	efiobj = find_obj(dp, false, rem);
+	/* Search for an exact match first */
+	efiobj = find_obj(dp, false, NULL);
 
+	/* Then for a fuzzy match */
+	if (!efiobj)
+		efiobj = find_obj(dp, false, rem);
+
+	/* And now for a fuzzy short match */
 	if (!efiobj)
 		efiobj = find_obj(dp, true, rem);
Heinrich Schuchardt Dec. 8, 2017, 7:02 a.m. UTC | #9
On 12/08/2017 06:55 AM, Alexander Graf wrote:
> 
> 
> On 07.12.17 12:45, Jonathan Gray wrote:
>> On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote:
>>> On 12/07/2017 08:00 AM, Jonathan Gray wrote:
>>>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
>>>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
>>>>> to introduce the el torito scheme to all partition table types: Spawn
>>>>> individual disk objects for each partition on a disk.
>>>>>
>>>>> Unfortunately, that code ended up creating partitions with offset=0 which meant
>>>>> that anyone accessing these objects gets data from the raw block device instead
>>>>> of the partition.
>>>>>
>>>>> Furthermore, all the el torito logic to spawn devices for partitions was
>>>>> duplicated. So let's merge the two code paths and give partition disk objects
>>>>> good offsets to work from, so that payloads can actually make use of them.
>>>>>
>>>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
>>>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>
>>>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
>>>> node with the loaded image protocol on rpi_3 with mmc/usb.
>>>
>>> Could you, please, specify which software you are trying to call:
>>> Linux EFI stub, Grub, or anything else?
>>
>> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI
>> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/
>> Disk image with fat/ffs filesystems
>> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs
>>
>> though it would likely show up on other archs as well
>>
>> armv7 equivalents of the above
>> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI
>> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/
>> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs
>>
>>>
>>> Which patches did you consider?
>>> Did you apply these patch series that are not yet in efi-next?
>>> efi_loader: correct media device paths
>>> efi_loader: avoid use after free
>>
>> just master
>> c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path
>>
>> with a small patch to force calling gnu sed for some non-portable
>> sed use in check-config.sh
>>
>> 'efi_loader: avoid use after free' doesn't help
>> 'efi_loader: correct media device paths' doesn't either
> 
> As a quick heads-up: The device path matching is broken. The patch below
> should fix it, but I want to create a travis-ci case around that first
> and also wrap it up more nicely.
> 
> Alex
> 
> 
> diff --git a/lib/efi_loader/efi_device_path.c
> b/lib/efi_loader/efi_device_path.c
> index b4e2f933cb..d12a38c450 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c

Hello Alex,

thank you for your further investigations

We really need comments above the functions stating what they are 
expected to do.

> @@ -126,6 +126,7 @@ static struct efi_object *find_obj(struct
> efi_device_path *dp, bool short_path,
>   				   struct efi_device_path **rem)
>   {
>   	struct efi_object *efiobj;
> +	unsigned int dp_size = efi_dp_size(dp);
> 
>   	list_for_each_entry(efiobj, &efi_obj_list, link) {
>   		struct efi_handler *handler;
> @@ -141,10 +142,18 @@ static struct efi_object *find_obj(struct
> efi_device_path *dp, bool short_path,
>   		do {
>   			if (efi_dp_match(dp, obj_dp) == 0) {
>   				if (rem) {
> +					/*
> +					 * Allow partial matches, but inform
> +					 * the caller.
> +					 */
>   					*rem = ((void *)dp) +
>   						efi_dp_size(obj_dp);
> +					return efiobj;
> +				} else {
> +					/* Only return on exact matches */
> +					if (efi_dp_size(obj_dp) == dp_size)
> +						return efiobj;
>   				}
> -				return efiobj;
>   			}
> 
>   			obj_dp = shorten_path(efi_dp_next(obj_dp));
> @@ -164,8 +173,14 @@ struct efi_object *efi_dp_find_obj(struct
> efi_device_path *dp,
>   {
>   	struct efi_object *efiobj;
> 
> -	efiobj = find_obj(dp, false, rem);
> +	/* Search for an exact match first */
> +	efiobj = find_obj(dp, false, NULL);
> 
> +	/* Then for a fuzzy match */
> +	if (!efiobj)
> +		efiobj = find_obj(dp, false, rem);
> +
> +	/* And now for a fuzzy short match */
>   	if (!efiobj)
>   		efiobj = find_obj(dp, true, rem);
> 

Unfortunately after your patch some problems with device paths still remain:

I applied your patch after my patch series. I ran qemu-x86_defconfig 
with an IDE disk. To display the device paths I used 'bootefi selftest'. 
With and without your patch I saw a device path

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/HD(1,MBR,0x986b6db1,0x800,0x3f800)

Between VenHW and HD the device path node for the IDE disk is missing.

We should have something like

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Ata(0,Master,0)/HD(1,MBR,0x986b6db1,0x800,0x3f800)

iPXE reports an error because the parent of the partition is not a block 
device.

We should add to efi_selftest a test requiring:
The parent of a partition HD(* and the partition itself must expose the 
EFI_BLOCK_IO_PROTOCOL.
Cf. UEFI Spec 10.4.5 Media Device Path Rules
The root node should not expose the EFI_BLOCK_IO_PROTOCOL.

We should further add a test requiring:
A partition HD(* with non-zero partition number must have a positive 
offset and size.

Best regards

Heinrich
Heinrich Schuchardt Dec. 8, 2017, 7:09 a.m. UTC | #10
On 12/07/2017 08:00 AM, Jonathan Gray wrote:
> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
>> to introduce the el torito scheme to all partition table types: Spawn
>> individual disk objects for each partition on a disk.
>>
>> Unfortunately, that code ended up creating partitions with offset=0 which meant
>> that anyone accessing these objects gets data from the raw block device instead
>> of the partition.

Hello Jonathan,

according to the UEFI standard the whole disk may be represented by as 
partition number 0 (with offset 0).

UEFI spec 2.7:
10.3.6.1 Hard Drive
A partition number of zero can be used to represent the raw hard drive 
or a raw extended partition.

Do you have access to the source of the BSD loader that you are calling?
Please, check that it complies to this aspect of the UEFI API.

Best regards

Heinrich


>>
>> Furthermore, all the el torito logic to spawn devices for partitions was
>> duplicated. So let's merge the two code paths and give partition disk objects
>> good offsets to work from, so that payloads can actually make use of them.
>>
>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> node with the loaded image protocol on rpi_3 with mmc/usb.
> 
> broken
> ## Starting EFI application at 01000000 ...
> Scanning disk sdhci@7e300000.blk...
> efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
> efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1
> efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4
> Scanning disk usb_mass_storage.lun0...
> efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
> efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1
> efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4
> Found 6 disks
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_diskprobe
> efi_device_path_depth looking for type 4 i=0 type 1
> efi_device_path_depth looking for type 4 i=1 type 3
> efi_device_path_depth looking for type 4 i=2 type 3
> efi_device_path_depth looking for type 4 i=3 type 3
> efi_device_path_depth no match for type 4
> depth=-1
> 
> working (this commit reverted)
> ## Starting EFI application at 01000000 ...
> Scanning disk sdhci@7e300000.blk...
> efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=1
> efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=4
> efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
> Scanning disk usb_mass_storage.lun0...
> efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=1
> efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=4
> efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
> Found 2 disks
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 20 type 1 b: len 20 type: 1
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 11 type 3 b: len 11 type: 3
> efi_dp_match a: len 42 type 4 b: len 42 type: 4
> efi_diskprobe
> efi_device_path_depth looking for type 4 i=0 type 1
> efi_device_path_depth looking for type 4 i=1 type 3
> efi_device_path_depth looking for type 4 i=2 type 3
> efi_device_path_depth looking for type 4 i=3 type 3
> efi_device_path_depth looking for type 4 i=4 type 4
> depth=4
> 
>> ---
>>   lib/efi_loader/efi_disk.c | 60 ++++++++++-------------------------------------
>>   1 file changed, 13 insertions(+), 47 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> index 68ba2cf7b2..4e457a841b 100644
>> --- a/lib/efi_loader/efi_disk.c
>> +++ b/lib/efi_loader/efi_disk.c
>> @@ -264,21 +264,17 @@ out_of_memory:
>>   	printf("ERROR: Out of memory\n");
>>   }
>>   
>> -static int efi_disk_create_eltorito(struct blk_desc *desc,
>> -				    const char *if_typename,
>> -				    int diskid,
>> -				    const char *pdevname)
>> +static int efi_disk_create_partitions(struct blk_desc *desc,
>> +				      const char *if_typename,
>> +				      int diskid,
>> +				      const char *pdevname)
>>   {
>>   	int disks = 0;
>> -#if CONFIG_IS_ENABLED(ISO_PARTITION)
>>   	char devname[32] = { 0 }; /* dp->str is u16[32] long */
>>   	disk_partition_t info;
>>   	int part;
>>   
>> -	if (desc->part_type != PART_TYPE_ISO)
>> -		return 0;
>> -
>> -	/* and devices for each partition: */
>> +	/* Add devices for each partition */
>>   	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>>   		if (part_get_info(desc, part, &info))
>>   			continue;
>> @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc,
>>   		disks++;
>>   	}
>>   
>> -	/* ... and add block device: */
>> -	efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0);
>> -#endif
>> -
>>   	return disks;
>>   }
>>   
>> @@ -318,31 +310,18 @@ int efi_disk_register(void)
>>   	     uclass_next_device_check(&dev)) {
>>   		struct blk_desc *desc = dev_get_uclass_platdata(dev);
>>   		const char *if_typename = dev->driver->name;
>> -		disk_partition_t info;
>> -		int part;
>>   
>>   		printf("Scanning disk %s...\n", dev->name);
>>   
>> -		/* add devices for each partition: */
>> -		for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>> -			if (part_get_info(desc, part, &info))
>> -				continue;
>> -			efi_disk_add_dev(dev->name, if_typename, desc,
>> -					 desc->devnum, 0, part);
>> -		}
>> -
>> -		/* ... and add block device: */
>> +		/* Add block device for the full device */
>>   		efi_disk_add_dev(dev->name, if_typename, desc,
>>   				 desc->devnum, 0, 0);
>>   
>>   		disks++;
>>   
>> -		/*
>> -		* El Torito images show up as block devices in an EFI world,
>> -		* so let's create them here
>> -		*/
>> -		disks += efi_disk_create_eltorito(desc, if_typename,
>> -						  desc->devnum, dev->name);
>> +		/* Partitions show up as block devices in EFI */
>> +		disks += efi_disk_create_partitions(desc, if_typename,
>> +						    desc->devnum, dev->name);
>>   	}
>>   #else
>>   	int i, if_type;
>> @@ -361,8 +340,6 @@ int efi_disk_register(void)
>>   		for (i = 0; i < 4; i++) {
>>   			struct blk_desc *desc;
>>   			char devname[32] = { 0 }; /* dp->str is u16[32] long */
>> -			disk_partition_t info;
>> -			int part;
>>   
>>   			desc = blk_get_devnum_by_type(if_type, i);
>>   			if (!desc)
>> @@ -373,24 +350,13 @@ int efi_disk_register(void)
>>   			snprintf(devname, sizeof(devname), "%s%d",
>>   				 if_typename, i);
>>   
>> -			/* add devices for each partition: */
>> -			for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>> -				if (part_get_info(desc, part, &info))
>> -					continue;
>> -				efi_disk_add_dev(devname, if_typename, desc,
>> -						 i, 0, part);
>> -			}
>> -
>> -			/* ... and add block device: */
>> +			/* Add block device for the full device */
>>   			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
>>   			disks++;
>>   
>> -			/*
>> -			 * El Torito images show up as block devices
>> -			 * in an EFI world, so let's create them here
>> -			 */
>> -			disks += efi_disk_create_eltorito(desc, if_typename,
>> -							  i, devname);
>> +			/* Partitions show up as block devices in EFI */
>> +			disks += efi_disk_create_partitions(desc, if_typename,
>> +							    i, devname);
>>   		}
>>   	}
>>   #endif
>> -- 
>> 2.12.3
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>
Jonathan Gray Dec. 8, 2017, 7:27 a.m. UTC | #11
On Fri, Dec 08, 2017 at 08:09:46AM +0100, Heinrich Schuchardt wrote:
> On 12/07/2017 08:00 AM, Jonathan Gray wrote:
> > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
> > > Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> > > to introduce the el torito scheme to all partition table types: Spawn
> > > individual disk objects for each partition on a disk.
> > > 
> > > Unfortunately, that code ended up creating partitions with offset=0 which meant
> > > that anyone accessing these objects gets data from the raw block device instead
> > > of the partition.
> 
> Hello Jonathan,
> 
> according to the UEFI standard the whole disk may be represented by as
> partition number 0 (with offset 0).

You are quoting Alexander there not me.

> 
> UEFI spec 2.7:
> 10.3.6.1 Hard Drive
> A partition number of zero can be used to represent the raw hard drive or a
> raw extended partition.
> 
> Do you have access to the source of the BSD loader that you are calling?
> Please, check that it complies to this aspect of the UEFI API.

Already mentioned in this thread, but again:
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/

Which works fine with the EDK2 based firmware on the overdrive 1000.
And has found various regressions in changes made to the efi loader in
U-Boot over the last few months.

> 
> Best regards
> 
> Heinrich
> 
> 
> > > 
> > > Furthermore, all the el torito logic to spawn devices for partitions was
> > > duplicated. So let's merge the two code paths and give partition disk objects
> > > good offsets to work from, so that payloads can actually make use of them.
> > > 
> > > Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> > > Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> > > Signed-off-by: Alexander Graf <agraf@suse.de>
> > 
> > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> > node with the loaded image protocol on rpi_3 with mmc/usb.
> > 
> > broken
> > ## Starting EFI application at 01000000 ...
> > Scanning disk sdhci@7e300000.blk...
> > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
> > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1
> > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4
> > Scanning disk usb_mass_storage.lun0...
> > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
> > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1
> > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4
> > Found 6 disks
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 11 type 3 b: len 11 type: 3
> > efi_dp_match a: len 11 type 3 b: len 11 type: 3
> > efi_dp_match a: len 11 type 3 b: len 11 type: 3
> > efi_diskprobe
> > efi_device_path_depth looking for type 4 i=0 type 1
> > efi_device_path_depth looking for type 4 i=1 type 3
> > efi_device_path_depth looking for type 4 i=2 type 3
> > efi_device_path_depth looking for type 4 i=3 type 3
> > efi_device_path_depth no match for type 4
> > depth=-1
> > 
> > working (this commit reverted)
> > ## Starting EFI application at 01000000 ...
> > Scanning disk sdhci@7e300000.blk...
> > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=1
> > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=4
> > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0
> > Scanning disk usb_mass_storage.lun0...
> > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=1
> > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=4
> > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0
> > Found 2 disks
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 20 type 1 b: len 20 type: 1
> > efi_dp_match a: len 11 type 3 b: len 11 type: 3
> > efi_dp_match a: len 11 type 3 b: len 11 type: 3
> > efi_dp_match a: len 11 type 3 b: len 11 type: 3
> > efi_dp_match a: len 42 type 4 b: len 42 type: 4
> > efi_diskprobe
> > efi_device_path_depth looking for type 4 i=0 type 1
> > efi_device_path_depth looking for type 4 i=1 type 3
> > efi_device_path_depth looking for type 4 i=2 type 3
> > efi_device_path_depth looking for type 4 i=3 type 3
> > efi_device_path_depth looking for type 4 i=4 type 4
> > depth=4
> > 
> > > ---
> > >   lib/efi_loader/efi_disk.c | 60 ++++++++++-------------------------------------
> > >   1 file changed, 13 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > index 68ba2cf7b2..4e457a841b 100644
> > > --- a/lib/efi_loader/efi_disk.c
> > > +++ b/lib/efi_loader/efi_disk.c
> > > @@ -264,21 +264,17 @@ out_of_memory:
> > >   	printf("ERROR: Out of memory\n");
> > >   }
> > > -static int efi_disk_create_eltorito(struct blk_desc *desc,
> > > -				    const char *if_typename,
> > > -				    int diskid,
> > > -				    const char *pdevname)
> > > +static int efi_disk_create_partitions(struct blk_desc *desc,
> > > +				      const char *if_typename,
> > > +				      int diskid,
> > > +				      const char *pdevname)
> > >   {
> > >   	int disks = 0;
> > > -#if CONFIG_IS_ENABLED(ISO_PARTITION)
> > >   	char devname[32] = { 0 }; /* dp->str is u16[32] long */
> > >   	disk_partition_t info;
> > >   	int part;
> > > -	if (desc->part_type != PART_TYPE_ISO)
> > > -		return 0;
> > > -
> > > -	/* and devices for each partition: */
> > > +	/* Add devices for each partition */
> > >   	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > >   		if (part_get_info(desc, part, &info))
> > >   			continue;
> > > @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc,
> > >   		disks++;
> > >   	}
> > > -	/* ... and add block device: */
> > > -	efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0);
> > > -#endif
> > > -
> > >   	return disks;
> > >   }
> > > @@ -318,31 +310,18 @@ int efi_disk_register(void)
> > >   	     uclass_next_device_check(&dev)) {
> > >   		struct blk_desc *desc = dev_get_uclass_platdata(dev);
> > >   		const char *if_typename = dev->driver->name;
> > > -		disk_partition_t info;
> > > -		int part;
> > >   		printf("Scanning disk %s...\n", dev->name);
> > > -		/* add devices for each partition: */
> > > -		for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > > -			if (part_get_info(desc, part, &info))
> > > -				continue;
> > > -			efi_disk_add_dev(dev->name, if_typename, desc,
> > > -					 desc->devnum, 0, part);
> > > -		}
> > > -
> > > -		/* ... and add block device: */
> > > +		/* Add block device for the full device */
> > >   		efi_disk_add_dev(dev->name, if_typename, desc,
> > >   				 desc->devnum, 0, 0);
> > >   		disks++;
> > > -		/*
> > > -		* El Torito images show up as block devices in an EFI world,
> > > -		* so let's create them here
> > > -		*/
> > > -		disks += efi_disk_create_eltorito(desc, if_typename,
> > > -						  desc->devnum, dev->name);
> > > +		/* Partitions show up as block devices in EFI */
> > > +		disks += efi_disk_create_partitions(desc, if_typename,
> > > +						    desc->devnum, dev->name);
> > >   	}
> > >   #else
> > >   	int i, if_type;
> > > @@ -361,8 +340,6 @@ int efi_disk_register(void)
> > >   		for (i = 0; i < 4; i++) {
> > >   			struct blk_desc *desc;
> > >   			char devname[32] = { 0 }; /* dp->str is u16[32] long */
> > > -			disk_partition_t info;
> > > -			int part;
> > >   			desc = blk_get_devnum_by_type(if_type, i);
> > >   			if (!desc)
> > > @@ -373,24 +350,13 @@ int efi_disk_register(void)
> > >   			snprintf(devname, sizeof(devname), "%s%d",
> > >   				 if_typename, i);
> > > -			/* add devices for each partition: */
> > > -			for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > > -				if (part_get_info(desc, part, &info))
> > > -					continue;
> > > -				efi_disk_add_dev(devname, if_typename, desc,
> > > -						 i, 0, part);
> > > -			}
> > > -
> > > -			/* ... and add block device: */
> > > +			/* Add block device for the full device */
> > >   			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
> > >   			disks++;
> > > -			/*
> > > -			 * El Torito images show up as block devices
> > > -			 * in an EFI world, so let's create them here
> > > -			 */
> > > -			disks += efi_disk_create_eltorito(desc, if_typename,
> > > -							  i, devname);
> > > +			/* Partitions show up as block devices in EFI */
> > > +			disks += efi_disk_create_partitions(desc, if_typename,
> > > +							    i, devname);
> > >   		}
> > >   	}
> > >   #endif
> > > -- 
> > > 2.12.3
> > > 
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot@lists.denx.de
> > > https://lists.denx.de/listinfo/u-boot
> > 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Mark Kettenis Dec. 10, 2017, 11:46 a.m. UTC | #12
> From: Alexander Graf <agraf@suse.de>
> Date: Thu, 7 Dec 2017 11:27:15 +0100
> 
> On 07.12.17 08:00, Jonathan Gray wrote:
> > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
> >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> >> to introduce the el torito scheme to all partition table types: Spawn
> >> individual disk objects for each partition on a disk.
> >>
> >> Unfortunately, that code ended up creating partitions with offset=0 which meant
> >> that anyone accessing these objects gets data from the raw block device instead
> >> of the partition.
> >>
> >> Furthermore, all the el torito logic to spawn devices for partitions was
> >> duplicated. So let's merge the two code paths and give partition disk objects
> >> good offsets to work from, so that payloads can actually make use of them.
> >>
> >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> > 
> > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> > node with the loaded image protocol on rpi_3 with mmc/usb.
> 
> Hooray :). Do you think you could somehow assemble a test that runs
> inside Travis-CI to make sure we catch your loader? I still need to do a
> better one for grub to ensure that that one's happy too.

Yes, I'd very much like to do that.  I suppose this should be a
qemu-based test in test/py/tests?  It is not clear to me how I run
those outside of Travis-CI.  Can you point me in the right direction?
Jonathan Gray Feb. 1, 2018, 6:39 a.m. UTC | #13
On Fri, Dec 08, 2017 at 06:55:02AM +0100, Alexander Graf wrote:
> 
> 
> On 07.12.17 12:45, Jonathan Gray wrote:
> > On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote:
> >> On 12/07/2017 08:00 AM, Jonathan Gray wrote:
> >>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote:
> >>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried
> >>>> to introduce the el torito scheme to all partition table types: Spawn
> >>>> individual disk objects for each partition on a disk.
> >>>>
> >>>> Unfortunately, that code ended up creating partitions with offset=0 which meant
> >>>> that anyone accessing these objects gets data from the raw block device instead
> >>>> of the partition.
> >>>>
> >>>> Furthermore, all the el torito logic to spawn devices for partitions was
> >>>> duplicated. So let's merge the two code paths and give partition disk objects
> >>>> good offsets to work from, so that payloads can actually make use of them.
> >>>>
> >>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions)
> >>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com>
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>
> >>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE
> >>> node with the loaded image protocol on rpi_3 with mmc/usb.
> >>
> >> Could you, please, specify which software you are trying to call:
> >> Linux EFI stub, Grub, or anything else?
> > 
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI
> > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/
> > Disk image with fat/ffs filesystems
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs
> > 
> > though it would likely show up on other archs as well
> > 
> > armv7 equivalents of the above
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI
> > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/
> > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs
> > 
> >>
> >> Which patches did you consider?
> >> Did you apply these patch series that are not yet in efi-next?
> >> efi_loader: correct media device paths
> >> efi_loader: avoid use after free
> > 
> > just master
> > c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path
> > 
> > with a small patch to force calling gnu sed for some non-portable
> > sed use in check-config.sh
> > 
> > 'efi_loader: avoid use after free' doesn't help
> > 'efi_loader: correct media device paths' doesn't either
> 
> As a quick heads-up: The device path matching is broken. The patch below
> should fix it, but I want to create a travis-ci case around that first
> and also wrap it up more nicely.
> 
> Alex

With the latest master at the time of writing
(2e87980580d0bf4781ad0d63efd456aa1a73d03f) the rpi_3 target again
seems to be broken while it worked with the v2018.01 release.

The qemu_arm64 target however works with the latest master.

U-Boot 2018.03-rc1-00058-g36d3bd9514 (Feb 01 2018 - 17:04:29 +1100)

DRAM:  948 MiB
RPI 3 Model B (0xa02082)
MMC:   mmc@7e202000: 0, sdhci@7e300000: 1
Loading Environment from FAT... OK
In:    serial
Out:   vidconsole
Err:   vidconsole
Net:   No ethernet found.
starting USB...
USB0:	Core Release: 2.80a
scanning bus 0 for devices... 4 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found
Hit any key to stop autoboot:  0

Device 0: Vendor: SanDisk Rev: 1.00 Prod: Ultra
	    Type: Removable Hard Disk
	    Capacity: 29327.3 MB = 28.6 GB (60062500 x 512)
... is now current device
Scanning usb 0:1...
Found EFI removable media binary efi/boot/bootaa64.efi
78335 bytes read in 79 ms (967.8 KiB/s)
## Starting EFI application at 01000000 ...
Scanning disk mmc@7e202000.blk...
Card did not respond to voltage select!
mmc_init: -95, time 24
Scanning disk sdhci@7e300000.blk...
>> OpenBSD/arm64 BOOTAA64 0.8
boot>
cannot open sd0a:/etc/random.seed: Device not configured
booting sd0a:/bsd: open sd0a:/bsd: Device not configured
 failed(6). will try /bsd
boot>
cannot open sd0a:/etc/random.seed: Device not configured
booting sd0a:/bsd: open sd0a:/bsd: Device not configured
 failed(6). will try /bsd
Turning timeout off.

U-Boot 2018.01 (Jan 09 2018 - 16:25:44 +1100)

DRAM:  948 MiB
RPI 3 Model B (0xa02082)
MMC:   sdhci@7e300000: 0
reading uboot.env
In:    serial
Out:   vidconsole
Err:   vidconsole
Net:   No ethernet found.
starting USB...
USB0:	Core Release: 2.80a
scanning bus 0 for devices... 4 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found
Hit any key to stop autoboot:  0

Device 0: Vendor: SanDisk Rev: 1.00 Prod: Ultra
	    Type: Removable Hard Disk
	    Capacity: 29327.3 MB = 28.6 GB (60062500 x 512)
... is now current device
Scanning usb 0:1...
Found EFI removable media binary efi/boot/bootaa64.efi
reading efi/boot/bootaa64.efi
78335 bytes read in 81 ms (944.3 KiB/s)
## Starting EFI application at 01000000 ...
Scanning disk sdhci@7e300000.blk...
Scanning disk usb_mass_storage.lun0...
Found 6 disks
>> OpenBSD/arm64 BOOTAA64 0.8
boot>
booting sd0a:/bsd: 3890560+575675+581952+806704|[276788+96+457320+243052]=0x841d20
...

U-Boot 2018.03-rc1-00058-g36d3bd9514 (Feb 01 2018 - 17:05:55 +1100)

DRAM:  2 GiB
In:    pl011@9000000
Out:   pl011@9000000
Err:   pl011@9000000
Net:   No ethernet found.
Hit any key to stop autoboot:  0
scanning bus for devices...
Target spinup took 0 ms.
SATA link 1 timeout.
SATA link 2 timeout.
SATA link 3 timeout.
SATA link 4 timeout.
SATA link 5 timeout.
AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode
flags: 64bit ncq only
  Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
	    Type: Hard Disk
	    Capacity: 2048.0 MB = 2.0 GB (4194304 x 512)

Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
	    Type: Hard Disk
	    Capacity: 2048.0 MB = 2.0 GB (4194304 x 512)
... is now current device
Scanning scsi 0:1...
load - load binary file from a filesystem

Usage:
load <interface> [<dev[:part]> [<addr> [<filename> [bytes [pos]]]]]
    - Load binary file 'filename' from partition 'part' on device
       type 'interface' instance 'dev' to address 'addr' in memory.
      'bytes' gives the size to load in bytes.
      If 'bytes' is 0 or omitted, the file is read until the end.
      'pos' gives the file byte position to start reading from.
      If 'pos' is 0 or omitted, the file is read from the start.
Found EFI removable media binary efi/boot/bootaa64.efi
Scanning disk ahci_scsi.id0lun0...
Found 3 disks
78335 bytes read in 9 ms (8.3 MiB/s)
## Starting EFI application at 40400000 ...
>> OpenBSD/arm64 BOOTAA64 0.8
boot>
booting sd0a:/bsd: 3955580+580404+584072+804720|[278621+96+459048+243979]=0x842f30
...
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 68ba2cf7b2..4e457a841b 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -264,21 +264,17 @@  out_of_memory:
 	printf("ERROR: Out of memory\n");
 }
 
-static int efi_disk_create_eltorito(struct blk_desc *desc,
-				    const char *if_typename,
-				    int diskid,
-				    const char *pdevname)
+static int efi_disk_create_partitions(struct blk_desc *desc,
+				      const char *if_typename,
+				      int diskid,
+				      const char *pdevname)
 {
 	int disks = 0;
-#if CONFIG_IS_ENABLED(ISO_PARTITION)
 	char devname[32] = { 0 }; /* dp->str is u16[32] long */
 	disk_partition_t info;
 	int part;
 
-	if (desc->part_type != PART_TYPE_ISO)
-		return 0;
-
-	/* and devices for each partition: */
+	/* Add devices for each partition */
 	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
 		if (part_get_info(desc, part, &info))
 			continue;
@@ -289,10 +285,6 @@  static int efi_disk_create_eltorito(struct blk_desc *desc,
 		disks++;
 	}
 
-	/* ... and add block device: */
-	efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0);
-#endif
-
 	return disks;
 }
 
@@ -318,31 +310,18 @@  int efi_disk_register(void)
 	     uclass_next_device_check(&dev)) {
 		struct blk_desc *desc = dev_get_uclass_platdata(dev);
 		const char *if_typename = dev->driver->name;
-		disk_partition_t info;
-		int part;
 
 		printf("Scanning disk %s...\n", dev->name);
 
-		/* add devices for each partition: */
-		for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
-			if (part_get_info(desc, part, &info))
-				continue;
-			efi_disk_add_dev(dev->name, if_typename, desc,
-					 desc->devnum, 0, part);
-		}
-
-		/* ... and add block device: */
+		/* Add block device for the full device */
 		efi_disk_add_dev(dev->name, if_typename, desc,
 				 desc->devnum, 0, 0);
 
 		disks++;
 
-		/*
-		* El Torito images show up as block devices in an EFI world,
-		* so let's create them here
-		*/
-		disks += efi_disk_create_eltorito(desc, if_typename,
-						  desc->devnum, dev->name);
+		/* Partitions show up as block devices in EFI */
+		disks += efi_disk_create_partitions(desc, if_typename,
+						    desc->devnum, dev->name);
 	}
 #else
 	int i, if_type;
@@ -361,8 +340,6 @@  int efi_disk_register(void)
 		for (i = 0; i < 4; i++) {
 			struct blk_desc *desc;
 			char devname[32] = { 0 }; /* dp->str is u16[32] long */
-			disk_partition_t info;
-			int part;
 
 			desc = blk_get_devnum_by_type(if_type, i);
 			if (!desc)
@@ -373,24 +350,13 @@  int efi_disk_register(void)
 			snprintf(devname, sizeof(devname), "%s%d",
 				 if_typename, i);
 
-			/* add devices for each partition: */
-			for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
-				if (part_get_info(desc, part, &info))
-					continue;
-				efi_disk_add_dev(devname, if_typename, desc,
-						 i, 0, part);
-			}
-
-			/* ... and add block device: */
+			/* Add block device for the full device */
 			efi_disk_add_dev(devname, if_typename, desc, i, 0, 0);
 			disks++;
 
-			/*
-			 * El Torito images show up as block devices
-			 * in an EFI world, so let's create them here
-			 */
-			disks += efi_disk_create_eltorito(desc, if_typename,
-							  i, devname);
+			/* Partitions show up as block devices in EFI */
+			disks += efi_disk_create_partitions(desc, if_typename,
+							    i, devname);
 		}
 	}
 #endif