[U-Boot] [PATCH v4 5/5] efi_loader: disk: install file system protocol to a whole disk

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

On Sat, Oct 12, 2019 at 09:43:33PM +0200, Heinrich Schuchardt wrote:
> On 10/7/19 7:59 AM, AKASHI Takahiro wrote:
> >Currently, a whole disk without any partitions is not associated
> >with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses some
> >file system, there is a chance that we may not be able to access
> >it, particularly, when accesses are to be attempted after searching
> >that protocol against a device handle.
> >
> >With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
> >to such a disk if part_get_info() shows there is no partition
> >table installed on it.
> 
> That is what I would expect. But it is not what this patch really does.

Literally, you're right, but

> See below.
> 
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  lib/efi_loader/efi_disk.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >index 861fcaf3747f..7ee4ed26a2ea 100644
> >--- a/lib/efi_loader/efi_disk.c
> >+++ b/lib/efi_loader/efi_disk.c
> >@@ -337,7 +337,8 @@ static efi_status_t efi_disk_add_dev(
> >  			       diskobj->dp);
> >  	if (ret != EFI_SUCCESS)
> >  		return ret;
> >-	if (part >= 1 && efi_fs_exists(desc, part)) {
> >+	/* partitions or whole disk without partitions */
> >+	if (efi_fs_exists(desc, part)) {
> 
> Function efi_fs_exists() does not check if there is a partition table.
> It only checks if there is a file system.
> 
> For creating a test image you can use the following commands:
> 
> cat > partioning << EOF
> label: dos
> label-id: 0x6fe3a999
> device: image
> unit: sectors
> image1: start=    1024, size= 524288, type=0c
> EOF
> dd if=/dev/zero of=test.img count=1 bs=1MiB
> /usr/sbin/sfdisk test.img < partioning
> dd if=test.img of=partition.tbl bs=8 count=9 skip=55
> /usr/sbin/mkfs.vfat test.img 1024
> dd conv=fsync,notrunc if=partition.tbl of=test.img bs=8 count=9 seek=55
> sudo losetup -o 524288 --sizelimit 524288 /dev/loop1 test.img
> sudo mkfs.vfat /dev/loop1
> sudo losetup -D /dev/loop1
> sudo mount test.img /mnt
> sudo sh -c "echo 'file system on block device' > /mnt/description.txt"
> sudo umount /mnt
> sudo mount test.img /mnt -o offset=524288
> sudo sh -c "echo 'file system on partition 1' > /mnt/description.txt"
> sudo umount /mnt

Your example above is quite quirky, and totally unpractical.
If people would create such a file system, they would expect
that they could access a raw block device as well as a partition #1.

However, I would like to drop this patch(#5) from my patch set
because I don't find any public interface that can be used to
determine if there exists a file system on a raw device.

I initially thought of reverting my patch to v2, where part_get_info()
was used, but it doesn't work as expected for part == 0.
For example, GPT partition dirver, disk_efi.c, uses an internal
function, find_valid_gpt(), but part_get_info_efi() returns an error
for part == 0.

For me, it is much easier to modify my pytest for UEFI secure boot
than to carve out a generic interface for *all* the file systems.

My patch #1 to #4 have already been reviewed by you and there are
no outstanding issues.

-Takahiro Akashi

> Without your patch I get
> 
> UEFI Interactive Shell v2.2
> EDK II
> UEFI v2.80 (Das U-Boot, 0x20191000)
> Mapping table
>       FS0: Alias(s):HD0a0b:;BLK1:
> 
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(1,MBR,0x6fe3a999,0x400,0x400)
>      BLK0: Alias(s):
>           /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> Press ESC in 4 seconds to skip startup.nsh or any other key to continue.
> Shell> fs0:
> FS0:\> cat description.txt
> file system on partition 1
> 
> With your patch:
> 
> UEFI Interactive Shell v2.2
> EDK II
> UEFI v2.80 (Das U-Boot, 0x20191000)
> Mapping table
>       FS0: Alias(s):F0a0:;BLK0:
>           /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>       FS1: Alias(s):HD0a0b:;BLK1:
> 
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(1,MBR,0x6fe3a999,0x400,0x400)
> Press ESC in 4 seconds to skip startup.nsh or any other key to continue.
> Shell> fs0:
> FS0:\> cat description.txt
> file system on block device
> 
> FS0:\> fs1:
> FS1:\> cat description.txt
> file system on partition 1
> 
> So though a partition table is discovered a file system is mounted on
> the block device. In my special case the file system on the block device
> really existed and was well separated from partition 1. But typically
> expect that there is no file system on the block device if there is a
> partition table.
> 
> For your convenience I have uploaded the image file to
> https://github.com/U-Boot-EFI/test_file_system
> 
> Best regards
> 
> Heinrich
> 
> >  		diskobj->volume = efi_simple_file_system(desc, part,
> >  							 diskobj->dp);
> >  		ret = efi_add_protocol(&diskobj->header,
> >