From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 090AD21E49BCA for ; Tue, 22 Aug 2017 10:56:13 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5ABB3C034E7B; Tue, 22 Aug 2017 17:58:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5ABB3C034E7B Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-92.phx2.redhat.com [10.3.116.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0F0F17C8AE; Tue, 22 Aug 2017 17:58:42 +0000 (UTC) To: Andrew Fish , Paulo Alcantara Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Dong, Eric" , "Wu, Hao A" , Jordan Justen , "Gao, Liming" , Mike Kinney , "Zeng, Star" References: <20170820181557.28761-1-pcacjr@zytor.com> <734D49CCEBEEF84792F5B80ED585239D5B9F3CD8@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 22 Aug 2017 19:58:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 22 Aug 2017 17:58:45 +0000 (UTC) Subject: Re: [PATCH v2 0/6] read-only UDF file system support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Aug 2017 17:56:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/22/17 19:21, Andrew Fish wrote: > >> On Aug 22, 2017, at 6:14 AM, Paulo Alcantara 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 >