[U-Boot] [PATCH v4 0/5] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk

[U-Boot] [PATCH v4 0/5] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk

[AKASHI Takahiro]

Changes in v4 (Oct 7, 2019)
* modify commit message of patch#1 after Heinrich's suggestion
* add patch#2 after Heinrich's suggestion
* add a function description to efi_fs_exists() after Heinrich's
  suggestion (patch#4)

Changes in v3 (Oct 4, 2019)
* add patch#1
* use newly-added fs_get_type() instead fo fs_get_type_name()
* check for FS_TYPE_ANY to confirm if a file system exists or not
* remove a redundant check on whether a device is a partition
  or a whole disk

Changes in v2 (Sept 12, 2019)
* add patch#1 and #2
* install the protocol only if a file system does exist

AKASHI Takahiro (5):
  fs: export fs_close()
  fs: clean up around fs_type
  fs: add fs_get_type() for current filesystem type
  efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available
  efi_loader: disk: install file system protocol to a whole disk

 fs/fs.c                   | 18 ++++++++++++++----
 include/fs.h              | 17 +++++++++++++++++
 lib/efi_loader/efi_disk.c | 25 ++++++++++++++++++++++++-
 3 files changed, 55 insertions(+), 5 deletions(-)

-- 
2.21.0

[U-Boot] [PATCH v4 1/5] fs: export fs_close()

[AKASHI Takahiro]

fs_close() closes the connection to a file system which opened with
either fs_set_blk_dev() or fs_set_dev_with_part(). Many file system
functions implicitly call fs_close(), e.g. fs_closedir(), fs_exist(),
fs_ln(), fs_ls(), fs_mkdir(), fs_read(), fs_size(), fs_write()
and fs_unlink().
So just export it.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fs.c      | 2 +-
 include/fs.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/fs.c b/fs/fs.c
index d8a4ced4698e..64ba25fea8bf 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -389,7 +389,7 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part)
 	return -1;
 }
 
-static void fs_close(void)
+void fs_close(void)
 {
 	struct fstype_info *info = fs_get_info(fs_type);
 
diff --git a/include/fs.h b/include/fs.h
index 7601b0343bcd..5a1244d57fd2 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -37,6 +37,13 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype);
  */
 int fs_set_blk_dev_with_part(struct blk_desc *desc, int part);
 
+/**
+ * fs_close() - Unset current block device and partition
+ *
+ * Should be paired with either fs_set_blk_dev() or fs_set_dev_with_part()
+ */
+void fs_close(void);
+
 /**
  * fs_get_type_name() - Get type of current filesystem
  *
-- 
2.21.0

[U-Boot] [PATCH v4 2/5] fs: clean up around fs_type

[AKASHI Takahiro]

fs_ls(), fs_mkdir() and fs_unlink() sets fs_type to FS_TYPE_ANY
explicitly, but it is redundant as they call fs_close().
So just remove those lines.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index 64ba25fea8bf..b17b76a46ae0 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -413,7 +413,6 @@ int fs_ls(const char *dirname)
 
 	ret = info->ls(dirname);
 
-	fs_type = FS_TYPE_ANY;
 	fs_close();
 
 	return ret;
@@ -597,7 +596,6 @@ int fs_unlink(const char *filename)
 
 	ret = info->unlink(filename);
 
-	fs_type = FS_TYPE_ANY;
 	fs_close();
 
 	return ret;
@@ -611,7 +609,6 @@ int fs_mkdir(const char *dirname)
 
 	ret = info->mkdir(dirname);
 
-	fs_type = FS_TYPE_ANY;
 	fs_close();
 
 	return ret;
-- 
2.21.0

[U-Boot] [PATCH v4 3/5] fs: add fs_get_type() for current filesystem type

[AKASHI Takahiro]

This function is a variant of fs_get_type_name() and returns a filesystem
type with which the current device is associated.
We don't want to export fs_type variable directly because we have to take
care of it consistently within fs.c.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 fs/fs.c      | 13 +++++++++++++
 include/fs.h | 10 ++++++++++
 2 files changed, 23 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index b17b76a46ae0..0c66d6047703 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -307,6 +307,19 @@ static struct fstype_info *fs_get_info(int fstype)
 	return info;
 }
 
+/**
+ * fs_get_type() - Get type of current filesystem
+ *
+ * Return: filesystem type
+ *
+ * Returns filesystem type representing the current filesystem, or
+ * FS_TYPE_ANY for any unrecognised filesystem.
+ */
+int fs_get_type(void)
+{
+	return fs_type;
+}
+
 /**
  * fs_get_type_name() - Get type of current filesystem
  *
diff --git a/include/fs.h b/include/fs.h
index 5a1244d57fd2..6dfdb5c5307a 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -44,6 +44,16 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part);
  */
 void fs_close(void);
 
