public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Andrew Fish <afish@apple.com>
To: Paulo Alcantara <pcacjr@zytor.com>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Dong, Eric" <eric.dong@intel.com>,
	"Wu, Hao A" <hao.a.wu@intel.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	"Gao, Liming" <liming.gao@intel.com>,
	Mike Kinney <michael.d.kinney@intel.com>,
	Laszlo Ersek <lersek@redhat.com>,
	"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH v2 0/6] read-only UDF file system support
Date: Tue, 22 Aug 2017 10:21:30 -0700	[thread overview]
Message-ID: <D0151820-1924-4BCF-A093-5B8C173F3622@apple.com> (raw)
In-Reply-To: <bb43a219-6379-a881-e7a3-6edf4cdb1d35@zytor.com>


> 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



  reply	other threads:[~2017-08-22 17:19 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-20 18:15 [PATCH v2 0/6] read-only UDF file system support Paulo Alcantara
2017-08-20 18:15 ` [PATCH v2 1/6] MdePkg: Add UDF volume structure definitions Paulo Alcantara
2017-08-20 18:15 ` [PATCH v2 2/6] MdeModulePkg/PartitionDxe: Add UDF file system support Paulo Alcantara
2017-08-20 18:15 ` [PATCH v2 3/6] MdeModulePkg: Initial UDF/ECMA-167 " Paulo Alcantara
2017-08-20 18:15 ` [PATCH v2 4/6] OvmfPkg: Enable UDF " Paulo Alcantara
2017-08-21 16:14   ` Laszlo Ersek
2017-08-20 18:15 ` [PATCH v2 5/6] ArmVirtPkg: " Paulo Alcantara
2017-08-21 15:35   ` Ard Biesheuvel
2017-08-21 16:11   ` Laszlo Ersek
2017-08-20 18:15 ` [PATCH v2 6/6] Nt32Pkg: " Paulo Alcantara
2017-08-21  2:29 ` [PATCH v2 0/6] read-only " Ni, Ruiyu
2017-08-21  2:37   ` Gao, Liming
2017-08-22 13:23     ` Paulo Alcantara
2017-08-22 13:14   ` Paulo Alcantara
2017-08-22 17:21     ` Andrew Fish [this message]
2017-08-22 17:56       ` Paulo Alcantara
2017-08-22 22:36         ` Andrew Fish
2017-08-22 17:58       ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D0151820-1924-4BCF-A093-5B8C173F3622@apple.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox