[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Jan 30, 2019 at 07:49:37AM +0100, Heinrich Schuchardt wrote:
> On 1/30/19 6:48 AM, AKASHI Takahiro wrote:
> > On Tue, Jan 29, 2019 at 11:33:31PM +0100, Heinrich Schuchardt wrote:
> >> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> >>> efi_disk_create() will initialize efi_disk attributes for each device,
> >>> either UCLASS_BLK or UCLASS_PARTITION.
> >>>
> >>> Currently (temporarily), efi_disk_obj structure is embedded into
> >>> blk_desc to hold efi-specific attributes.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  drivers/block/blk-uclass.c |   9 ++
> >>>  drivers/core/device.c      |   3 +
> >>>  include/blk.h              |  24 +++++
> >>>  include/dm/device.h        |   4 +
> >>>  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> >>>  5 files changed, 174 insertions(+), 58 deletions(-)
> >>>
> >>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> >>> index d4ca30f23fc1..28c45d724113 100644
> >>> --- a/drivers/block/blk-uclass.c
> >>> +++ b/drivers/block/blk-uclass.c
> >>> @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
> >>>  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> >>>  };
> >>>  
> >>> +/* FIXME */
> >>> +extern int efi_disk_create(struct udevice *dev);
> >>> +
> >>>  U_BOOT_DRIVER(blk_partition) = {
> >>>  	.name		= "blk_partition",
> >>>  	.id		= UCLASS_PARTITION,
> >>> @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
> >>>  		part_data->partnum = part;
> >>>  		part_data->gpt_part_info = info;
> >>>  
> >>> +		ret = efi_disk_create(dev);
> >>> +		if (ret) {
> >>> +			device_unbind(dev);
> >>> +			return ret;
> >>> +		}
> >>> +
> >>>  		disks++;
> >>>  	}
> >>>  
> >>> diff --git a/drivers/core/device.c b/drivers/core/device.c
> >>> index 0d15e5062b66..8625fccb0dcb 100644
> >>> --- a/drivers/core/device.c
> >>> +++ b/drivers/core/device.c
> >>> @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv,
> >>>  	dev->parent = parent;
> >>>  	dev->driver = drv;
> >>>  	dev->uclass = uc;
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +	INIT_LIST_HEAD(&dev->efi_obj.protocols);
> >>
> >> This looks like an incomplete initialization of efi_obj. Why don't we
> >> use efi_create_handle.
> > 
> > I think that it is more or less a matter of implementation.
> > I will address this issue later if necessary.
> > 
> >> Why should a device be aware of struct efi_obj? We only need a handle to
> >> install protocols.
> >>
> >>> +#endif
> >>>  
> >>>  	dev->seq = -1;
> >>>  	dev->req_seq = -1;
> >>> diff --git a/include/blk.h b/include/blk.h
> >>> index d0c033aece0f..405f6f790d66 100644
> >>> --- a/include/blk.h
> >>> +++ b/include/blk.h
> >>> @@ -8,6 +8,7 @@
> >>>  #define BLK_H
> >>>  
> >>>  #include <efi.h>
> >>> +#include <efi_api.h>
> >>>  
> >>>  #ifdef CONFIG_SYS_64BIT_LBA
> >>>  typedef uint64_t lbaint_t;
> >>> @@ -53,6 +54,26 @@ enum sig_type {
> >>>  	SIG_TYPE_COUNT			/* Number of signature types */
> >>>  };
> >>>  
> >>> +/* FIXME */
> >>
> >> Fix what?
> > 
> > For simplicity, this data structure was copied from efi_disk.c
> > in this initial release.
> > As the implementation goes *sophisticated*, some members may go away
> > or move somewhere, say in blk_desc itself.
> > 
> >>> +/**
> >>> + * struct efi_disk_obj - EFI disk object
> >>> + *
> >>> + * @ops:	EFI disk I/O protocol interface
> >>> + * @media:	block I/O media information
> >>> + * @dp:		device path to the block device
> >>> + * @part:	partition
> >>> + * @volume:	simple file system protocol of the partition
> >>> + * @offset:	offset into disk for simple partition
> >>> + */
> >>> +struct efi_disk_obj {
> >>> +	struct efi_block_io ops;
> >>> +	struct efi_block_io_media media;
> >>> +	struct efi_device_path *dp;
> >>> +	unsigned int part;
> >>> +	struct efi_simple_file_system_protocol *volume;
> >>> +	lbaint_t offset;
> >>> +};
> >>> +
> >>>  /*
> >>>   * With driver model (CONFIG_BLK) this is uclass platform data, accessible
> >>>   * with dev_get_uclass_platdata(dev)
> >>> @@ -92,6 +113,9 @@ struct blk_desc {
> >>>  	 * device. Once these functions are removed we can drop this field.
> >>>  	 */
> >>>  	struct udevice *bdev;
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +	struct efi_disk_obj efi_disk;
> >>
> >> This must be a handle (i.e. a pointer). Otherwise when the last protocol
> >> is removed we try to free memory that was never malloc'ed.
> > 
> > Who actually frees?
> 
> see UEFI spec for UninstallProtocolInterface():
> 
> "If the last protocol interface is removed from a handle, the handle is
> freed and is no longer valid."