+/**
+ * fs_get_type() - Get type of current filesystem
+ *
+ * Return: filesystem type
+ *
+ * Returns filesystem type representing the current filesystem, or
+ * FS_TYPE_ANY for any unrecognised filesystem.
+ */
+int fs_get_type(void);
+
 /**
  * fs_get_type_name() - Get type of current filesystem
  *
-- 
2.21.0

[U-Boot] [PATCH v4 4/5] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available

[AKASHI Takahiro]

In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always
installed to all the partitions even if some of them may house no file
system.

With this patch, that protocol will be installed only if any file system
exists.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_disk.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 9007a5f77f3d..861fcaf3747f 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -9,6 +9,7 @@
 #include <blk.h>
 #include <dm.h>
 #include <efi_loader.h>
+#include <fs.h>
 #include <part.h>
 #include <malloc.h>
 
@@ -262,6 +263,27 @@ efi_fs_from_path(struct efi_device_path *full_path)
 	return handler->protocol_interface;
 }
 
+/**
+ * efi_fs_exists() - check if a partition bears a file system
+ *
+ * @desc:	block device descriptor
+ * @part:	partition number
+ * Return:	1 if a file system exists on the partition
+ *		0 otherwise
+ */
+static int efi_fs_exists(struct blk_desc *desc, int part)
+{
+	if (fs_set_blk_dev_with_part(desc, part))
+		return 0;
+
+	if (fs_get_type() == FS_TYPE_ANY)
+		return 0;
+
+	fs_close();
+
+	return 1;
+}
+
 /*
  * Create a handle for a partition or disk
  *
@@ -315,7 +337,7 @@ static efi_status_t efi_disk_add_dev(
 			       diskobj->dp);
 	if (ret != EFI_SUCCESS)
 		return ret;
-	if (part >= 1) {
+	if (part >= 1 && efi_fs_exists(desc, part)) {
 		diskobj->volume = efi_simple_file_system(desc, part,
 							 diskobj->dp);
 		ret = efi_add_protocol(&diskobj->header,
-- 
2.21.0

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

[AKASHI Takahiro]

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.

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)) {
 		diskobj->volume = efi_simple_file_system(desc, part,
 							 diskobj->dp);
 		ret = efi_add_protocol(&diskobj->header,
-- 
2.21.0

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

[Heinrich Schuchardt]

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.
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

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,
>

[U-Boot] [PATCH v4 2/5] fs: clean up around fs_type

[Heinrich Schuchardt]

On 10/7/19 7:59 AM, AKASHI Takahiro wrote:
> fs_ls(), fs_mkdir() and fs_unlink() sets fs_type to FS_TYPE_ANY
> explicitly, but it is redundant as they call fs_close().
> So just remove those lines.
>
> Signed-off-by: AKASHI Takahiro<takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH v4 1/5] fs: export fs_close()

[Heinrich Schuchardt]

On 10/7/19 7:59 AM, AKASHI Takahiro wrote:
> fs_close() closes the connection to a file system which opened with
> either fs_set_blk_dev() or fs_set_dev_with_part(). Many file system
> functions implicitly call fs_close(), e.g. fs_closedir(), fs_exist(),
> fs_ln(), fs_ls(), fs_mkdir(), fs_read(), fs_size(), fs_write()
> and fs_unlink().
> So just export it.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   fs/fs.c      | 2 +-
>   include/fs.h | 7 +++++++
>   2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index d8a4ced4698e..64ba25fea8bf 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -389,7 +389,7 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part)
>   	return -1;
>   }
>
> -static void fs_close(void)
> +void fs_close(void)
>   {
>   	struct fstype_info *info = fs_get_info(fs_type);
>
> diff --git a/include/fs.h b/include/fs.h
> index 7601b0343bcd..5a1244d57fd2 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -37,6 +37,13 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype);
>    */
>   int fs_set_blk_dev_with_part(struct blk_desc *desc, int part);
>
> +/**
> + * fs_close() - Unset current block device and partition
> + *
> + * Should be paired with either fs_set_blk_dev() or fs_set_dev_with_part()
> + */
> +void fs_close(void);
> +
>   /**
>    * fs_get_type_name() - Get type of current filesystem
>    *
>

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

[AKASHI Takahiro]

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,
> >

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

[Heinrich Schuchardt]

On 10/15/19 9:34 AM, AKASHI Takahiro wrote:
> 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)) {

The following should work:

         if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
             efi_fs_exists(desc, part)) {

But unfortunately it does not because if you format the image with
mkfs.vfat desc->part_type will be set to PART_TYPE_DOS.

So what is missing is a fix in disk/part_dos.c. Currently it only checks
for bytes 0x55 and 0xAA.

One might want to check additionally if there is either a disk
identifier at position 0x01B8 or one of the type bytes of the four
primary partitions is set. And if neither is set return -1 from
test_block_type().

Best regards

Heinrich

>>
>> 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,
>>>
>

[U-Boot] [BUG] efi_driver: crash while reading from iSCSI drive

[Heinrich Schuchardt]

The patch

commit 867400677cda0fac4a411f1549fe3a61bb5ed172
efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available

breaks booting my Pine A64 LTS board via iPXE and GRUB. But I assume
this is not at the base of the problem.

My iSCSI drive is partitioned like this:

Device        Boot    Start      End  Sectors  Size Id Type
pine-a64-lts1          2048   194559   192512   94M ef EFI   vfat
pine-a64-lts2 *      194560  2148351  1953792  954M 83 Linux ext2
pine-a64-lts3       2148352 25585663 23437312 11.2G 83 Linux ext4
pine-a64-lts4      25585664 67106815 41521152 19.8G 83 Linux ext4

Looking at the debug output below the following questions arise:

Why is ext2 not recognized as a file system?
Why is the system crashing when trying to read 1024 blocks from the ext4
partition?

.config contains CONFIG_FS_EXT4=y

EFI: efi_bl_bind: handle 00000000b9f94560, interface 00000000b8e9f7f8
EFI: efi_bl_read: read 'efiblk#0', from block 0, 1 blocks
EFI: Call: io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: 0 returned by io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: efi_bl_read: r = 0
EFI: efi_bl_bind: block device 'efiblk#0' created
EFI: efi_bl_read: read 'efiblk#0', from block 2048, 1 blocks
EFI: Call: io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: 0 returned by io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: efi_bl_read: r = 0
lib/efi_loader/efi_disk.c(279) efi_fs_exists: 0 returned by
fs_set_blk_dev_with_part(desc, 1)
EFI: efi_bl_read: read 'efiblk#0', from block 194560, 1 blocks
EFI: Call: io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: 0 returned by io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: efi_bl_read: r = 0
EFI: efi_bl_read: read 'efiblk#0', from block 195584, 1024 blocks
EFI: Call: io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: 0 returned by io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: efi_bl_read: r = 0
** Unrecognized filesystem type **
lib/efi_loader/efi_disk.c(279) efi_fs_exists: -1 returned by
fs_set_blk_dev_with_part(desc, 2)
EFI: efi_bl_read: read 'efiblk#0', from block 2148352, 1 blocks
EFI: Call: io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: 0 returned by io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)
EFI: efi_bl_read: r = 0
EFI: efi_bl_read: read 'efiblk#0', from block 2149376, 1024 blocks
EFI: Call: io->read_blocks( io, io->media->media_id, (u64)blknr,
(efi_uintn_t)blkcnt * (efi_uintn_t)io->media->block_size, buffer)

"Synchronous Abort" handler, esr 0x02000000

elr: ffffffff8c0a6028 lr : ffffffff8c0a6000 (reloc)
elr: 0000000000000028 lr : 0000000000000000
x0 : 00000000b9f918f8 x1 : 00000000b9f31004
x2 : 00000000b9f918f8 x3 : 00000000b9f918f8
x4 : 00000000b9f918f8 x5 : 00000000b9f918f8
x6 : 00000000b9f918f8 x7 : 0000000000000000
x8 : 00000000b9f918f8 x9 : 00000000b8ea1038
x10: 0000000000000001 x11: 0000000019100bb0
x12: 0000000000000000 x13: 0000000000000001
x14: 0000000000000002 x15: 0000000000000003
x16: 00000000000000c8 x17: 00000000b9f918f8
x18: 00000000b9f31c30 x19: 00000000b9f918f8
x20: 00000000b8e9ddd0 x21: 00000000b8e99000
x22: 00000000b9f31008 x23: 00000000b9f3105a
x24: 00000000b9f31068 x25: 00000000b9f31110
x26: 0000000000000000 x27: 0000000000000000
x28: 0000000000000000 x29: 00000000b9f31160

Code: ea000011 ea000000 ea000013 eafffffe (e3a00001)

Looks like we jumped into nowhere land:

All code
========
    0:   ea000011        ands    x17, x0, x0
    4:   ea000000        ands    x0, x0, x0
    8:   ea000013        ands    x19, x0, x0
    c:   eafffffe        bics    x30, xzr, xzr, ror #63
   10:*  e3a00001        .inst   0xe3a00001 ; undefined          <--
trapping instruction

Best regards

Heinrich

[U-Boot] [BUG] efi_driver: crash while reading from iSCSI drive

[AKASHI Takahiro]

On Tue, Oct 22, 2019 at 10:29:09PM +0200, Heinrich Schuchardt wrote:
> The patch
> 
> commit 867400677cda0fac4a411f1549fe3a61bb5ed172
> efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available
> 
> breaks booting my Pine A64 LTS board via iPXE and GRUB. But I assume
> this is not at the base of the problem.
> 
> My iSCSI drive is partitioned like this:
> 
> Device        Boot    Start      End  Sectors  Size Id Type
> pine-a64-lts1          2048   194559   192512   94M ef EFI   vfat
> pine-a64-lts2 *      194560  2148351  1953792  954M 83 Linux ext2
> pine-a64-lts3       2148352 25585663 23437312 11.2G 83 Linux ext4
> pine-a64-lts4      25585664 67106815 41521152 19.8G 83 Linux ext4
> 
> Looking at the debug output below the following questions arise:
> 
> Why is ext2 not recognized as a file system?
> Why is the system crashing when trying to read 1024 blocks from the ext4
> partition?

Try the workaround attached below.
It seems that some fields, particularly log2blksz, in blk_dev held by
ext_fs(of ext_filesystem in fs/ext4/ext4fs.c) are not initialized.

I think that ext4's initialization code should be reworked.

-Takahiro Akashi

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 861fcaf3747f..0792e53b32c6 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -337,6 +337,12 @@ static efi_status_t efi_disk_add_dev(
 			       diskobj->dp);
 	if (ret != EFI_SUCCESS)
 		return ret;
+	if (!part) {
+		char buf[10];
+
+		sprintf(buf, "%d:%d", dev_index, part);
+		fs_set_blk_dev(if_typename, buf, FS_TYPE_ANY);
+	}
 	if (part >= 1 && efi_fs_exists(desc, part)) {
 		diskobj->volume = efi_simple_file_system(desc, part,
 							 diskobj->dp);

[U-Boot] [BUG] efi_driver: crash while reading from iSCSI drive

[Heinrich Schuchardt]

On 10/23/19 12:30 PM, AKASHI Takahiro wrote:
> On Tue, Oct 22, 2019 at 10:29:09PM +0200, Heinrich Schuchardt wrote:
>> The patch
>>
>> commit 867400677cda0fac4a411f1549fe3a61bb5ed172
>> efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available
>>
>> breaks booting my Pine A64 LTS board via iPXE and GRUB. But I assume
>> this is not at the base of the problem.
>>
>> My iSCSI drive is partitioned like this:
>>
>> Device        Boot    Start      End  Sectors  Size Id Type
>> pine-a64-lts1          2048   194559   192512   94M ef EFI   vfat
>> pine-a64-lts2 *      194560  2148351  1953792  954M 83 Linux ext2
>> pine-a64-lts3       2148352 25585663 23437312 11.2G 83 Linux ext4
>> pine-a64-lts4      25585664 67106815 41521152 19.8G 83 Linux ext4
>>
>> Looking at the debug output below the following questions arise:
>>
>> Why is ext2 not recognized as a file system?
>> Why is the system crashing when trying to read 1024 blocks from the ext4
>> partition?
>
> Try the workaround attached below.
> It seems that some fields, particularly log2blksz, in blk_dev held by
> ext_fs(of ext_filesystem in fs/ext4/ext4fs.c) are not initialized.
>
> I think that ext4's initialization code should be reworked.

Thanks for looking into this.

The error is in efi_bl_bind() (lib/efi_driver/efi_block_device.c). I
missed to use the block size of the block IO protocol to initialize
desc->blksz and desc->log2blksz.

Our FAT driver takes the sector size from the boot sector in
get_fs_info() (fs/fat/fat.c) and ignores the block descriptor which will
lead to errors if the logical sector size does not match the physical
sector size.

Best regards

Heinrich