[v1] read-only UDF file system support

[edk2] [PATCH 0/4] read-only UDF file system support

Posted by Paulo Alcantara 6 years, 8 months ago

Hi,

I'm posting this series again after ~3 years that introduces UDF file
system support in UEFI. Why? Because Laszlo (or Red Hat) seemed to be
interested in such support.

I started working on this driver just as an excuse to learn UEFI
development at that time. This work isn't based on any previous one and
it's BSD licensed. I also *never* intended to replace it with the
default FAT file system. On the contrary, I was looking to give people
an opportunity to use file system features that current FAT file system
lacks.

This series was never reviewed or fully tested. I basically used Linux
and mkudffs[1] to test different UDF disks, as well as booting a Linux
(EFI stub) rootfs from UDF file systems. Please, I'd really appreciate
if some of one could help reviewing or testing it.

Note that UDF file system support was *only* added to OVMF platform and
it's disabled by default through UDF_ENABLE build option. There's also a
feature PCD flag that turns on or off parsing of UDF volumes during
partition discovery in PartitionDxe driver.

Branch: https://github.com/pcacjr/edk2/tree/udf-fs

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Mark Doran <mark.doran@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---

Paulo Alcantara (4):
  MdeModulePkg/PartitionDxe: Add UDF/ECMA-167 file system support
  MdeModulePkg: Initial UDF/ECMA-167 file system support
  MdeModulePkg/UdfDxe: Add seek, read and listing support on files
  OvmfPkg: Introduce UDF_ENABLE build flag

 MdeModulePkg/MdeModulePkg.dec                      |    6 +
 .../Universal/Disk/PartitionDxe/Partition.c        |    3 +-
 .../Universal/Disk/PartitionDxe/Partition.h        |   41 +-
 .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |   10 +-
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  335 +++
 MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c |  185 ++
 MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  901 +++++++
 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c      |  195 ++
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 2532 ++++++++++++++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  407 ++++
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1276 ++++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
 MdePkg/Include/IndustryStandard/Udf.h              |   78 +
 OvmfPkg/OvmfPkgIa32.dsc                            |    7 +
 OvmfPkg/OvmfPkgIa32.fdf                            |    3 +
 OvmfPkg/OvmfPkgIa32X64.dsc                         |    7 +
 OvmfPkg/OvmfPkgIa32X64.fdf                         |    3 +
 OvmfPkg/OvmfPkgX64.dsc                             |    7 +
 OvmfPkg/OvmfPkgX64.fdf                             |    3 +
 19 files changed, 6057 insertions(+), 8 deletions(-)
 create mode 100644 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/File.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
 create mode 100644 MdePkg/Include/IndustryStandard/Udf.h

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] read-only UDF file system support

Posted by Zeng, Star 6 years, 8 months ago

Cc Ray and Hao.

-----Original Message-----
From: Paulo Alcantara [mailto:pcacjr@zytor.com] 
Sent: Wednesday, August 9, 2017 3:32 AM
To: edk2-devel@lists.01.org
Cc: Paulo Alcantara <pcacjr@zytor.com>; Laszlo Ersek <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Doran, Mark <mark.doran@intel.com>
Subject: [PATCH 0/4] read-only UDF file system support

Hi,

I'm posting this series again after ~3 years that introduces UDF file system support in UEFI. Why? Because Laszlo (or Red Hat) seemed to be interested in such support.

I started working on this driver just as an excuse to learn UEFI development at that time. This work isn't based on any previous one and it's BSD licensed. I also *never* intended to replace it with the default FAT file system. On the contrary, I was looking to give people an opportunity to use file system features that current FAT file system lacks.

This series was never reviewed or fully tested. I basically used Linux and mkudffs[1] to test different UDF disks, as well as booting a Linux (EFI stub) rootfs from UDF file systems. Please, I'd really appreciate if some of one could help reviewing or testing it.

Note that UDF file system support was *only* added to OVMF platform and it's disabled by default through UDF_ENABLE build option. There's also a feature PCD flag that turns on or off parsing of UDF volumes during partition discovery in PartitionDxe driver.

Branch: https://github.com/pcacjr/edk2/tree/udf-fs

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Mark Doran <mark.doran@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---