Got it.
So, under the current implementation, any efi_object must be allocated
by efi code itself and all derived efi objects have an efi_object
as the first member.
(We can lift this restriction by adding object-specific "free" function,
like calling (handle->free)(handle) instead of free(handle) though.)

Umm. This will make it a bit difficult to remove efi_disk_obj from
our code.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >>> +#endif
> >>>  #else
> >>>  	unsigned long	(*block_read)(struct blk_desc *block_dev,
> >>>  				      lbaint_t start,
> >>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>> index 27a6d7b9fdb0..121bfb46b1a0 100644
> >>> --- a/include/dm/device.h
> >>> +++ b/include/dm/device.h
> >>> @@ -12,6 +12,7 @@
> >>>  
> >>>  #include <dm/ofnode.h>
> >>>  #include <dm/uclass-id.h>
> >>> +#include <efi_loader.h>
> >>>  #include <fdtdec.h>
> >>>  #include <linker_lists.h>
> >>>  #include <linux/compat.h>
> >>> @@ -146,6 +147,9 @@ struct udevice {
> >>>  #ifdef CONFIG_DEVRES
> >>>  	struct list_head devres_head;
> >>>  #endif
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +	struct efi_object efi_obj;
> >>> +#endif
> >>>  };
> >>>  
> >>>  /* Maximum sequence number supported */
> >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>> index c037526ad2d0..84fa0ddfba14 100644
> >>> --- a/lib/efi_loader/efi_disk.c
> >>> +++ b/lib/efi_loader/efi_disk.c
> >>> @@ -14,33 +14,6 @@
> >>>  
> >>>  const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
> >>>  
> >>> -/**
> >>> - * struct efi_disk_obj - EFI disk object
> >>> - *
> >>> - * @header:	EFI object header
> >>> - * @ops:	EFI disk I/O protocol interface
> >>> - * @ifname:	interface name for block device
> >>> - * @dev_index:	device index of block device
> >>> - * @media:	block I/O media information
> >>> - * @dp:		device path to the block device
> >>> - * @part:	partition
> >>> - * @volume:	simple file system protocol of the partition
> >>> - * @offset:	offset into disk for simple partition
> >>> - * @desc:	internal block device descriptor
> >>> - */
> >>> -struct efi_disk_obj {
> >>> -	struct efi_object header;
> >>> -	struct efi_block_io ops;
> >>> -	const char *ifname;
> >>> -	int dev_index;
> >>> -	struct efi_block_io_media media;
> >>> -	struct efi_device_path *dp;
> >>> -	unsigned int part;
> >>> -	struct efi_simple_file_system_protocol *volume;
> >>> -	lbaint_t offset;
> >>> -	struct blk_desc *desc;
> >>> -};
> >>> -
> >>>  static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
> >>>  			char extended_verification)
> >>>  {
> >>> @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
> >>>  	unsigned long n;
> >>>  
> >>>  	diskobj = container_of(this, struct efi_disk_obj, ops);
> >>> -	desc = (struct blk_desc *) diskobj->desc;
> >>> +	desc = container_of(diskobj, struct blk_desc, efi_disk);
> >>>  	blksz = desc->blksz;
> >>>  	blocks = buffer_size / blksz;
> >>>  	lba += diskobj->offset;
> >>> @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
> >>>  	return handler->protocol_interface;
> >>>  }
> >>>  
> >>> +#ifndef CONFIG_BLK
> >>>  /*
> >>>   * Create a handle for a partition or disk
> >>>   *
> >>> @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >>>  
> >>>  	return disks;
> >>>  }
> >>> +#else
> >>> +static int efi_disk_create_common(efi_handle_t handle,
> >>> +				  struct efi_disk_obj *disk,
> >>> +				  struct blk_desc *desc)
> >>> +{
> >>> +	efi_status_t ret;
> >>> +
> >>> +	/* Hook up to the device list */
> >>> +	efi_add_handle(handle);
> >>> +
> >>> +	/* Fill in EFI IO Media info (for read/write callbacks) */
> >>> +	disk->media.removable_media = desc->removable;
> >>> +	disk->media.media_present = 1;
> >>> +	disk->media.block_size = desc->blksz;
> >>> +	disk->media.io_align = desc->blksz;
> >>> +	disk->media.last_block = desc->lba - disk->offset;
> >>> +	disk->ops.media = &disk->media;
> >>> +
> >>> +	/* add protocols */
> >>> +	disk->ops = block_io_disk_template;
> >>> +	ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
> >>> +	if (ret != EFI_SUCCESS)
> >>> +		goto err;
> >>> +
> >>> +	ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
> >>> +	if (ret != EFI_SUCCESS)
> >>> +		goto err;
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	/* FIXME: undo adding protocols */
> >>> +	return -1;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Create a handle for a raw disk
> >>> + *
> >>> + * @dev		uclass device
> >>> + * @return	0 on success, -1 otherwise
> >>> + */
> >>> +int efi_disk_create_raw(struct udevice *dev)
> >>> +{
> >>> +	struct blk_desc *desc = dev_get_uclass_platdata(dev);
> >>> +	struct efi_disk_obj *disk = &desc->efi_disk;
> >>> +
> >>> +	/* Don't add empty devices */
> >>> +	if (!desc->lba)
> >>> +		return -1;
> >>> +
> >>> +	/* raw block device */
> >>> +	disk->offset = 0;
> >>> +	disk->part = 0;
> >>> +	disk->dp = efi_dp_from_part(desc, 0);
> >>> +
> >>> +	/* efi IO media */
> >>> +	disk->media.logical_partition = 0;
> >>> +
> >>> +	return efi_disk_create_common(&dev->efi_obj, disk, desc);
> >>> +}
> >>> +
> >>> +/*
> >>> + * Create a handle for a partition
> >>> + *
> >>> + * @dev		uclass device
> >>> + * @return	0 on success, -1 otherwise
> >>> + */
> >>> +int efi_disk_create_part(struct udevice *dev)
> >>> +{
> >>> +	struct udevice *parent = dev->parent;
> >>> +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
> >>> +	struct blk_desc *this;
> >>> +	struct disk_part *pdata = dev_get_uclass_platdata(dev);
> >>> +	struct efi_disk_obj *disk;
> >>> +	struct efi_device_path *node;
> >>> +
> >>> +	int ret;
> >>> +
> >>> +	/* dummy block device */
> >>> +	this = malloc(sizeof(*this));
> >>> +	if (!this)
> >>> +		return -1;
> >>> +	disk = &this->efi_disk;
> >>> +
> >>> +	/* logical disk partition */
> >>> +	disk->offset = pdata->gpt_part_info.start;
> >>> +	disk->part = pdata->partnum;
> >>> +
> >>> +	node = efi_dp_part_node(desc, disk->part);
> >>> +	disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
> >>> +	efi_free_pool(node);
> >>> +
> >>> +	/* efi IO media */
> >>> +	disk->media.logical_partition = 1;
> >>> +
> >>> +	ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
> >>> +	if (ret)
> >>> +		goto err;
> >>> +
> >>> +	/* partition may support file system access */
> >>> +	disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
> >>> +	ret = efi_add_protocol(&dev->efi_obj,
> >>> +			       &efi_simple_file_system_protocol_guid,
> >>> +			       disk->volume);
> >>> +	if (ret != EFI_SUCCESS)
> >>> +		goto err;
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	free(this);
> >>> +	/* FIXME: undo create */
> >>> +
> >>> +	return -1;
> >>> +}
> >>> +
> >>> +int efi_disk_create(struct udevice *dev)
> >>> +{
> >>> +	enum uclass_id id;
> >>> +
> >>> +	id = device_get_uclass_id(dev);
> >>> +
> >>> +	if (id == UCLASS_BLK)
> >>> +		return efi_disk_create_raw(dev);
> >>> +	else if (id == UCLASS_PARTITION)
> >>> +		return efi_disk_create_part(dev);
> >>> +	else
> >>> +		return -1;
> >>> +}
> >>> +#endif /* CONFIG_BLK */
> >>>  
> >>>  /*
> >>>   * U-Boot doesn't have a list of all online disk devices. So when running our
> >>> @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >>>   */
> >>>  efi_status_t efi_disk_register(void)
> >>>  {
> >>> +#ifndef CONFIG_BLK
> >>>  	struct efi_disk_obj *disk;
> >>>  	int disks = 0;
> >>>  	efi_status_t ret;
> >>> -#ifdef CONFIG_BLK
> >>> -	struct udevice *dev;
> >>> -
> >>> -	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
> >>> -	     uclass_next_device_check(&dev)) {
> >>> -		struct blk_desc *desc = dev_get_uclass_platdata(dev);
> >>> -		const char *if_typename = blk_get_if_type_name(desc->if_type);
> >>> -
> >>> -		/* Add block device for the full device */
> >>> -		printf("Scanning disk %s...\n", dev->name);
> >>
> >> Who cares for this output? If really needed make it debug().
> > 
> > Please note that this is a line to be deleted.
> > 
> >>> -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> >>> -					desc, desc->devnum, 0, 0, &disk);
> >>> -		if (ret == EFI_NOT_READY) {
> >>> -			printf("Disk %s not ready\n", dev->name);
> >>
> >> Who minds if it is a CD-ROM drive with no disk inserted? Debug() might
> >> be adequate here.
> > 
> > Ditto
> > 
> >>> -			continue;
> >>> -		}
> >>> -		if (ret) {
> >>> -			printf("ERROR: failure to add disk device %s, r = %lu\n",
> >>> -			       dev->name, ret & ~EFI_ERROR_MASK);
> >>> -			return ret;
> >>> -		}
> >>> -		disks++;
> >>> -
> >>> -		/* Partitions show up as block devices in EFI */
> >>> -		disks += efi_disk_create_partitions(
> >>> -					&disk->header, desc, if_typename,
> >>> -					desc->devnum, dev->name);
> >>> -	}
> >>> -#else
> >>>  	int i, if_type;
> >>>  
> >>>  	/* Search for all available disk devices */
> >>> @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
> >>>  						 if_typename, i, devname);
> >>>  		}
> >>>  	}
> >>> -#endif
> >>>  	printf("Found %d disks\n", disks);
> >>
> >> I would prefer this to be debug() or eliminated.
> > 
> > I didn't change anything on this line and the function, efi_disk_register(),
> > will eventually go away.
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +#endif
> >>>  
> >>>  	return EFI_SUCCESS;
> >>>  }
> >>>
> >>
> > 
>