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 664AE20945B64 for ; Sat, 16 Sep 2017 14:16:46 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7FC7F4A6E0; Sat, 16 Sep 2017 21:19:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7FC7F4A6E0 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-8.rdu2.redhat.com [10.10.120.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id 77338600C0; Sat, 16 Sep 2017 21:19:45 +0000 (UTC) To: "Yao, Jiewen" , "Ni, Ruiyu" , Paulo Alcantara Cc: "Wu, Hao A" , "edk2-devel@lists.01.org" , "Zeng, Star" References: <734D49CCEBEEF84792F5B80ED585239D5BA4206B@SHSMSX151.ccr.corp.intel.com> <081B7D33-9F33-40CE-B569-62CC8C204B56@zytor.com> <734D49CCEBEEF84792F5B80ED585239D5BA420DF@SHSMSX151.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BA4212E@SHSMSX151.ccr.corp.intel.com> <1C01A5C1-09DE-4747-BA65-4EB668D76094@zytor.com> <734D49CCEBEEF84792F5B80ED585239D5BA42270@SHSMSX151.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BA42713@SHSMSX151.ccr.corp.intel.com> <508e1df7-fe00-126b-d583-c81db8514e10@redhat.com> <11481899-6F36-4877-B4FF-732B2781F3CB@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9BC398@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9BC3E0@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <3d377bc8-7193-7296-02b9-b9fb7ffac3d8@redhat.com> Date: Sat, 16 Sep 2017 23:19:44 +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: <74D8A39837DF1E4DA445A8C0B3885C503A9BC3E0@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Sat, 16 Sep 2017 21:19:47 +0000 (UTC) Subject: Re: Functionality issues in UDF 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: Sat, 16 Sep 2017 21:16:46 -0000 Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Jiewen, thank you for looking into this. I'm replying below quite at length; please bear with me. At the end I'm going to propose a new alternative to the PCD: On 09/16/17 01:58, Yao, Jiewen wrote: > Hi Laszlo/Ruiyu/Star > I went through the whole conversation and have some thought: > > For staging, I quote the description from https://github.com/tianocore/edk2-staging > > I think UDF matches all below criteria. "edk2 required quality criteria" is the key. > > We have lots of features there - such as StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem, StructurePcd. > > All those features need more validation. > We are following the rule and I think UDF feature can follow the same rule. > > If there is quality concern and the quality concern cannot be resolved in a short period of time, the staging tree is a better choice. > (NOTE: "Fix it immediately" is not an alternative; we can't do that.) Moving the UDF feature, including the PartitionDxe changes, to edk2-staging would require the following: (1) A maintainer that owns and maintains the UDF branch. - I definitely cannot be this person, first because I have no UDF knowledge, and second because I have no time for any more maintainer responsibilities than I currently have. Again, my interest in UDF is *only* as a user. - I assume Ray will also have no time for maintaining a UDF feature branch in edk2-staging. - If Paulo is willing to become a maintainer for the UDF branch in edk2-staging, that's good. Assuming he is *allowed* to become such a maintainer. (According to the edk2-staging description, periodic rebases to edk2/master are required, hence Paulo would be required to dedicate time to maintaining "edk2-staging/udf" on an on-going basis.) (2) The UDF-related commits would have to be reverted from edk2, and applied to edk2-staging/udf. The first part (the reverts) would be bad. The commits that have gone into UDF support thus far -- namely the build error fixes from everyone, and there's also a pending style improvement series from Hao Wu -- have already improved UDF a lot, and would have never happened in the staging repository. Furthermore, let me explain how most end-users (upstream users) consume OVMF: - edk2 maintainers push patches to edk2/master, - various people (Ard and others from Linaro, my colleague Gerd Hoffmann on his own time, probably others) operate build bots / CI environments that fetch edk2 master every day, and build RPM, DEB etc. packages for OVMF - end-users set up YUM or APT repositories on their machines, pointing to these packages, and periodically update their OVMF packages. This means we have a significant user base. If we break something in edk2/master, the build bots report build failures, and the users report functional failures. This is good for edk2/master -- not just for OvmfPkg, but for many other modules built into the OVMF binary. If we move UDF to edk2-staging/udf, then UDF support will never reach these end-users. The people that operate the CI environments will not repeatedly merge edk2-staging/udf into edk2/master on their end, just so they can build UDF support into OVMF. Such merges can even fail, if rebasing the edk2-staging/udf branch to the current edk2/master branch requires *manual* conflict resolution, and the edk2-staging/udf maintainer is busy. --*-- I understand that you guys absolutely don't want a new FeaturePCD. How about the following alternative. When the edk2-staging repository was initially discussed, I mentioned to Mike that Staging or Experimental directories would also be a possibility, inside edk2/master. So the below is a variant of that idea: (a) In the "MdeModulePkg/Universal/Disk/PartitionDxe" directory, create another directory called "Experimental". (b) Move the "Udf.c" source file to: MdeModulePkg/Universal/Disk/PartitionDxe/Experimental/Udf.c (c) Create a file called "Stubs.c": MdeModulePkg/Universal/Disk/PartitionDxe/Stubs.c This file would contain an implementation of PartitionInstallUdfChildHandles() that returned EFI_NOT_FOUND *unconditionally*, and did nothing else. (d) In the "PartitionDxe.inf" file, replace "Udf.c" with "Stubs.c", under [Sources]. (e) Create "PartitionExperimentalDxe.inf" in the same directory where "PartitionDxe.inf" is: MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf MdeModulePkg/Universal/Disk/PartitionDxe/PartitionExperimentalDxe.inf The second INF file would be identical to "PartitionDxe.inf", except it would include "Experimental/Udf.c", rather than "Stubs.c". The end result is that the PartitionDxe driver is built with just a UDF stub function that does nothing, and the PartitionExperimentalDxe driver is built with UDF support. The current regression would be fixed for all platforms immediately that use MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf No new PCD for MdeModulePkg, just a new INF file, and a stub file. In turn, OvmfPkg, ArmVirtPkg, and Nt32Pkg could accept -D UDF_ENABLE, and select the proper INF file, based on UDF_ENABLE. The -D UDF_ENABLE flag would also be very easy to add, for the people that operate the CI environments / build bots. If this were acceptable, I'd be glad to send patches for it. If it's not acceptable, then I think we should simply revert the UDF patches from edk2, and *not* bother about creating an edk2-staging/udf branch. I don't believe that CI operators and end-users would care about edk2-staging/udf, even if Paulo spent the time and rebased that branch to edk2/master very frequently. (Above, you mentioned the staging branches StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem, StructurePcd -- and those are branches that *I* don't know about.) So, can we do "PartitionExperimentalDxe.inf"? Thanks! Laszlo > ======================================================== > This repository is used by EDK II as a staging location for new > features that are not yet ready for inclusion in EDK II. > > Introduction > ================= > Need place on tianocore.org where new features that are not ready for product > integration can be checked in for evaluation by the EDK II community prior to > adding to the edk2 trunk. This serves several purposes: > > * Encourage source code to be shared earlier in the development process > * Allow source code to be shared that does not yet meet all edk2 required quality criteria > * Allow source code to be shared so the EDK II community can help finish and validate new features > * Provide a location to hold new features until they are deemed ready for product integration > * Provide a location to hold new features until there is a natural point in edk2 release cycle to fully validate the new feature. > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen > Sent: Saturday, September 16, 2017 7:39 AM > To: Laszlo Ersek ; Ni, Ruiyu ; Paulo Alcantara > Cc: Wu, Hao A ; edk2-devel@lists.01.org; Zeng, Star > Subject: Re: [edk2] Functionality issues in UDF support > > Hi Laszlo and Ruiyu > I can think 1 possible alternative, for your consideration only. > > 1) Move the feature to OvmfPkg. > > As such, it won't block us at this moment. > > Once the UDF solution has good quality, we can move it back to MdeModulePkg. > > Thank you > Yao Jiewen > > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Saturday, September 16, 2017 12:51 AM > To: Ni, Ruiyu >; Paulo Alcantara > > Cc: Yao, Jiewen >; Wu, Hao A >; edk2-devel@lists.01.org; Zeng, Star > > Subject: Re: [edk2] Functionality issues in UDF support > > On 09/15/17 18:40, Ni, Ruiyu wrote: >> Laszlo, >> Please do not add a PCD for this. Too many PCDs are no good to the project. > > I understand that new MdeModulePkg PCDs are not liked, but what do you > propose instead? If we simply revert the PartitionDxe changes, then > people that want to experiment with general UDF support under OVMF won't > be able to do that at all. > > I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and > Nt32Pkg, which would control both the FeaturePCD and the inclusion of > UdfDxe in the build. If you disagree with the FeaturePCD, I can stop > working on this, but I don't know what the alternative is. "Fix it > immediately" is not an alternative; we can't do that. If you want to > revert the change, it's your prerogative, but that will prevent > everybody from testing gradual UDF improvements. (No 3rd parties build > OVMF from any staging branches, so if the feature is only available on a > staging branch, it might as well not exist, for the outside world.) > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >