[PATCH] EFI: Fix ReadBlocks API reading incorrect sector for, UCLASS_PARTITION devices
From 7a7dd7f16352fc916279cca05a3fa617f8bbef64 Mon Sep 17 00:00:00 2001 From: Paul Barbieri plb365@gmail.com Date: Tue, 28 Jun 2022 20:24:33 -0400 Subject: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for UCLASS_PARTITION devices
The requested partition disk sector incorrectly has the partition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
Signed-Off-by: Paul Barbieri paul.barbieri@verizon.net Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org --- lib/efi_loader/efi_disk.c | 8 +------- lib/efi_selftest/efi_selftest_block_device.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @dp: device path to the block device * @part: partition * @volume: simple file system protocol of the partition - * @offset: offset into disk for simple partition * @dev: associated DM device */ struct efi_disk_obj { @@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz; - lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction); @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->offset = part_info->start; diskobj->media.last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) guid = &efi_system_partition_guid; } else { diskobj->dp = efi_dp_from_part(desc, part); - diskobj->offset = 0; diskobj->media.last_block = desc->lba - 1; } diskobj->part = part; @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" - ", offset " LBAF ", last_block %llu\n", + ", last_block %llu\n", diskobj->part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, - diskobj->offset, diskobj->media.last_block);
/* Store first EFI system partition */ diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..ef6bdafe2e 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@ * ConnectController is used to setup partitions and to install the simple * file protocol. * A known file is read from the file system and verified. + * Test that the read_blocks API correctly reads a block from the device. */
#include <efi_selftest.h> @@ -312,6 +313,7 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos; + char block[512];
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); @@ -449,6 +451,23 @@ static int execute(void) return EFI_ST_FAILURE; }
+ /* Test read_blocks() can read same file data. */ + boottime->set_mem(block, block_io_protocol->media->block_size, 0); + ret = block_io_protocol->read_blocks(block_io_protocol, + block_io_protocol->media->media_id, + (0x5000 >> LB_BLOCK_SIZE) - 1, + block_io_protocol->media->block_size, + block); + if (ret != EFI_SUCCESS) { + efi_st_error("ReadBlocks failed\n"); + return EFI_ST_FAILURE; + } + + if (memcmp(&block[1], buf, 11)) { + efi_st_error("Unexpected block content\n"); + return EFI_ST_FAILURE; + } + #ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
Hi Paul,
On Tue, Jun 28, 2022 at 09:02:47PM -0400, Paul Barbieri wrote:
From 7a7dd7f16352fc916279cca05a3fa617f8bbef64 Mon Sep 17 00:00:00 2001 From: Paul Barbieri plb365@gmail.com Date: Tue, 28 Jun 2022 20:24:33 -0400 Subject: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for UCLASS_PARTITION devices
The requested partition disk sector incorrectly has the partition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
Your change on efi_disk.c looks good, but
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
Your test will never exercise the code you added in efi_disk.c (or efi_disk_rw_blocks()) because "block device" selftest uses its own block device driver.
-Takahiro Akashi
Signed-Off-by: Paul Barbieri paul.barbieri@verizon.net Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_disk.c | 8 +------- lib/efi_selftest/efi_selftest_block_device.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @dp: device path to the block device * @part: partition * @volume: simple file system protocol of the partition
- @offset: offset into disk for simple partition
* @dev: associated DM device */ struct efi_disk_obj { @@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz; - lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction); @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->offset = part_info->start; diskobj->media.last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) guid = &efi_system_partition_guid; } else { diskobj->dp = efi_dp_from_part(desc, part); - diskobj->offset = 0; diskobj->media.last_block = desc->lba - 1; } diskobj->part = part; @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" - ", offset " LBAF ", last_block %llu\n", + ", last_block %llu\n", diskobj->part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, - diskobj->offset, diskobj->media.last_block);
/* Store first EFI system partition */ diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..ef6bdafe2e 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@ * ConnectController is used to setup partitions and to install the simple * file protocol. * A known file is read from the file system and verified.
- Test that the read_blocks API correctly reads a block from the device.
*/
#include <efi_selftest.h> @@ -312,6 +313,7 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos; + char block[512];
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); @@ -449,6 +451,23 @@ static int execute(void) return EFI_ST_FAILURE; }
+ /* Test read_blocks() can read same file data. */ + boottime->set_mem(block, block_io_protocol->media->block_size, 0); + ret = block_io_protocol->read_blocks(block_io_protocol,
- block_io_protocol->media->media_id,
+ (0x5000 >> LB_BLOCK_SIZE) - 1,
- block_io_protocol->media->block_size,
+ block); + if (ret != EFI_SUCCESS) { + efi_st_error("ReadBlocks failed\n"); + return EFI_ST_FAILURE; + }
+ if (memcmp(&block[1], buf, 11)) { + efi_st_error("Unexpected block content\n"); + return EFI_ST_FAILURE; + }
#ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ | -- 2.17.1
On 6/29/22 03:02, Paul Barbieri wrote:
From 7a7dd7f16352fc916279cca05a3fa617f8bbef64 Mon Sep 17 00:00:00 2001 From: Paul Barbieri plb365@gmail.com Date: Tue, 28 Jun 2022 20:24:33 -0400 Subject: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for UCLASS_PARTITION devices
The requested partition disk sector incorrectly has the partition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
This is not a valid patch:
$ git am /tmp/0.patch warning: Patch sent with format=flowed; space at the end of lines might be lost. Applying: EFI: Fix ReadBlocks API reading incorrect sector for, UCLASS_PARTITION devices error: corrupt patch at line 11 Patch failed at 0001 EFI: Fix ReadBlocks API reading incorrect sector for, UCLASS_PARTITION devices
Please, use 'git send-email' for sending patches not Thunderbird.
Best regards
Heinrich
Signed-Off-by: Paul Barbieri paul.barbieri@verizon.net Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_disk.c | 8 +------- lib/efi_selftest/efi_selftest_block_device.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @dp: device path to the block device * @part: partition * @volume: simple file system protocol of the partition
- @offset: offset into disk for simple partition
* @dev: associated DM device */ struct efi_disk_obj { @@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz; - lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction); @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->offset = part_info->start; diskobj->media.last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) guid = &efi_system_partition_guid; } else { diskobj->dp = efi_dp_from_part(desc, part); - diskobj->offset = 0; diskobj->media.last_block = desc->lba - 1; } diskobj->part = part; @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" - ", offset " LBAF ", last_block %llu\n", + ", last_block %llu\n", diskobj->part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, - diskobj->offset, diskobj->media.last_block);
/* Store first EFI system partition */ diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..ef6bdafe2e 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@ * ConnectController is used to setup partitions and to install the simple * file protocol. * A known file is read from the file system and verified.
- Test that the read_blocks API correctly reads a block from the device.
*/
#include <efi_selftest.h> @@ -312,6 +313,7 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos; + char block[512];
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); @@ -449,6 +451,23 @@ static int execute(void) return EFI_ST_FAILURE; }
+ /* Test read_blocks() can read same file data. */ + boottime->set_mem(block, block_io_protocol->media->block_size, 0); + ret = block_io_protocol->read_blocks(block_io_protocol,
- block_io_protocol->media->media_id,
+ (0x5000 >> LB_BLOCK_SIZE) - 1,
- block_io_protocol->media->block_size,
+ block); + if (ret != EFI_SUCCESS) { + efi_st_error("ReadBlocks failed\n"); + return EFI_ST_FAILURE; + }
+ if (memcmp(&block[1], buf, 11)) { + efi_st_error("Unexpected block content\n"); + return EFI_ST_FAILURE; + }
#ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
The requsted partition disk sector incorrectly has the parition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
I verified that the test was using the correct code path by defining DEBUG in lib/efi_loader/efi_disk.c This showed that when the test was run, efi_disk_read_blocks() was executed and it called efi_disk_rw_blocks(). Adding a little extra debug, I saw that efi_disk_rw_blocks() called dev_read(). I also added a debug print in the read_blocks() routine of efi_selftest_block_device.c. One thing to note, if I moved the read_blocks test before the file system test, I would see the read_blocks() routine print in the middle of the efi_disk_rw_blocks() routine. If I put the test after the file system test, I don't see read_blocks() get called. I assume this is because dev_read() is able to get the block from the cache.
Signed-Off-by: Paul Barbieri plb365@gmail.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org --- lib/efi_loader/efi_disk.c | 8 +------ lib/efi_selftest/efi_selftest_block_device.c | 23 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @dp: device path to the block device * @part: partition * @volume: simple file system protocol of the partition - * @offset: offset into disk for simple partition * @dev: associated DM device */ struct efi_disk_obj { @@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz; - lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction); @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->offset = part_info->start; diskobj->media.last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) guid = &efi_system_partition_guid; } else { diskobj->dp = efi_dp_from_part(desc, part); - diskobj->offset = 0; diskobj->media.last_block = desc->lba - 1; } diskobj->part = part; @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" - ", offset " LBAF ", last_block %llu\n", + ", last_block %llu\n", diskobj->part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, - diskobj->offset, diskobj->media.last_block);
/* Store first EFI system partition */ diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..7b50c829ad 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@ * ConnectController is used to setup partitions and to install the simple * file protocol. * A known file is read from the file system and verified. + * Test that the read_blocks API correctly reads a block from the device. */
#include <efi_selftest.h> @@ -312,6 +313,9 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos; + char block[2 * (1 << LB_BLOCK_SIZE)]; + char *block_io_aligned; + u32 align;
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); @@ -449,6 +453,25 @@ static int execute(void) return EFI_ST_FAILURE; }
+ /* Test read_blocks() can read same file data. */ + boottime->set_mem(block, sizeof(block), 0); + align = block_io_protocol->media->io_align; + block_io_aligned = (char *)(((uintptr_t)block + align-1) & ~(align-1)); + ret = block_io_protocol->read_blocks(block_io_protocol, + block_io_protocol->media->media_id, + (0x5000 >> LB_BLOCK_SIZE) - 1, + block_io_protocol->media->block_size, + block_io_aligned); + if (ret != EFI_SUCCESS) { + efi_st_error("ReadBlocks failed\n"); + return EFI_ST_FAILURE; + } + + if (memcmp(&block_io_aligned[1], buf, 11)) { + efi_st_error("Unexpected block content\n"); + return EFI_ST_FAILURE; + } + #ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
On 6/29/22 19:44, Paul Barbieri wrote:
The requsted partition disk sector incorrectly has the parition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
I verified that the test was using the correct code path by defining DEBUG in lib/efi_loader/efi_disk.c This showed that when the test was run, efi_disk_read_blocks() was executed and it called efi_disk_rw_blocks(). Adding a little extra debug, I saw that efi_disk_rw_blocks() called dev_read(). I also added a debug print in the read_blocks() routine of efi_selftest_block_device.c. One thing to note, if I moved the read_blocks test before the file system test, I would see the read_blocks() routine print in the middle of the efi_disk_rw_blocks() routine. If I put the test after the file system test, I don't see read_blocks() get called. I assume this is because dev_read() is able to get the block from the cache.
Signed-Off-by: Paul Barbieri plb365@gmail.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_disk.c | 8 +------ lib/efi_selftest/efi_selftest_block_device.c | 23 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID;
- @dp: device path to the block device
- @part: partition
- @volume: simple file system protocol of the partition
*/ struct efi_disk_obj {
- @offset: offset into disk for simple partition
- @dev: associated DM device
@@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume;
- lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz;
lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction);
@@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node);
diskobj->offset = part_info->start;diskobj->offset = 0;@@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d"
", offset " LBAF ", last_block %llu\n",
", last_block %llu\n",
diskobj->offset,diskobj->media.last_block);
/* Store first EFI system partition */
diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..7b50c829ad 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@
- ConnectController is used to setup partitions and to install the simple
- file protocol.
- A known file is read from the file system and verified.
- Test that the read_blocks API correctly reads a block from the device.
*/
#include <efi_selftest.h>
@@ -312,6 +313,9 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos;
char block[2 * (1 << LB_BLOCK_SIZE)];
char *block_io_aligned;
u32 align;
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1);
@@ -449,6 +453,25 @@ static int execute(void) return EFI_ST_FAILURE; }
- /* Test read_blocks() can read same file data. */
- boottime->set_mem(block, sizeof(block), 0);
- align = block_io_protocol->media->io_align;
- block_io_aligned = (char *)(((uintptr_t)block + align-1) & ~(align-1));
- ret = block_io_protocol->read_blocks(block_io_protocol,
block_io_protocol->media->media_id,(0x5000 >> LB_BLOCK_SIZE) - 1,
This code really needs a comment. E.g. In the test data
* the partition starts at block 1 * the file hello.txt with content 'Hello world!' is located at 0x5000 of the disk
Here we read block 0x27 (offset 0x4e00 of the partition) and expect the string 'Hello world!' to be at the start of the block.
Best regards
Heinrich
block_io_protocol->media->block_size,block_io_aligned);- if (ret != EFI_SUCCESS) {
efi_st_error("ReadBlocks failed\n");return EFI_ST_FAILURE;- }
- if (memcmp(&block_io_aligned[1], buf, 11)) {
efi_st_error("Unexpected block content\n");return EFI_ST_FAILURE;- }
- #ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
The requsted partition disk sector incorrectly has the parition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
Signed-Off-by: Paul Barbieri plb365@gmail.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org --- Changes for v3: - Added requested comment in test code
lib/efi_loader/efi_disk.c | 8 +----- lib/efi_selftest/efi_selftest_block_device.c | 28 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @dp: device path to the block device * @part: partition * @volume: simple file system protocol of the partition - * @offset: offset into disk for simple partition * @dev: associated DM device */ struct efi_disk_obj { @@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz; - lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction); @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->offset = part_info->start; diskobj->media.last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) guid = &efi_system_partition_guid; } else { diskobj->dp = efi_dp_from_part(desc, part); - diskobj->offset = 0; diskobj->media.last_block = desc->lba - 1; } diskobj->part = part; @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" - ", offset " LBAF ", last_block %llu\n", + ", last_block %llu\n", diskobj->part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, - diskobj->offset, diskobj->media.last_block);
/* Store first EFI system partition */ diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..d11f673148 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@ * ConnectController is used to setup partitions and to install the simple * file protocol. * A known file is read from the file system and verified. + * Test that the read_blocks API correctly reads a block from the device. */
#include <efi_selftest.h> @@ -312,6 +313,9 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos; + char block[2 * (1 << LB_BLOCK_SIZE)]; + char *block_io_aligned; + u32 align;
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); @@ -449,6 +453,30 @@ static int execute(void) return EFI_ST_FAILURE; }
+ /* Test read_blocks() can read same file data. */ + boottime->set_mem(block, sizeof(block), 0); + align = block_io_protocol->media->io_align; + block_io_aligned = (char *)(((uintptr_t)block + align-1) & ~(align-1)); + /* In the test data, the partition starts at block 1 and the file + hello.txt with the content 'Hello world!' is located at 0x5000 + of the disk. Here we read block 0x27 (offset 0x4e00 of the + partition) and expect the string 'Hello world!' to be at the + start of block. */ + ret = block_io_protocol->read_blocks(block_io_protocol, + block_io_protocol->media->media_id, + (0x5000 >> LB_BLOCK_SIZE) - 1, + block_io_protocol->media->block_size, + block_io_aligned); + if (ret != EFI_SUCCESS) { + efi_st_error("ReadBlocks failed\n"); + return EFI_ST_FAILURE; + } + + if (memcmp(&block_io_aligned[1], buf, 11)) { + efi_st_error("Unexpected block content\n"); + return EFI_ST_FAILURE; + } + #ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
On 6/30/22 13:02, Paul Barbieri wrote:
The requsted partition disk sector incorrectly has the parition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
Signed-Off-by: Paul Barbieri plb365@gmail.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org
Changes for v3: - Added requested comment in test code
lib/efi_loader/efi_disk.c | 8 +----- lib/efi_selftest/efi_selftest_block_device.c | 28 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID;
- @dp: device path to the block device
- @part: partition
- @volume: simple file system protocol of the partition
*/ struct efi_disk_obj {
- @offset: offset into disk for simple partition
- @dev: associated DM device
@@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume;
- lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz;
lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction);
@@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node);
diskobj->offset = part_info->start;diskobj->offset = 0;@@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d"
", offset " LBAF ", last_block %llu\n",
", last_block %llu\n",
diskobj->offset,diskobj->media.last_block);
/* Store first EFI system partition */
diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..d11f673148 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@
- ConnectController is used to setup partitions and to install the simple
- file protocol.
- A known file is read from the file system and verified.
- Test that the read_blocks API correctly reads a block from the device.
*/
#include <efi_selftest.h>
@@ -312,6 +313,9 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos;
char block[2 * (1 << LB_BLOCK_SIZE)];
char *block_io_aligned;
u32 align;
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1);
@@ -449,6 +453,30 @@ static int execute(void) return EFI_ST_FAILURE; }
- /* Test read_blocks() can read same file data. */
- boottime->set_mem(block, sizeof(block), 0);
- align = block_io_protocol->media->io_align;
- block_io_aligned = (char *)(((uintptr_t)block + align-1) & ~(align-1));
Align is a 32 bit value but the pointer may be a 64 bit address:
Running the test on the sandbox results in a crash:
align: 512 block: 0x00007ffe78df28b8 block_io_aligned: 0x0000000078df2a00 Segmentation fault (core dumped)
IoAlign = 0 is an allowable value. Your code would set the last 32 bits to zero in this case.
Keep it simple. Just define block_io_align as an LB_BLOCK_SIZE aligned array of size LB_BLOCK_SIZE. Anyway the block driver in this unit test does not care about alignment. read_blocks() only executes memcpy().
Best regards
Heinrich
- /* In the test data, the partition starts at block 1 and the file
hello.txt with the content 'Hello world!' is located at 0x5000of the disk. Here we read block 0x27 (offset 0x4e00 of thepartition) and expect the string 'Hello world!' to be at thestart of block. */- ret = block_io_protocol->read_blocks(block_io_protocol,
block_io_protocol->media->media_id,(0x5000 >> LB_BLOCK_SIZE) - 1,block_io_protocol->media->block_size,block_io_aligned);- if (ret != EFI_SUCCESS) {
efi_st_error("ReadBlocks failed\n");return EFI_ST_FAILURE;- }
- if (memcmp(&block_io_aligned[1], buf, 11)) {
efi_st_error("Unexpected block content\n");return EFI_ST_FAILURE;- }
- #ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
The requsted partition disk sector incorrectly has the parition start sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() routine adds the diskobj->offset to the requested lba. When the device is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
Takahiro Akashi suggested removing the offset field from the efi_disk_obj structure since disk-uclass.c handles the partition start biasing. Device types other than UCLASS_PARTITION set the diskobj->offset field to zero which makes the field unnecessary. This change removes the offset field from the structure and removes all references from the code which is isolated to the lib/efi_loader/efi_disk.c module.
This change also adds a test for the EFI ReadBlocks() API in the EFI selftest code. There is already a test for reading a FAT file. The new test uses ReadBlocks() to read the same "disk" block and compare it to the data read from the file system API.
Signed-Off-by: Paul Barbieri plb365@gmail.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: AKASHI Takahiro takahiro.akashi@linaro.org --- Changes for v3: - Added requested comment in test code Changes for v4: - Use aligned buffer instead of computing aligned address
lib/efi_loader/efi_disk.c | 8 +------ lib/efi_selftest/efi_selftest_block_device.c | 24 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1e82f52dc0..1d700b2a6b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @dp: device path to the block device * @part: partition * @volume: simple file system protocol of the partition - * @offset: offset into disk for simple partition * @dev: associated DM device */ struct efi_disk_obj { @@ -47,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - lbaint_t offset; struct udevice *dev; /* TODO: move it to efi_object */ };
@@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, diskobj = container_of(this, struct efi_disk_obj, ops); blksz = diskobj->media.block_size; blocks = buffer_size / blksz; - lba += diskobj->offset;
EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", blocks, lba, blksz, direction); @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
diskobj->dp = efi_dp_append_node(dp_parent, node); efi_free_pool(node); - diskobj->offset = part_info->start; diskobj->media.last_block = part_info->size - 1; if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) guid = &efi_system_partition_guid; } else { diskobj->dp = efi_dp_from_part(desc, part); - diskobj->offset = 0; diskobj->media.last_block = desc->lba - 1; } diskobj->part = part; @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( *disk = diskobj;
EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" - ", offset " LBAF ", last_block %llu\n", + ", last_block %llu\n", diskobj->part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, - diskobj->offset, diskobj->media.last_block);
/* Store first EFI system partition */ diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index 60fa655766..6da923909e 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -11,6 +11,7 @@ * ConnectController is used to setup partitions and to install the simple * file protocol. * A known file is read from the file system and verified. + * Test that the read_blocks API correctly reads a block from the device. */
#include <efi_selftest.h> @@ -312,6 +313,7 @@ static int execute(void) char buf[16] __aligned(ARCH_DMA_MINALIGN); u32 part1_size; u64 pos; + char block[1 << LB_BLOCK_SIZE] __aligned(1 << LB_BLOCK_SIZE);
/* Connect controller to virtual disk */ ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); @@ -449,6 +451,28 @@ static int execute(void) return EFI_ST_FAILURE; }
+ /* Test read_blocks() can read same file data. */ + boottime->set_mem(block, sizeof(block), 0); + /* In the test data, the partition starts at block 1 and the file + hello.txt with the content 'Hello world!' is located at 0x5000 + of the disk. Here we read block 0x27 (offset 0x4e00 of the + partition) and expect the string 'Hello world!' to be at the + start of block. */ + ret = block_io_protocol->read_blocks(block_io_protocol, + block_io_protocol->media->media_id, + (0x5000 >> LB_BLOCK_SIZE) - 1, + block_io_protocol->media->block_size, + block); + if (ret != EFI_SUCCESS) { + efi_st_error("ReadBlocks failed\n"); + return EFI_ST_FAILURE; + } + + if (memcmp(&block[1], buf, 11)) { + efi_st_error("Unexpected block content\n"); + return EFI_ST_FAILURE; + } + #ifdef CONFIG_FAT_WRITE /* Write file */ ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Paul Barbieri