From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 30 Jan 2019 16:26:29 +0900 Subject: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk In-Reply-To: <92d6a4e7-ee6d-e9ce-e734-024dc930fbac@gmx.de> References: <20190129025956.21870-1-takahiro.akashi@linaro.org> <20190129025956.21870-3-takahiro.akashi@linaro.org> <20190130054851.GY20286@linaro.org> <92d6a4e7-ee6d-e9ce-e734-024dc930fbac@gmx.de> Message-ID: <20190130072628.GZ20286@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >>> --- > >>> 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 > >>> +#include > >>> > >>> #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 > >>> #include > >>> +#include > >>> #include > >>> #include > >>> #include > >>> @@ -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; > >>> } > >>> > >> > > >