From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [65.50.211.136]) (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 382D92095DE41 for ; Thu, 10 Aug 2017 16:30:08 -0700 (PDT) Received: from [IPv6:2804:7f4:c480:30da::2] ([IPv6:2804:7f4:c480:30da:0:0:0:2]) (authenticated bits=0) by mail.zytor.com (8.15.2/8.15.2) with ESMTPSA id v7ANU5UF026901 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 10 Aug 2017 16:30:07 -0700 To: "Ni, Ruiyu" , "Zeng, Star" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" , "Wu, Hao A" , "Justen, Jordan L" , Andrew Fish , "Gao, Liming" , "Kinney, Michael D" , Laszlo Ersek References: <20170808193143.18128-1-pcacjr@zytor.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9132AF@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5B9DED45@SHSMSX104.ccr.corp.intel.com> <50a39f82-576f-f0bb-c133-4b448c332ca5@zytor.com> <7e6edff5-2d39-80bb-91dc-c47e332b28ad@zytor.com> <734D49CCEBEEF84792F5B80ED585239D5B9E5E6B@SHSMSX104.ccr.corp.intel.com> From: Paulo Alcantara Message-ID: <39fa8f40-51d7-e210-3558-8cc8f8271b25@zytor.com> Date: Thu, 10 Aug 2017 20:30:04 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5B9E5E6B@SHSMSX104.ccr.corp.intel.com> Subject: Re: [PATCH 0/4] 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: Thu, 10 Aug 2017 23:30:08 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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 ; Zeng, Star ; edk2-devel@lists.01.org >> Cc: Dong, Eric ; Wu, Hao A ; Justen, Jordan L ; >> Andrew Fish ; Gao, Liming ; Kinney, Michael D ; >> Laszlo Ersek >> 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