Paulo Alcantara (4):
  MdeModulePkg/PartitionDxe: Add UDF/ECMA-167 file system support
  MdeModulePkg: Initial UDF/ECMA-167 file system support
  MdeModulePkg/UdfDxe: Add seek, read and listing support on files
  OvmfPkg: Introduce UDF_ENABLE build flag

 MdeModulePkg/MdeModulePkg.dec                      |    6 +
 .../Universal/Disk/PartitionDxe/Partition.c        |    3 +-
 .../Universal/Disk/PartitionDxe/Partition.h        |   41 +-
 .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |   10 +-
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  335 +++
 MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c |  185 ++
 MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  901 +++++++
 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c      |  195 ++
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 2532 ++++++++++++++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  407 ++++
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1276 ++++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
 MdePkg/Include/IndustryStandard/Udf.h              |   78 +
 OvmfPkg/OvmfPkgIa32.dsc                            |    7 +
 OvmfPkg/OvmfPkgIa32.fdf                            |    3 +
 OvmfPkg/OvmfPkgIa32X64.dsc                         |    7 +
 OvmfPkg/OvmfPkgIa32X64.fdf                         |    3 +
 OvmfPkg/OvmfPkgX64.dsc                             |    7 +
 OvmfPkg/OvmfPkgX64.fdf                             |    3 +
 19 files changed, 6057 insertions(+), 8 deletions(-)  create mode 100644 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/File.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
 create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
 create mode 100644 MdePkg/Include/IndustryStandard/Udf.h

--
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] read-only UDF file system support

Posted by Ni, Ruiyu 6 years, 8 months ago

Paulo,
Thanks for enabling the UDF support into EDKII.
Below are several comments:
1. Could you please separate the patch to modify MdePkg and MdeModulePkg?

2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID.
     Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?

2. Why do you need a PCD to control the UDF support? I prefer no. More choices
     is not always good😊

3.  Is gUdfVolumeSignatureGuid only used in Partition driver to produce the device
      path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least gUdfVolumeSignatureGuid
      can be removed, the GUID macro is enough?

4.  Have you verified on a CDROM which contains both ElTorito and CDFS?

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, August 9, 2017 9:18 AM
> To: Paulo Alcantara <pcacjr@zytor.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Andrew Fish <afish@apple.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Doran, Mark
> <mark.doran@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>
> Subject: RE: [PATCH 0/4] read-only UDF file system support
> 
> Cc Ray and Hao.
> 
> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Wednesday, August 9, 2017 3:32 AM
> To: edk2-devel@lists.01.org
> Cc: Paulo Alcantara <pcacjr@zytor.com>; Laszlo Ersek <lersek@redhat.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Andrew Fish
> <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Doran, Mark <mark.doran@intel.com>
> Subject: [PATCH 0/4] read-only UDF file system support
> 
> Hi,
> 
> I'm posting this series again after ~3 years that introduces UDF file system
> support in UEFI. Why? Because Laszlo (or Red Hat) seemed to be interested
> in such support.
> 
> I started working on this driver just as an excuse to learn UEFI development
> at that time. This work isn't based on any previous one and it's BSD licensed. I
> also *never* intended to replace it with the default FAT file system. On the
> contrary, I was looking to give people an opportunity to use file system
> features that current FAT file system lacks.
> 
> This series was never reviewed or fully tested. I basically used Linux and
> mkudffs[1] to test different UDF disks, as well as booting a Linux (EFI stub)
> rootfs from UDF file systems. Please, I'd really appreciate if some of one
> could help reviewing or testing it.
> 
> Note that UDF file system support was *only* added to OVMF platform and
> it's disabled by default through UDF_ENABLE build option. There's also a
> feature PCD flag that turns on or off parsing of UDF volumes during partition
> discovery in PartitionDxe driver.
> 
> Branch: https://github.com/pcacjr/edk2/tree/udf-fs
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Mark Doran <mark.doran@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> ---
> 
> Paulo Alcantara (4):
>   MdeModulePkg/PartitionDxe: Add UDF/ECMA-167 file system support
>   MdeModulePkg: Initial UDF/ECMA-167 file system support
>   MdeModulePkg/UdfDxe: Add seek, read and listing support on files
>   OvmfPkg: Introduce UDF_ENABLE build flag
> 
>  MdeModulePkg/MdeModulePkg.dec                      |    6 +
>  .../Universal/Disk/PartitionDxe/Partition.c        |    3 +-
>  .../Universal/Disk/PartitionDxe/Partition.h        |   41 +-
>  .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |   10 +-
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  335 +++
>  MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c |  185 ++
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  901 +++++++
>  MdeModulePkg/Universal/Disk/UdfDxe/FileName.c      |  195 ++
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 2532
> ++++++++++++++++++++
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  407 ++++
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1276 ++++++++++
>  MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
>  MdePkg/Include/IndustryStandard/Udf.h              |   78 +
>  OvmfPkg/OvmfPkgIa32.dsc                            |    7 +
>  OvmfPkg/OvmfPkgIa32.fdf                            |    3 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                         |    7 +
>  OvmfPkg/OvmfPkgIa32X64.fdf                         |    3 +
>  OvmfPkg/OvmfPkgX64.dsc                             |    7 +
>  OvmfPkg/OvmfPkgX64.fdf                             |    3 +
>  19 files changed, 6057 insertions(+), 8 deletions(-)  create mode 100644
> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
>  create mode 100644
> MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c
>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/File.c
>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
>  create mode 100644
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
>  create mode 100644 MdePkg/Include/IndustryStandard/Udf.h
> 
> --
> 2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] read-only UDF file system support

Posted by Paulo Alcantara 6 years, 8 months ago

Hi Ray,

Thanks for the review. My comments below.

On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
> Paulo,
> Thanks for enabling the UDF support into EDKII.
> Below are several comments:
> 1. Could you please separate the patch to modify MdePkg and MdeModulePkg?

Sure.

> 2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID.
>       Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?

The Volume Identifier structure is defined in ECMA-167 specification 
(not UDF specific), so perhaps it would be better if we create a 
separate header file (e.g., Ecma-167.h) and define 
ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito 
and UDF codes. What do you think?

> 
> 2. Why do you need a PCD to control the UDF support? I prefer no. More choices
>       is not always good😊

The original idea of including this PCD was to avoid adding unnecessary 
overhead in PartitionDxe driver to something (UDF) that wasn't actually 
defined in UEFI specification -- leaving it as an optional feature.

But yes, I agree with you that just complicates things and would be 
better to remove it.

> 
> 3.  Is gUdfVolumeSignatureGuid only used in Partition driver to produce the device
>        path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least gUdfVolumeSignatureGuid
>        can be removed, the GUID macro is enough?
> 
> 4.  Have you verified on a CDROM which contains both ElTorito and CDFS?

In the past I tested it with some Windows 8 and 10 ISO images I grabbed 
somewhere (both had ISO9660 & ElTorito's boot catalogs) but I haven't 
verified if it *still* works with them, yet. I'll do it when I get a chance.

Thanks,
Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] read-only UDF file system support

Posted by Paulo Alcantara 6 years, 8 months ago

(heh - forgot to answer your question about the GUID :-) )

On 8/9/2017 10:26 AM, Paulo Alcantara wrote:
> Hi Ray,
> 
> Thanks for the review. My comments below.
> 
> On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
>> Paulo,
>> Thanks for enabling the UDF support into EDKII.
>> Below are several comments:
>> 1. Could you please separate the patch to modify MdePkg and MdeModulePkg?
> 
> Sure.
> 
>> 2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID.
>>       Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?
> 
> The Volume Identifier structure is defined in ECMA-167 specification 
> (not UDF specific), so perhaps it would be better if we create a 
> separate header file (e.g., Ecma-167.h) and define 
> ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito 
> and UDF codes. What do you think?
> 
>>
>> 2. Why do you need a PCD to control the UDF support? I prefer no. More 
>> choices
>>       is not always good😊
> 
> The original idea of including this PCD was to avoid adding unnecessary 
> overhead in PartitionDxe driver to something (UDF) that wasn't actually 
> defined in UEFI specification -- leaving it as an optional feature.
> 
> But yes, I agree with you that just complicates things and would be 
> better to remove it.
> 
>>
>> 3.  Is gUdfVolumeSignatureGuid only used in Partition driver to 
>> produce the device
>>        path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least 
>> gUdfVolumeSignatureGuid
>>        can be removed, the GUID macro is enough?

The GUID is used in both Partition and UdfDxe drivers. Do you mean that 
we should use HARDDRIVE_DEVICE_PATH and then appending the UDF-specific 
GUID to it?

Thanks,
Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] read-only UDF file system support

Posted by Ni, Ruiyu 6 years, 8 months ago

Regards,
Ray

>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Wednesday, August 9, 2017 10:01 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Dong, Eric <eric.dong@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>Laszlo Ersek <lersek@redhat.com>
>Subject: Re: [edk2] [PATCH 0/4] read-only UDF file system support
>
>(heh - forgot to answer your question about the GUID :-) )
>
>On 8/9/2017 10:26 AM, Paulo Alcantara wrote:
>> Hi Ray,
>>
>> Thanks for the review. My comments below.
>>
>> On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
>>> Paulo,
>>> Thanks for enabling the UDF support into EDKII.
>>> Below are several comments:
>>> 1. Could you please separate the patch to modify MdePkg and MdeModulePkg?
>>
>> Sure.
>>
>>> 2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID.
>>>       Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?
>>
>> The Volume Identifier structure is defined in ECMA-167 specification
>> (not UDF specific), so perhaps it would be better if we create a
>> separate header file (e.g., Ecma-167.h) and define
>> ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito
>> and UDF codes. What do you think?
>>
>>>
>>> 2. Why do you need a PCD to control the UDF support? I prefer no. More
>>> choices
>>>       is not always good😊
>>
>> The original idea of including this PCD was to avoid adding unnecessary
>> overhead in PartitionDxe driver to something (UDF) that wasn't actually
>> defined in UEFI specification -- leaving it as an optional feature.
>>
>> But yes, I agree with you that just complicates things and would be
>> better to remove it.
>>
>>>
>>> 3.  Is gUdfVolumeSignatureGuid only used in Partition driver to
>>> produce the device
>>>        path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least
>>> gUdfVolumeSignatureGuid
>>>        can be removed, the GUID macro is enough?
>
>The GUID is used in both Partition and UdfDxe drivers. Do you mean that
>we should use HARDDRIVE_DEVICE_PATH and then appending the UDF-specific
>GUID to it?

I originally thought UdfDxe driver needs to use this GUID to know it's a UDF partition.
But later I found UdfDxe contains a function SupportUdfFileSystem() to parse the
partition content to decide whether to manage that partition.
I wasn't able to find the reference of this GUID.

I just tried again, still didn't find it. But I did find some improper implementation of
driver-model logic.
I saw UdfDxe uses OPEN_PROTOCOL_GET to open the BlockIo and DiskIo.
It should use OPEN_PROTOCOL_BY_DRIVER.
In details, it should firstly try OPEN_BY_DRIVER to open the BlockIo and DiskIo in
Supported(), when either one fails, the Supported() returns failure. But please
keep in mind to close the successfully-opened BlockIo/DiskIo. Supported() is a
test function called by DxeCore, and it should not alter the system state.
Start() should then repeat the same open logic as that in Supported(), but it's
find to use ASSERT_EFI_ERROR() to assert the return status of OPEN_BY_DRIVER
is EFI_SUCCESS because per UEFI spec, Start() should only be called when Supported()
returns EFI_SUCCESS. Start() will then install the SimpleFileSystem instance on the
same Handle that have BlockIo/DiskIo installed. (I noticed that your driver creates
a new handle for SimpleFileSystem, which is unnecessary and may break the driver
model chain.)

The OPEN_BY_DRIVER open is necessary because DxeCore driver model core logic
records the special open operation. and when BlockIo or DiskIo is uninstalled,
the Stop() function whose corresponding Start() is opening the same BlockIo/DiskIo
OPEN_BY_DRIVER will get called. So you can treat OPEN_BY_DRIVER is a mechanism
to notify the upper layer driver to destroy the newly created service(SimpleFileSystem)
when lower layer services (BlockIo/DiskIo) it layers on is destroyed (Uninstalled).

Not sure if I explained well. You could refer to
https://github.com/tianocore/tianocore.github.io/wiki/UEFI-Driver-Writer's-Guide
written by Kinney Michael for detailed instructions.

>
>Thanks,
>Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] read-only UDF file system support

Posted by Paulo Alcantara 6 years, 8 months ago

Hi,

(sorry for the late reply)

On 09/08/2017 22:11, Ni, Ruiyu wrote:
> 
> 
> Regards,
> Ray
> 
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Wednesday, August 9, 2017 10:01 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>> Cc: Dong, Eric <eric.dong@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Laszlo Ersek <lersek@redhat.com>
>> Subject: Re: [edk2] [PATCH 0/4] read-only UDF file system support
>>
>> (heh - forgot to answer your question about the GUID :-) )
>>
>> On 8/9/2017 10:26 AM, Paulo Alcantara wrote:
>>> Hi Ray,
>>>
>>> Thanks for the review. My comments below.
>>>
>>> On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
>>>> Paulo,
>>>> Thanks for enabling the UDF support into EDKII.
>>>> Below are several comments:
>>>> 1. Could you please separate the patch to modify MdePkg and MdeModulePkg?
>>>
>>> Sure.
>>>
>>>> 2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID.
>>>>        Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?
>>>
>>> The Volume Identifier structure is defined in ECMA-167 specification
>>> (not UDF specific), so perhaps it would be better if we create a
>>> separate header file (e.g., Ecma-167.h) and define
>>> ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito
>>> and UDF codes. What do you think?
>>>
>>>>
>>>> 2. Why do you need a PCD to control the UDF support? I prefer no. More
>>>> choices
>>>>        is not always good😊
>>>
>>> The original idea of including this PCD was to avoid adding unnecessary
>>> overhead in PartitionDxe driver to something (UDF) that wasn't actually
>>> defined in UEFI specification -- leaving it as an optional feature.
>>>
>>> But yes, I agree with you that just complicates things and would be
>>> better to remove it.
>>>
>>>>
>>>> 3.  Is gUdfVolumeSignatureGuid only used in Partition driver to
>>>> produce the device
>>>>         path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least
>>>> gUdfVolumeSignatureGuid
>>>>         can be removed, the GUID macro is enough?
>>
>> The GUID is used in both Partition and UdfDxe drivers. Do you mean that
>> we should use HARDDRIVE_DEVICE_PATH and then appending the UDF-specific
>> GUID to it?
> 
> I originally thought UdfDxe driver needs to use this GUID to know it's a UDF partition.
> But later I found UdfDxe contains a function SupportUdfFileSystem() to parse the
> partition content to decide whether to manage that partition.
> I wasn't able to find the reference of this GUID.

Yes - you're right. I think it should just check for the installed GUID, 
indeed. So we don't have to parse and look for UDF file systems twice.

> 
> I just tried again, still didn't find it. But I did find some improper implementation of
> driver-model logic.
> I saw UdfDxe uses OPEN_PROTOCOL_GET to open the BlockIo and DiskIo.
> It should use OPEN_PROTOCOL_BY_DRIVER.
> In details, it should firstly try OPEN_BY_DRIVER to open the BlockIo and DiskIo in
> Supported(), when either one fails, the Supported() returns failure. But please
> keep in mind to close the successfully-opened BlockIo/DiskIo. Supported() is a
> test function called by DxeCore, and it should not alter the system state.
> Start() should then repeat the same open logic as that in Supported(), but it's
> find to use ASSERT_EFI_ERROR() to assert the return status of OPEN_BY_DRIVER
> is EFI_SUCCESS because per UEFI spec, Start() should only be called when Supported()
> returns EFI_SUCCESS. Start() will then install the SimpleFileSystem instance on the
> same Handle that have BlockIo/DiskIo installed. (I noticed that your driver creates
> a new handle for SimpleFileSystem, which is unnecessary and may break the driver
> model chain.)
> 
> The OPEN_BY_DRIVER open is necessary because DxeCore driver model core logic
> records the special open operation. and when BlockIo or DiskIo is uninstalled,
> the Stop() function whose corresponding Start() is opening the same BlockIo/DiskIo
> OPEN_BY_DRIVER will get called. So you can treat OPEN_BY_DRIVER is a mechanism
> to notify the upper layer driver to destroy the newly created service(SimpleFileSystem)
> when lower layer services (BlockIo/DiskIo) it layers on is destroyed (Uninstalled).
> 
> Not sure if I explained well. You could refer to
> https://github.com/tianocore/tianocore.github.io/wiki/UEFI-Driver-Writer's-Guide
> written by Kinney Michael for detailed instructions.

Good catch! You explained it very well. I'll fix that up in the next series.

Thanks!
Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel