Am 27. März 2025 11:02:55 MEZ schrieb Neil Armstrong
<neil.armstr...@linaro.org>:
>The actual architecture of the EFI part parser means the
>entire GPT partition table needs to be parsed for each
>part_info() callback.
>
>Since the default part scan code will always scan up to
>128 partitions for a block, and devices with an UFS chip
>with up to 8 LUNs are very common in the field, this means
>a complete GPT parsing and validation will be done
>up to 1024 times instead of 8 on such devices.
>
>The GPT parsing can be cached between each part_info()
>call speed up to 3x on the worse condition when the CPU
>i-cache and d-cache are disabled, like in the SPL.
>
>The implementation does caching each time a correct GPT
>has been parsed and verified and will match the blk_desc
>and LBA to serve the cached data or force a re-parsing.
>
>In order to allow GPT manipulation, some of the calls
>ignores the cache and some will discard the cached data.
>
>On the SM8650 QRD platform with a KIOXIA THGJFJT1E45BATPC
>configured with 8 LUNs, the scsi scan takes 0.2s with both
>CPU caches enabled, but when disabling both CPU caches it
>goes up to 4s to do the full scan of all 8 LUN partitions.
>
>With this change the scan takes only 0.18s with both CPU
>caches enabled running 1.1x times faster, and with both CPU
>caches disabled the full scan takes only 1.27s running
>3x faster.
>
>While 20ms could look negligeable, it's still a 20ms gain
>in the boot flow and a non negligeable reduction in
>calculation and memory allocation since for each scan
>it would allocate and free the gpt_pte table up to
>1024 times, now it would only do 8 allocations, reducing
>memory fragmentation.
Could you, please, describe when you clear the cache. I would think of:
* Media change
* Updating partition table (mbr and gpt commands)
* Any raw write to sector < 35
I cannot see any of this implemented.
>
>Signed-off-by: Neil Armstrong <neil.armstr...@linaro.org>
>---
> disk/part_efi.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 77 insertions(+), 16 deletions(-)
>
>diff --git a/disk/part_efi.c b/disk/part_efi.c
>index
>932d058c184ce6946b7142e7c2d9637b28e4661e..4f02fabe265ce5ca4c8f55e41768a95582022e21
> 100644
>--- a/disk/part_efi.c
>+++ b/disk/part_efi.c
>@@ -55,13 +55,59 @@ static inline u32 efi_crc32(const void *buf, u32 len)
> static int pmbr_part_valid(struct partition *part);
> static int is_pmbr_valid(legacy_mbr * mbr);
> static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>- gpt_entry **pgpt_pte);
>+ gpt_entry **pgpt_pte, bool cache);
Please, provide Sphinx style function documentation describing all parameters.
> static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
> gpt_header *pgpt_head);
> static int is_pte_valid(gpt_entry * pte);
> static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
> gpt_entry **pgpt_pte);
>
>+static struct gpt_pte_cache_data {
Please, provide Sphinx style documentation for the structure and its elements.
>+ struct blk_desc *desc;
>+ u64 lba;
>+ gpt_entry *gpt_pte;
>+ gpt_header gpt_head;
>+} gpt_pte_cache;
>+
>+static void clear_gpt_pte_cache(void)
>+{
Add function documentation for all changed function interfaces.
Besr regards
Heinrich
>+ if (gpt_pte_cache.desc) {
>+ if (gpt_pte_cache.gpt_pte)
>+ free(gpt_pte_cache.gpt_pte);
>+
>+ memset(&gpt_pte_cache, 0, sizeof(gpt_pte_cache));
>+ }
>+}
>+
>+static void cache_gpt_pte(struct blk_desc *desc, u64 lba,
>+ gpt_entry *gpt_pte, gpt_header *pgpt_head)
>+{
>+ if (gpt_pte_cache.gpt_pte)
>+ free(gpt_pte_cache.gpt_pte);
>+
>+ gpt_pte_cache.desc = desc;
>+ gpt_pte_cache.lba = lba;
>+ gpt_pte_cache.gpt_pte = gpt_pte;
>+ if (pgpt_head)
>+ memcpy(&gpt_pte_cache.gpt_head, pgpt_head, sizeof(gpt_header));
>+}
>+
>+static gpt_entry *get_cached_gpt_pte(struct blk_desc *desc, u64 lba,
>+ gpt_header *pgpt_head)
>+{
>+ if (gpt_pte_cache.desc && gpt_pte_cache.gpt_pte) {
>+ if (gpt_pte_cache.desc == desc &&
>+ gpt_pte_cache.lba == lba) {
>+ memcpy(pgpt_head, &gpt_pte_cache.gpt_head,
>sizeof(gpt_header));
>+ return gpt_pte_cache.gpt_pte;
>+ }
>+
>+ clear_gpt_pte_cache();
>+ }
>+
>+ return NULL;
>+}
>+
> static char *print_efiname(gpt_entry *pte)
> {
> static char name[PARTNAME_SZ + 1];
>@@ -211,8 +257,6 @@ int get_disk_guid(struct blk_desc *desc, char *guid)
> guid_bin = gpt_head->disk_guid.b;
> uuid_bin_to_str(guid_bin, guid, UUID_STR_FORMAT_GUID);
>
>- /* Remember to free pte */
>- free(gpt_pte);
> return 0;
> }
>
>@@ -252,9 +296,6 @@ static void __maybe_unused part_print_efi(struct blk_desc
>*desc)
> uuid = (unsigned char *)gpt_pte[i].unique_partition_guid.b;
> printf("\tguid:\t%pUl\n", uuid);
> }
>-
>- /* Remember to free pte */
>- free(gpt_pte);
> return;
> }
>
>@@ -277,7 +318,6 @@ static int __maybe_unused part_get_info_efi(struct
>blk_desc *desc, int part,
> if (part > le32_to_cpu(gpt_head->num_partition_entries) ||
> !is_pte_valid(&gpt_pte[part - 1])) {
> log_debug("Invalid partition number %d\n", part);
>- free(gpt_pte);
> return -EPERM;
> }
>
>@@ -307,8 +347,6 @@ static int __maybe_unused part_get_info_efi(struct
>blk_desc *desc, int part,
> log_debug("start 0x" LBAF ", size 0x" LBAF ", name %s\n", info->start,
> info->size, info->name);
>
>- /* Remember to free pte */
>- free(gpt_pte);
> return 0;
> }
>
>@@ -382,6 +420,9 @@ int write_gpt_table(struct blk_desc *desc, gpt_header
>*gpt_h, gpt_entry *gpt_e)
> * sizeof(gpt_entry)), desc);
> u32 calc_crc32;
>
>+ /* Clean cache */
>+ clear_gpt_pte_cache();
>+
> debug("max lba: %x\n", (u32)desc->lba);
> /* Setup the Protective MBR */
> if (set_protective_mbr(desc) < 0)
>@@ -620,6 +661,9 @@ int gpt_restore(struct blk_desc *desc, char *str_disk_guid,
> gpt_entry *gpt_e;
> int ret, size;
>
>+ /* Clean cache */
>+ clear_gpt_pte_cache();
>+
> size = PAD_TO_BLOCKSIZE(sizeof(gpt_header), desc);
> gpt_h = malloc_cache_aligned(size);
> if (gpt_h == NULL) {
>@@ -689,7 +733,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header
>*gpt_head,
> */
> if (is_gpt_valid(desc,
> GPT_PRIMARY_PARTITION_TABLE_LBA,
>- gpt_head, gpt_pte) != 1) {
>+ gpt_head, gpt_pte, false) != 1) {
> log_debug("Invalid GPT\n");
> return -1;
> }
>@@ -706,7 +750,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header
>*gpt_head,
> }
>
> if (is_gpt_valid(desc, (desc->lba - 1),
>- gpt_head, gpt_pte) != 1) {
>+ gpt_head, gpt_pte, false) != 1) {
> log_debug("Invalid Backup GPT\n");
> return -1;
> }
>@@ -764,10 +808,13 @@ int gpt_repair_headers(struct blk_desc *desc)
> int is_gpt1_valid, is_gpt2_valid;
> int ret = -1;
>
>+ /* Clean cache */
>+ clear_gpt_pte_cache();
>+
> is_gpt1_valid = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>- gpt_h1, &gpt_e1);
>+ gpt_h1, &gpt_e1, false);
> is_gpt2_valid = is_gpt_valid(desc, desc->lba - 1,
>- gpt_h2, &gpt_e2);
>+ gpt_h2, &gpt_e2, false);
>
> if (is_gpt1_valid && is_gpt2_valid) {
> ret = 0;
>@@ -906,6 +953,9 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
>void *buf)
> lbaint_t lba;
> int cnt;
>
>+ /* Clean cache */
>+ clear_gpt_pte_cache();
>+
> if (is_valid_gpt_buf(desc, buf))
> return -1;
>
>@@ -1023,12 +1073,13 @@ static int is_pmbr_valid(legacy_mbr *mbr)
> * lba is the logical block address of the GPT header to test
> * gpt is a GPT header ptr, filled on return.
> * ptes is a PTEs ptr, filled on return.
>+ * cache is a bool, true to use the cached gpt_pte from previous call
> *
> * Description: returns 1 if valid, 0 on error, 2 if ignored header
> * If valid, returns pointers to PTEs.
> */
> static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>- gpt_entry **pgpt_pte)
>+ gpt_entry **pgpt_pte, bool cache)
> {
> /* Confirm valid arguments prior to allocation. */
> if (!desc || !pgpt_head) {
>@@ -1036,6 +1087,12 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba,
>gpt_header *pgpt_head,
> return 0;
> }
>
>+ if (cache) {
>+ *pgpt_pte = get_cached_gpt_pte(desc, lba, pgpt_head);
>+ if (*pgpt_pte)
>+ return 1;
>+ }
>+
> ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, mbr, 1, desc->blksz);
>
> /* Read MBR Header from device */
>@@ -1081,6 +1138,9 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba,
>gpt_header *pgpt_head,
> return 0;
> }
>
>+ if (cache)
>+ cache_gpt_pte(desc, lba, *pgpt_pte, pgpt_head);
>+
> /* We're done, all's well */
> return 1;
> }
>@@ -1100,13 +1160,14 @@ static int find_valid_gpt(struct blk_desc *desc,
>gpt_header *gpt_head,
> int r;
>
> r = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_head,
>- pgpt_pte);
>+ pgpt_pte, true);
>
> if (r != 1) {
> if (r != 2)
> log_debug("Invalid GPT\n");
>
>- if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte)
>+ if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte,
>+ true)
> != 1) {
> log_debug("Invalid Backup GPT\n");
> return 0;
>
>---
>base-commit: 4adbf64ff8d8c730223fd8ae299d770bebb6fe86
>change-id: 20250327-u-boot-efi-part-cache-ddc1499c3ef6
>
>Best regards,
