[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 at 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 */