[U-Boot] [PATCH 1/2] efi_loader: export efi_locate_handle() function
This function will be used later to implement efi_disk_update().
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- include/efi_loader.h | 4 ++++ lib/efi_loader/efi_boottime.c | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index ce0f420b5004..5cc3bded03fa 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -318,6 +318,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle); void efi_delete_handle(efi_handle_t obj); /* Call this to validate a handle and find the EFI object for it */ struct efi_object *efi_search_obj(const efi_handle_t handle); +/* locate handles */ +efi_status_t efi_locate_handle(enum efi_locate_search_type search_type, + const efi_guid_t *protocol, void *search_key, + efi_uintn_t *buffer_size, efi_handle_t *buffer); /* Find a protocol on a handle */ efi_status_t efi_search_protocol(const efi_handle_t handle, const efi_guid_t *protocol_guid, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 8a43a5a84091..a3c56bbab552 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1309,10 +1309,9 @@ static int efi_search(enum efi_locate_search_type search_type, * * Return: status code */ -static efi_status_t efi_locate_handle( - enum efi_locate_search_type search_type, - const efi_guid_t *protocol, void *search_key, - efi_uintn_t *buffer_size, efi_handle_t *buffer) +efi_status_t efi_locate_handle(enum efi_locate_search_type search_type, + const efi_guid_t *protocol, void *search_key, + efi_uintn_t *buffer_size, efi_handle_t *buffer) { struct efi_object *efiobj; efi_uintn_t size = 0;
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 8 +- include/efi_loader.h | 2 + lib/efi_loader/efi_disk.c | 150 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..493022a09482 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -57,8 +57,14 @@ efi_status_t efi_init_obj_list(void) efi_save_gd();
/* Initialize once only */ - if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) + if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) { +#ifdef CONFIG_PARTITIONS + ret = efi_disk_update(); + if (ret != EFI_SUCCESS) + printf("+++ updating disks failed\n"); +#endif return efi_obj_list_initialized; + }
/* Initialize system table */ ret = efi_initialize_system_table(); diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..e5a080281dba 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,8 @@ efi_status_t efi_initialize_system_table(void); efi_status_t efi_console_register(void); /* Called by bootefi to make all disk storage accessible as EFI objects */ efi_status_t efi_disk_register(void); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..e1d47f34049b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,13 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_MARK_DELETE 0x1 + /** * struct efi_disk_obj - EFI disk object * * @header: EFI object header + * @flags: control flags * @ops: EFI disk I/O protocol interface * @ifname: interface name for block device * @dev_index: device index of block device @@ -30,6 +33,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header; + int flags; struct efi_block_io ops; const char *ifname; int dev_index; @@ -440,3 +444,149 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; } + +static void efi_disk_mark_delete(struct efi_disk_obj *disk) +{ + disk->flags |= _EFI_DISK_MARK_DELETE; +} + +static void efi_disk_mark_undelete(struct efi_disk_obj *disk) +{ + disk->flags &= ~_EFI_DISK_MARK_DELETE; +} + +static bool efi_disk_delete_marked(struct efi_disk_obj *disk) +{ + return disk->flags & _EFI_DISK_MARK_DELETE; +} + +static efi_status_t efi_disk_mark_delete_all(efi_handle_t **handlesp) +{ + efi_handle_t *handles = NULL; + efi_uintn_t size = 0; + int num, i; + struct efi_disk_obj *disk; + efi_status_t ret; + + ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL, + &size, handles); + if (ret == EFI_BUFFER_TOO_SMALL) { + handles = calloc(1, size); + if (!handles) + return EFI_OUT_OF_RESOURCES; + + ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL, + &size, handles); + } + if (ret != EFI_SUCCESS) { + free(handles); + return ret; + } + + num = size / sizeof(*handles); + for (i = 0; i < num; i++) { + disk = container_of(handles[i], struct efi_disk_obj, header); + efi_disk_mark_delete(disk); + } + + *handlesp = handles; + + return num; +} + +static int efi_disk_mark_undelete_handles(struct blk_desc *desc, + efi_handle_t *handles, int num) +{ + struct efi_disk_obj *disk; + int disks, i; + + for (i = 0, disks = 0; i < num; i++) { + disk = container_of(handles[i], struct efi_disk_obj, header); + + if (!desc || disk->desc == desc) { + efi_disk_mark_undelete(disk); + disks++; + } + } + + return disks; +} + +static efi_status_t efi_disk_delete_all(efi_handle_t *handles, int num) +{ + struct efi_disk_obj *disk; + int i; + + for (i = 0; i < num; i++) { + disk = container_of(handles[i], struct efi_disk_obj, header); + + if (!efi_disk_delete_marked(disk)) + continue; + + efi_delete_handle(handles[i]); + } + + return EFI_SUCCESS; +} + +efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK + efi_handle_t *handles = NULL; + struct udevice *dev; + struct blk_desc *desc; + const char *if_typename; + struct efi_disk_obj *disk; + int n, disks = 0; + efi_status_t ret; + + ret = efi_disk_mark_delete_all(&handles); + if (ret & EFI_ERROR_MASK) + return ret; + + n = (int)ret; + ret = EFI_SUCCESS; + for (uclass_first_device_check(UCLASS_BLK, &dev); dev; + uclass_next_device_check(&dev)) { + desc = dev_get_uclass_platdata(dev); + if (n && efi_disk_mark_undelete_handles(desc, handles, n)) + /* existing device */ + continue; + + /* new device */ + if_typename = blk_get_if_type_name(desc->if_type); + + /* Add block device for the full device */ + printf("Scanning disk %s...\n", dev->name); + 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); + continue; + } + if (ret) { + printf("ERROR: failure to add disk device %s, r = %lu\n", + dev->name, ret & ~EFI_ERROR_MASK); + break; + } + disks++; + + /* Partitions show up as block devices in EFI */ + disks += efi_disk_create_partitions(&disk->header, desc, + if_typename, + desc->devnum, dev->name); + } + + if (n) { + if (ret != EFI_SUCCESS) + efi_disk_mark_undelete_handles(NULL, handles, n); + else + ret = efi_disk_delete_all(handles, n); + free(handles); + } + + return ret; +#else + return EFI_SUCCESS; +#endif +}
On 11/7/18 1:44 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 8 +- include/efi_loader.h | 2 + lib/efi_loader/efi_disk.c | 150 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..493022a09482 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -57,8 +57,14 @@ efi_status_t efi_init_obj_list(void) efi_save_gd();
/* Initialize once only */
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) {
If efi_obj_list_initialized is neither OBJ_LIST_NOT_INITIALIZED nor EFI_SUCCESS, return efi_obj_list_initialized.
+#ifdef CONFIG_PARTITIONS
ret = efi_disk_update();if (ret != EFI_SUCCESS)printf("+++ updating disks failed\n");+#endif return efi_obj_list_initialized;
}
/* Initialize system table */ ret = efi_initialize_system_table();
diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..e5a080281dba 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,8 @@ efi_status_t efi_initialize_system_table(void); efi_status_t efi_console_register(void); /* Called by bootefi to make all disk storage accessible as EFI objects */ efi_status_t efi_disk_register(void); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..e1d47f34049b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,13 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_MARK_DELETE 0x1
Plese, add a comment to the code explaining what this constant is used for. None of our other constants starts with an underscore.
In your coding below you keep handles for deleted drives. What will happen if a binary tries to access a deleted drive?
/**
- struct efi_disk_obj - EFI disk object
- @header: EFI object header
- @flags: control flags
- @ops: EFI disk I/O protocol interface
- @ifname: interface name for block device
- @dev_index: device index of block device
@@ -30,6 +33,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header;
- int flags; struct efi_block_io ops; const char *ifname; int dev_index;
@@ -440,3 +444,149 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; }
+static void efi_disk_mark_delete(struct efi_disk_obj *disk)
efi_disk_mark_deleted.
+{
- disk->flags |= _EFI_DISK_MARK_DELETE;
+}
+static void efi_disk_mark_undelete(struct efi_disk_obj *disk)
efi_disk_mark_not_deleted.
+{
- disk->flags &= ~_EFI_DISK_MARK_DELETE;
+}
+static bool efi_disk_delete_marked(struct efi_disk_obj *disk)
efi_disk_marked_deleted.
+{
- return disk->flags & _EFI_DISK_MARK_DELETE;
+}
Please, provide a comment describing what the function is meant to do.
+static efi_status_t efi_disk_mark_delete_all(efi_handle_t **handlesp) +{
- efi_handle_t *handles = NULL;
- efi_uintn_t size = 0;
- int num, i;
- struct efi_disk_obj *disk;
- efi_status_t ret;
- ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,
&size, handles);
Please, panic() or error out if ret != EFI_BUFFER_TOO_SMALL.
- if (ret == EFI_BUFFER_TOO_SMALL) {
handles = calloc(1, size);if (!handles)return EFI_OUT_OF_RESOURCES;ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,&size, handles);- }
- if (ret != EFI_SUCCESS) {
free(handles);return ret;- }
- num = size / sizeof(*handles);
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);efi_disk_mark_delete(disk);- }
- *handlesp = handles;
- return num;
+}
+static int efi_disk_mark_undelete_handles(struct blk_desc *desc,
efi_handle_t *handles, int num)+{
- struct efi_disk_obj *disk;
- int disks, i;
- for (i = 0, disks = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!desc || disk->desc == desc) {efi_disk_mark_undelete(disk);disks++;}- }
- return disks;
+}
+static efi_status_t efi_disk_delete_all(efi_handle_t *handles, int num) +{
- struct efi_disk_obj *disk;
- int i;
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!efi_disk_delete_marked(disk))continue;efi_delete_handle(handles[i]);
what will happen if an EFI runtime or boottime driver still has a reference to a handle which will be deleted here?
Isn't the whole idea of the deleted flag that you always keep the handle and reuse it when rescanning finds the same disk?
Please, do not delete any handle to block devices but reuse existing by comparing device paths.
- }
- return EFI_SUCCESS;
+}
+efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK
- efi_handle_t *handles = NULL;
- struct udevice *dev;
- struct blk_desc *desc;
- const char *if_typename;
- struct efi_disk_obj *disk;
- int n, disks = 0;
- efi_status_t ret;
- ret = efi_disk_mark_delete_all(&handles);
- if (ret & EFI_ERROR_MASK)
return ret;
Why should it be an error not to have any block device? Please, return EFI_SUCEESS if ret = EFI_NOT_FOUND.
- n = (int)ret;
- ret = EFI_SUCCESS;
- for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
uclass_next_device_check(&dev)) {desc = dev_get_uclass_platdata(dev);if (n && efi_disk_mark_undelete_handles(desc, handles, n))/* existing device */continue;/* new device */if_typename = blk_get_if_type_name(desc->if_type);/* Add block device for the full device */printf("Scanning disk %s...\n", dev->name);
Please, use debug().
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);continue;}if (ret) {printf("ERROR: failure to add disk device %s, r = %lu\n",dev->name, ret & ~EFI_ERROR_MASK);break;}disks++;/* Partitions show up as block devices in EFI */disks += efi_disk_create_partitions(&disk->header, desc,if_typename,desc->devnum, dev->name);- }
- if (n) {
if (ret != EFI_SUCCESS)efi_disk_mark_undelete_handles(NULL, handles, n);elseret = efi_disk_delete_all(handles, n);free(handles);
Why would you mark all disks deleted if you had a problem with one of them?
Best regards
Heinrich
- }
- return ret;
+#else
- return EFI_SUCCESS;
+#endif +}
Hi Heinrich,
Thank you for your comments. First of all, as I said [1], my essential question is whether my approach here is a right way to go. What do think?
On Wed, Nov 07, 2018 at 08:36:48AM +0100, Heinrich Schuchardt wrote:
On 11/7/18 1:44 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 8 +- include/efi_loader.h | 2 + lib/efi_loader/efi_disk.c | 150 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..493022a09482 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -57,8 +57,14 @@ efi_status_t efi_init_obj_list(void) efi_save_gd();
/* Initialize once only */
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) {
If efi_obj_list_initialized is neither OBJ_LIST_NOT_INITIALIZED nor EFI_SUCCESS, return efi_obj_list_initialized.
Right.
+#ifdef CONFIG_PARTITIONS
ret = efi_disk_update();if (ret != EFI_SUCCESS)printf("+++ updating disks failed\n");+#endif return efi_obj_list_initialized;
}
/* Initialize system table */ ret = efi_initialize_system_table();
diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..e5a080281dba 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,8 @@ efi_status_t efi_initialize_system_table(void); efi_status_t efi_console_register(void); /* Called by bootefi to make all disk storage accessible as EFI objects */ efi_status_t efi_disk_register(void); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..e1d47f34049b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,13 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_MARK_DELETE 0x1
Plese, add a comment to the code explaining what this constant is used for.
Sure.
None of our other constants starts with an underscore.
This flag name is totally efi_disk.c internal. So an underscore is added to prevent any potential name collision with UEFI spec. (unlikely though)
# The name will be renamed to _EFI_DISK_FLAG_DELETE(D) ^^^^
In your coding below you keep handles for deleted drives. What will
No, any handles marked "deleted" will be deleted in this patch.
happen if a binary tries to access a deleted drive?
See below.
/**
- struct efi_disk_obj - EFI disk object
- @header: EFI object header
- @flags: control flags
- @ops: EFI disk I/O protocol interface
- @ifname: interface name for block device
- @dev_index: device index of block device
@@ -30,6 +33,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header;
- int flags; struct efi_block_io ops; const char *ifname; int dev_index;
@@ -440,3 +444,149 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; }
+static void efi_disk_mark_delete(struct efi_disk_obj *disk)
efi_disk_mark_deleted.
OK.
+{
- disk->flags |= _EFI_DISK_MARK_DELETE;
+}
+static void efi_disk_mark_undelete(struct efi_disk_obj *disk)
efi_disk_mark_not_deleted.
I prefer clear_deleted.
+{
- disk->flags &= ~_EFI_DISK_MARK_DELETE;
+}
+static bool efi_disk_delete_marked(struct efi_disk_obj *disk)
efi_disk_marked_deleted.
OK.
+{
- return disk->flags & _EFI_DISK_MARK_DELETE;
+}
Please, provide a comment describing what the function is meant to do.
OK.
+static efi_status_t efi_disk_mark_delete_all(efi_handle_t **handlesp) +{
- efi_handle_t *handles = NULL;
- efi_uintn_t size = 0;
- int num, i;
- struct efi_disk_obj *disk;
- efi_status_t ret;
- ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,
&size, handles);Please, panic() or error out if ret != EFI_BUFFER_TOO_SMALL.
Panic should be always avoided as any efi-related command would fail anyway,
- if (ret == EFI_BUFFER_TOO_SMALL) {
handles = calloc(1, size);if (!handles)return EFI_OUT_OF_RESOURCES;ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,&size, handles);- }
- if (ret != EFI_SUCCESS) {
and this function fails in any failure case.
free(handles);return ret;- }
- num = size / sizeof(*handles);
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);efi_disk_mark_delete(disk);- }
- *handlesp = handles;
- return num;
+}
+static int efi_disk_mark_undelete_handles(struct blk_desc *desc,
efi_handle_t *handles, int num)+{
- struct efi_disk_obj *disk;
- int disks, i;
- for (i = 0, disks = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!desc || disk->desc == desc) {efi_disk_mark_undelete(disk);disks++;}- }
- return disks;
+}
+static efi_status_t efi_disk_delete_all(efi_handle_t *handles, int num) +{
- struct efi_disk_obj *disk;
- int i;
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!efi_disk_delete_marked(disk))continue;efi_delete_handle(handles[i]);what will happen if an EFI runtime or boottime driver still has a reference to a handle which will be deleted here?
Do you know any real case for this situation?
Isn't the whole idea of the deleted flag that you always keep the handle and reuse it when rescanning finds the same disk?
Please, do not delete any handle to block devices but reuse existing by comparing device paths.
I used to think of this option, but we can't exclude any possibility that an new device path matches with one of old (deleted) ones but the content in storage may have been changed. In such a case, we will lose data integrity.
# This is one of reasons behind the question in the top.
Any how, I think the efi_delete_handle() should be able to fail if a handle is still held by others.
- }
- return EFI_SUCCESS;
+}
+efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK
- efi_handle_t *handles = NULL;
- struct udevice *dev;
- struct blk_desc *desc;
- const char *if_typename;
- struct efi_disk_obj *disk;
- int n, disks = 0;
- efi_status_t ret;
- ret = efi_disk_mark_delete_all(&handles);
- if (ret & EFI_ERROR_MASK)
return ret;Why should it be an error not to have any block device? Please, return EFI_SUCEESS if ret = EFI_NOT_FOUND.
Right.
- n = (int)ret;
- ret = EFI_SUCCESS;
- for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
uclass_next_device_check(&dev)) {desc = dev_get_uclass_platdata(dev);if (n && efi_disk_mark_undelete_handles(desc, handles, n))/* existing device */continue;/* new device */if_typename = blk_get_if_type_name(desc->if_type);/* Add block device for the full device */printf("Scanning disk %s...\n", dev->name);Please, use debug().
Efi_disk_register() does this, too.
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);continue;}if (ret) {printf("ERROR: failure to add disk device %s, r = %lu\n",dev->name, ret & ~EFI_ERROR_MASK);break;}disks++;/* Partitions show up as block devices in EFI */disks += efi_disk_create_partitions(&disk->header, desc,if_typename,desc->devnum, dev->name);- }
- if (n) {
if (ret != EFI_SUCCESS)efi_disk_mark_undelete_handles(NULL, handles, n);elseret = efi_disk_delete_all(handles, n);free(handles);Why would you mark all disks deleted if you had a problem with one of them?
I'm afraid that you misunderstand here. The code will try to clear "deleted" marks for recovery in failure case as we can't do anything other than that here.
# "if" condition will be also reverted for better understanding.
Thanks, -Takahiro Akashi
Best regards
Heinrich
- }
- return ret;
+#else
- return EFI_SUCCESS;
+#endif +}
On 11/9/18 7:22 AM, AKASHI Takahiro wrote:
Hi Heinrich,
Thank you for your comments. First of all, as I said [1], my essential question is whether my approach here is a right way to go. What do think?
The approach that you have chosen is surely an improvement. On the other hand let's look at the target state:
Simon want's to eliminate all block device drivers that are not device tree based. So we could say an EFI handle for a disk has to be created when a device tree node is created for a block device. When the node is removed the EFI handle should be marked as stale.
The UEFI spec foresees applications, boottime drivers and runtime drivers as binaries. U-Boot cannot decide if a driver keeps a reference to a handle. So, please, do not deallocate EFI handles for block devices unless an application or driver (like iPXE) removes the last protocol.
Instead return "device not ready" if a stale block device handle is used.
Best regards
Heinrich
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt