[v2] read-only UDF file system support

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

Posted by Paulo Alcantara 7 years, 4 months ago

Hi,

This series introduces read-only UDF file system support in EDK2. As
Laszlo (or Red Hat) seemed to be interested in such support, I'm posting
it again after ~3 years.

The idea is not replacing the default FAT file system, nor breaking any
existing file system support, but extending EDK2 with a new file system
that might be useful for some people who are looking for specific file
system features that current FAT doesn't support.

Originally the driver was written to support UDF file systems as
specified by OSTA Universal Disk Format Specification 2.60. However,
some Windows 10 Enterprise ISO (UDF bridge) images that I tested
supported a revision of 1.02 thus I had to rework the driver a little
bit to support such revision as well.

v2:
 - Rework to _partially_ support UDF revisions <2.60.
 - Use existing CDROM_VOLUME_DESCRIPTOR structure defined in Eltorito.h
   instead of creating another one (UDF_VOLUME_DESCRIPTOR).
 - Fixed UdfDxe to correctly follow UEFI driver model.
 - Use HARDDRIVE_DEVICE_PATH instead of a vendor-defined one.
 - Detect UDF file systems only in PartitionDxe, and let UdfDxe driver
   check for specific UDF device path to decide whether or not install
   SimpleFs protocol.
 - Place MdePkg changes in a separate patch.

Repo:   https://github.com/pcacjr/edk2.git
Branch: 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>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: hao.a.wu@intel.com
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---

Paulo Alcantara (6):
  MdePkg: Add UDF volume structure definitions
  MdeModulePkg/PartitionDxe: Add UDF file system support
  MdeModulePkg: Initial UDF/ECMA-167 file system support
  OvmfPkg: Enable UDF file system support
  ArmVirtPkg: Enable UDF file system support
  Nt32Pkg: Enable UDF file system support

 ArmVirtPkg/ArmVirtQemu.dsc                         |    3 +-
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc               |    3 +-
 ArmVirtPkg/ArmVirtQemuKernel.dsc                   |    3 +-
 ArmVirtPkg/ArmVirtXen.dsc                          |    3 +-
 ArmVirtPkg/ArmVirtXen.fdf                          |    1 +
 .../Universal/Disk/PartitionDxe/Partition.c        |    9 +-
 .../Universal/Disk/PartitionDxe/Partition.h        |   32 +-
 .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |    3 +-
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  307 +++
 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   | 2375 ++++++++++++++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  415 ++++
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1246 ++++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
 MdePkg/Include/IndustryStandard/Udf.h              |   60 +
 Nt32Pkg/Nt32Pkg.dsc                                |    1 +
 Nt32Pkg/Nt32Pkg.fdf                                |    1 +
 OvmfPkg/OvmfPkgIa32.dsc                            |    1 +
 OvmfPkg/OvmfPkgIa32.fdf                            |    1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                         |    1 +
 OvmfPkg/OvmfPkgIa32X64.fdf                         |    1 +
 OvmfPkg/OvmfPkgX64.dsc                             |    1 +
 OvmfPkg/OvmfPkgX64.fdf                             |    1 +
 25 files changed, 5804 insertions(+), 11 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 v2 0/6] read-only UDF file system support

Posted by Ni, Ruiyu 7 years, 4 months ago

Paulo,
1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
  "CRC" might need to be replaced with "Crc".
  I also noticed some TAB key in file content.

2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
  But with:
     SignatureType = SIGNATURE_TYPE_UDF
     MBRType = MBR_TYPE_PCAT
     Signature = *
  And later UdfDxe driver checks the SignatureType and MBRType.
  I am not sure if it would be better to put the definitions in UEFI Spec,
  since they are referenced by different modules.
  I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
  When proposing to UEFI Spec, this also needs to be considered,
  for example, add PARTITION_TYPE_UDF to spec.

3. The driver model part looks good.

Regards,
Ray

>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Monday, August 21, 2017 2:16 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>; Ni, Ruiyu
><ruiyu.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>Subject: [PATCH v2 0/6] read-only UDF file system support
>
>Hi,
>
>This series introduces read-only UDF file system support in EDK2. As
>Laszlo (or Red Hat) seemed to be interested in such support, I'm posting
>it again after ~3 years.
>
>The idea is not replacing the default FAT file system, nor breaking any
>existing file system support, but extending EDK2 with a new file system
>that might be useful for some people who are looking for specific file
>system features that current FAT doesn't support.
>
>Originally the driver was written to support UDF file systems as
>specified by OSTA Universal Disk Format Specification 2.60. However,
>some Windows 10 Enterprise ISO (UDF bridge) images that I tested
>supported a revision of 1.02 thus I had to rework the driver a little
>bit to support such revision as well.
>
>v2:
> - Rework to _partially_ support UDF revisions <2.60.
> - Use existing CDROM_VOLUME_DESCRIPTOR structure defined in Eltorito.h
>   instead of creating another one (UDF_VOLUME_DESCRIPTOR).
> - Fixed UdfDxe to correctly follow UEFI driver model.
> - Use HARDDRIVE_DEVICE_PATH instead of a vendor-defined one.
> - Detect UDF file systems only in PartitionDxe, and let UdfDxe driver
>   check for specific UDF device path to decide whether or not install
>   SimpleFs protocol.
> - Place MdePkg changes in a separate patch.
>
>Repo:   https://github.com/pcacjr/edk2.git
>Branch: 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>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: hao.a.wu@intel.com
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>---
>
>Paulo Alcantara (6):
>  MdePkg: Add UDF volume structure definitions
>  MdeModulePkg/PartitionDxe: Add UDF file system support
>  MdeModulePkg: Initial UDF/ECMA-167 file system support
>  OvmfPkg: Enable UDF file system support
>  ArmVirtPkg: Enable UDF file system support
>  Nt32Pkg: Enable UDF file system support
>
> ArmVirtPkg/ArmVirtQemu.dsc                         |    3 +-
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc               |    3 +-
> ArmVirtPkg/ArmVirtQemuKernel.dsc                   |    3 +-
> ArmVirtPkg/ArmVirtXen.dsc                          |    3 +-
> ArmVirtPkg/ArmVirtXen.fdf                          |    1 +
> .../Universal/Disk/PartitionDxe/Partition.c        |    9 +-
> .../Universal/Disk/PartitionDxe/Partition.h        |   32 +-
> .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |    3 +-
> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  307 +++
> 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   | 2375 ++++++++++++++++++++
> MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  415 ++++
> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1246 ++++++++++
> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
> MdePkg/Include/IndustryStandard/Udf.h              |   60 +
> Nt32Pkg/Nt32Pkg.dsc                                |    1 +
> Nt32Pkg/Nt32Pkg.fdf                                |    1 +
> OvmfPkg/OvmfPkgIa32.dsc                            |    1 +
> OvmfPkg/OvmfPkgIa32.fdf                            |    1 +
> OvmfPkg/OvmfPkgIa32X64.dsc                         |    1 +
> OvmfPkg/OvmfPkgIa32X64.fdf                         |    1 +
> OvmfPkg/OvmfPkgX64.dsc                             |    1 +
> OvmfPkg/OvmfPkgX64.fdf                             |    1 +
> 25 files changed, 5804 insertions(+), 11 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 v2 0/6] read-only UDF file system support

Posted by Gao, Liming 7 years, 4 months ago

I suggest to run PatchCheck tool first. It is mentioned in https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

>-----Original Message-----
>From: Ni, Ruiyu
>Sent: Monday, August 21, 2017 10:29 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>; Zeng, Star <star.zeng@intel.com>; Dong, Eric
><eric.dong@intel.com>; Doran, Mark <mark.doran@intel.com>; Wu, Hao A
><hao.a.wu@intel.com>
>Subject: RE: [PATCH v2 0/6] read-only UDF file system support
>
>Paulo,
>1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
>  "CRC" might need to be replaced with "Crc".
>  I also noticed some TAB key in file content.
>
>2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
>  But with:
>     SignatureType = SIGNATURE_TYPE_UDF
>     MBRType = MBR_TYPE_PCAT
>     Signature = *
>  And later UdfDxe driver checks the SignatureType and MBRType.
>  I am not sure if it would be better to put the definitions in UEFI Spec,
>  since they are referenced by different modules.
>  I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
>  When proposing to UEFI Spec, this also needs to be considered,
>  for example, add PARTITION_TYPE_UDF to spec.
>
>3. The driver model part looks good.
>
>Regards,
>Ray
>
>>-----Original Message-----
>>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>>Sent: Monday, August 21, 2017 2:16 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>; Ni, Ruiyu
>><ruiyu.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>>Subject: [PATCH v2 0/6] read-only UDF file system support
>>
>>Hi,
>>
>>This series introduces read-only UDF file system support in EDK2. As
>>Laszlo (or Red Hat) seemed to be interested in such support, I'm posting
>>it again after ~3 years.
>>
>>The idea is not replacing the default FAT file system, nor breaking any
>>existing file system support, but extending EDK2 with a new file system
>>that might be useful for some people who are looking for specific file
>>system features that current FAT doesn't support.
>>
>>Originally the driver was written to support UDF file systems as
>>specified by OSTA Universal Disk Format Specification 2.60. However,
>>some Windows 10 Enterprise ISO (UDF bridge) images that I tested
>>supported a revision of 1.02 thus I had to rework the driver a little
>>bit to support such revision as well.
>>
>>v2:
>> - Rework to _partially_ support UDF revisions <2.60.
>> - Use existing CDROM_VOLUME_DESCRIPTOR structure defined in Eltorito.h
>>   instead of creating another one (UDF_VOLUME_DESCRIPTOR).
>> - Fixed UdfDxe to correctly follow UEFI driver model.
>> - Use HARDDRIVE_DEVICE_PATH instead of a vendor-defined one.
>> - Detect UDF file systems only in PartitionDxe, and let UdfDxe driver
>>   check for specific UDF device path to decide whether or not install
>>   SimpleFs protocol.
>> - Place MdePkg changes in a separate patch.
>>
>>Repo:   https://github.com/pcacjr/edk2.git
>>Branch: 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>
>>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>Cc: hao.a.wu@intel.com
>>Contributed-under: TianoCore Contribution Agreement 1.1
>>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>---
>>
>>Paulo Alcantara (6):
>>  MdePkg: Add UDF volume structure definitions
>>  MdeModulePkg/PartitionDxe: Add UDF file system support
>>  MdeModulePkg: Initial UDF/ECMA-167 file system support
>>  OvmfPkg: Enable UDF file system support
>>  ArmVirtPkg: Enable UDF file system support
>>  Nt32Pkg: Enable UDF file system support
>>
>> ArmVirtPkg/ArmVirtQemu.dsc                         |    3 +-
>> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc               |    3 +-
>> ArmVirtPkg/ArmVirtQemuKernel.dsc                   |    3 +-
>> ArmVirtPkg/ArmVirtXen.dsc                          |    3 +-
>> ArmVirtPkg/ArmVirtXen.fdf                          |    1 +
>> .../Universal/Disk/PartitionDxe/Partition.c        |    9 +-
>> .../Universal/Disk/PartitionDxe/Partition.h        |   32 +-
>> .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |    3 +-
>> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  307 +++
>> 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   | 2375
>++++++++++++++++++++
>> MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  415 ++++
>> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1246 ++++++++++
>> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
>> MdePkg/Include/IndustryStandard/Udf.h              |   60 +
>> Nt32Pkg/Nt32Pkg.dsc                                |    1 +
>> Nt32Pkg/Nt32Pkg.fdf                                |    1 +
>> OvmfPkg/OvmfPkgIa32.dsc                            |    1 +
>> OvmfPkg/OvmfPkgIa32.fdf                            |    1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc                         |    1 +
>> OvmfPkg/OvmfPkgIa32X64.fdf                         |    1 +
>> OvmfPkg/OvmfPkgX64.dsc                             |    1 +
>> OvmfPkg/OvmfPkgX64.fdf                             |    1 +
>> 25 files changed, 5804 insertions(+), 11 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 v2 0/6] read-only UDF file system support

Posted by Paulo Alcantara 7 years, 4 months ago

Hi,

On 8/20/2017 11:37 PM, Gao, Liming wrote:
> I suggest to run PatchCheck tool first. It is mentioned in https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Yes, I will! Thanks for remembering me that. I've also found some broken 
formatting all over the code. I'll send a v3 after fixing them up.

Thanks,
Paulo

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

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

Posted by Paulo Alcantara 7 years, 4 months ago

Hi,

I do apologize my late replies. At the moment, I'm only able to work on 
this during my free time. Thank you all for the reviews!

FWIW, my comments below.

On 8/20/2017 11:29 PM, Ni, Ruiyu wrote:
> Paulo,
> 1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
>    "CRC" might need to be replaced with "Crc".
>    I also noticed some TAB key in file content.

Sure.

> 
> 2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
>    But with:
>       SignatureType = SIGNATURE_TYPE_UDF
>       MBRType = MBR_TYPE_PCAT
>       Signature = *
>    And later UdfDxe driver checks the SignatureType and MBRType.
>    I am not sure if it would be better to put the definitions in UEFI Spec,
>    since they are referenced by different modules.
>    I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
>    When proposing to UEFI Spec, this also needs to be considered,
>    for example, add PARTITION_TYPE_UDF to spec.

Yes - I agree with you. My only concern is that UEFI specification 
doesn't either support UDF or there is any interest in supporting it, so 
by proposing an additional type for something that shouldn't be 
supported, might not work out.

(Andrew, any thoughts?)

"PARTITION_TYPE_OTHER" is also used when creating 
EFI_PARTITION_INFO_PROTOCOL for CDROM "El Torito" partitions, so either 
we could follow the same approach, or propose creating two other types: 
e.g., PARTITION_TYPE_ELTORITO and PARTITION_TYPE_UDF.

> 
> 3. The driver model part looks good.

Cool! Thanks for looking into that.

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

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

Posted by Andrew Fish 7 years, 4 months ago

> On Aug 22, 2017, at 6:14 AM, Paulo Alcantara <pcacjr@zytor.com> wrote:
> 
> Hi,
> 
> I do apologize my late replies. At the moment, I'm only able to work on this during my free time. Thank you all for the reviews!
> 
> FWIW, my comments below.
> 
> On 8/20/2017 11:29 PM, Ni, Ruiyu wrote:
>> Paulo,
>> 1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
>>   "CRC" might need to be replaced with "Crc".
>>   I also noticed some TAB key in file content.
> 
> Sure.
> 
>> 2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
>>   But with:
>>      SignatureType = SIGNATURE_TYPE_UDF
>>      MBRType = MBR_TYPE_PCAT
>>      Signature = *
>>   And later UdfDxe driver checks the SignatureType and MBRType.
>>   I am not sure if it would be better to put the definitions in UEFI Spec,
>>   since they are referenced by different modules.
>>   I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
>>   When proposing to UEFI Spec, this also needs to be considered,
>>   for example, add PARTITION_TYPE_UDF to spec.
> 
> Yes - I agree with you. My only concern is that UEFI specification doesn't either support UDF or there is any interest in supporting it, so by proposing an additional type for something that shouldn't be supported, might not work out.
> 
> (Andrew, any thoughts?)
> 

The enum space is owned by the UEFI spec and should never be extended outside the scope of the spec. Its not good to have an implementation running around that is not in a released spec, as it it is a future compatibility risk. 

Does the UDF actually start with a real MBR? If so is it possible to define a 32-bit MBR signature to indicate UDF. If not it should probably be a device path node like CD-ROM. 

You can all ways use the Vendor-Defined Media Device Path since it has GUID there is no risk of collision. 

Thanks,

Andrew Fish

> "PARTITION_TYPE_OTHER" is also used when creating EFI_PARTITION_INFO_PROTOCOL for CDROM "El Torito" partitions, so either we could follow the same approach, or propose creating two other types: e.g., PARTITION_TYPE_ELTORITO and PARTITION_TYPE_UDF.
> 
>> 3. The driver model part looks good.
> 
> Cool! Thanks for looking into that.
> 
> Paulo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

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

Posted by Paulo Alcantara 7 years, 4 months ago

Hi,

On 8/22/2017 2:21 PM, Andrew Fish wrote:
> 
>> On Aug 22, 2017, at 6:14 AM, Paulo Alcantara <pcacjr@zytor.com> wrote:
>>
>> Hi,
>>
>> I do apologize my late replies. At the moment, I'm only able to work on this during my free time. Thank you all for the reviews!
>>
>> FWIW, my comments below.
>>
>> On 8/20/2017 11:29 PM, Ni, Ruiyu wrote:
>>> Paulo,
>>> 1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
>>>    "CRC" might need to be replaced with "Crc".
>>>    I also noticed some TAB key in file content.
>>
>> Sure.
>>
>>> 2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
>>>    But with:
>>>       SignatureType = SIGNATURE_TYPE_UDF
>>>       MBRType = MBR_TYPE_PCAT
>>>       Signature = *
>>>    And later UdfDxe driver checks the SignatureType and MBRType.
>>>    I am not sure if it would be better to put the definitions in UEFI Spec,
>>>    since they are referenced by different modules.
>>>    I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
>>>    When proposing to UEFI Spec, this also needs to be considered,
>>>    for example, add PARTITION_TYPE_UDF to spec.
>>
>> Yes - I agree with you. My only concern is that UEFI specification doesn't either support UDF or there is any interest in supporting it, so by proposing an additional type for something that shouldn't be supported, might not work out.
>>
>> (Andrew, any thoughts?)
>>
> 
> The enum space is owned by the UEFI spec and should never be extended outside the scope of the spec. Its not good to have an implementation running around that is not in a released spec, as it it is a future compatibility risk.
> 
> Does the UDF actually start with a real MBR? If so is it possible to define a 32-bit MBR signature to indicate UDF. If not it should probably be a device path node like CD-ROM.

No. But it's possible to create an UDF file system inside a MBR/GPT 
partition so it would end up having a valid HARDDRIVE_DEVICE_PATH and no 
fake MBR device path wouldn't be created or needed at all.

The only problem is that when we find an UDF file system which isn't 
part of either a MBR or a GPT partition table, and then there is no 
matching device path for using with it.

> You can all ways use the Vendor-Defined Media Device Path since it has GUID there is no risk of collision.

Right. If you guys agree, I'll start using the vendor-defined device 
path again for all the cases where an UDF file system is found -- 
avoiding to break anything that's unrelated.

Thanks Andrew!

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

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

Posted by Andrew Fish 7 years, 4 months ago

> On Aug 22, 2017, at 10:56 AM, Paulo Alcantara <pcacjr@zytor.com> wrote:
> 
> Hi,
> 
> On 8/22/2017 2:21 PM, Andrew Fish wrote:
>>> On Aug 22, 2017, at 6:14 AM, Paulo Alcantara <pcacjr@zytor.com> wrote:
>>> 
>>> Hi,
>>> 
>>> I do apologize my late replies. At the moment, I'm only able to work on this during my free time. Thank you all for the reviews!
>>> 
>>> FWIW, my comments below.
>>> 
>>> On 8/20/2017 11:29 PM, Ni, Ruiyu wrote:
>>>> Paulo,
>>>> 1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
>>>>   "CRC" might need to be replaced with "Crc".
>>>>   I also noticed some TAB key in file content.
>>> 
>>> Sure.
>>> 
>>>> 2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
>>>>   But with:
>>>>      SignatureType = SIGNATURE_TYPE_UDF
>>>>      MBRType = MBR_TYPE_PCAT
>>>>      Signature = *
>>>>   And later UdfDxe driver checks the SignatureType and MBRType.
>>>>   I am not sure if it would be better to put the definitions in UEFI Spec,
>>>>   since they are referenced by different modules.
>>>>   I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
>>>>   When proposing to UEFI Spec, this also needs to be considered,
>>>>   for example, add PARTITION_TYPE_UDF to spec.
>>> 
>>> Yes - I agree with you. My only concern is that UEFI specification doesn't either support UDF or there is any interest in supporting it, so by proposing an additional type for something that shouldn't be supported, might not work out.
>>> 
>>> (Andrew, any thoughts?)
>>> 
>> The enum space is owned by the UEFI spec and should never be extended outside the scope of the spec. Its not good to have an implementation running around that is not in a released spec, as it it is a future compatibility risk.
>> Does the UDF actually start with a real MBR? If so is it possible to define a 32-bit MBR signature to indicate UDF. If not it should probably be a device path node like CD-ROM.
> 
> No. But it's possible to create an UDF file system inside a MBR/GPT partition so it would end up having a valid HARDDRIVE_DEVICE_PATH and no fake MBR device path wouldn't be created or needed at all.
> 
> The only problem is that when we find an UDF file system which isn't part of either a MBR or a GPT partition table, and then there is no matching device path for using with it.
> 

Most current file systems don't have device paths. CD-ROM is the exception, not the rule as it is an overlay of another file system type. 

Generally the MBR or GPT partition node just indicates a range of bytes on the disk. The file system driver starts looking at  byte 0 of the partition to find known structures to decide if it can start on the disk. For example this is the algorithm in the FAT driver: https://github.com/tianocore/edk2/blob/master/FatPkg/EnhancedFatDxe/Init.c#L198 <https://github.com/tianocore/edk2/blob/master/FatPkg/EnhancedFatDxe/Init.c#L198>.

I think this is kind of historical and makes the file system work on media with no partition, and media with partitioning the same way. 

Thanks,

Andrew Fish

>> You can all ways use the Vendor-Defined Media Device Path since it has GUID there is no risk of collision.
> 
> Right. If you guys agree, I'll start using the vendor-defined device path again for all the cases where an UDF file system is found -- avoiding to break anything that's unrelated.
> 
> Thanks Andrew!
> 
> Paulo

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

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

Posted by Laszlo Ersek 7 years, 4 months ago

On 08/22/17 19:21, Andrew Fish wrote:
> 
>> On Aug 22, 2017, at 6:14 AM, Paulo Alcantara <pcacjr@zytor.com> wrote:
>>
>> Hi,
>>
>> I do apologize my late replies. At the moment, I'm only able to work on this during my free time. Thank you all for the reviews!
>>
>> FWIW, my comments below.
>>
>> On 8/20/2017 11:29 PM, Ni, Ruiyu wrote:
>>> Paulo,
>>> 1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\)
>>>   "CRC" might need to be replaced with "Crc".
>>>   I also noticed some TAB key in file content.
>>
>> Sure.
>>
>>> 2. Your current implementation uses HARD_DRIVE_DEVICE_PATH.
>>>   But with:
>>>      SignatureType = SIGNATURE_TYPE_UDF
>>>      MBRType = MBR_TYPE_PCAT
>>>      Signature = *
>>>   And later UdfDxe driver checks the SignatureType and MBRType.
>>>   I am not sure if it would be better to put the definitions in UEFI Spec,
>>>   since they are referenced by different modules.
>>>   I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo.
>>>   When proposing to UEFI Spec, this also needs to be considered,
>>>   for example, add PARTITION_TYPE_UDF to spec.
>>
>> Yes - I agree with you. My only concern is that UEFI specification doesn't either support UDF or there is any interest in supporting it, so by proposing an additional type for something that shouldn't be supported, might not work out.
>>
>> (Andrew, any thoughts?)
>>
> 
> The enum space is owned by the UEFI spec and should never be extended outside the scope of the spec. Its not good to have an implementation running around that is not in a released spec, as it it is a future compatibility risk. 
> 
> Does the UDF actually start with a real MBR? If so is it possible to define a 32-bit MBR signature to indicate UDF. If not it should probably be a device path node like CD-ROM. 
> 
> You can all ways use the Vendor-Defined Media Device Path since it has GUID there is no risk of collision. 

(I vaguely recall that this was the sticking point last time around?...)

I support getting the patches merged in MdeModulePkg first, with a
Vendor-Defined Media Device Path -- assuming no other technical problems
remain --, and then standardizing a new enum second. The code under
MdeModulePkg can be updated later. (MdeModulePkg in upstream edk2 master
does not guarantee backward compatibility for device paths captured in
UEFI boot options and such -- there have been compat breaking changes in
the past --; I don't see why this couldn't work similarly.)

History has proved that without a company in the USWG actually pushing
for this feature, Paulo (or any other individual contributor) has meager
chance to drive standardization. Therefore making standardization a
prerequisite for the code is wrong. We have plenty of
Ekdii*ProtocolGuids too, for example.

As far as I can see, the only addition to MdePkg (not MdeModulePkg) is
"MdePkg/Include/IndustryStandard/Udf.h", which corresponds to the UDF
spec. So that should be OK (and unaffected by device path assignments).

In my opinion, it's OK if the UDF device paths are temporarily formatted
as "VenMsg(...)" -- for any suitable definition of "temporary".

If it's inconvenient to merge this driver under MdeModulePkg under the
above conditions -- and seeing how FileSystemPkg has never been created
over the years --, we can also merge it in OvmfPkg. In that case,
* we should conspicuously mark the filesystem driver "experimental",
* reinstate the ENABLE build switch -- seeing how UDF support is
actually not a requirement in the UEFI spec --, and
* assign a new Reviewer role to Paulo, for this module under OvmfPkg.

(I think OvmfPkg is totally inappropriate for hosting a
platform-independent filesystem driver, but apparently we're stuck,
again, on "standardization by a private individual".)

Thanks
Laszlo

> 
> Thanks,
> 
> Andrew Fish
> 
>> "PARTITION_TYPE_OTHER" is also used when creating EFI_PARTITION_INFO_PROTOCOL for CDROM "El Torito" partitions, so either we could follow the same approach, or propose creating two other types: e.g., PARTITION_TYPE_ELTORITO and PARTITION_TYPE_UDF.
>>
>>> 3. The driver model part looks good.
>>
>> Cool! Thanks for looking into that.
>>
>> Paulo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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