From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.7623.1588932602227391967 for ; Fri, 08 May 2020 03:10:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XbTLesuh; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588932601; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HNa1WyGymhRSFJTWoDyn1TE1xXjdLEb0fAkYiG7g5tQ=; b=XbTLesuhkaxgnAwCeam0UoLK/AxWahQ0HDtzqlfxvXtQA4sIV4iwmKiaY2AAKbWSzE4AOB wVyewIh0LTjKW7MYlSSamy3lWnVrf9pvw8r8t6ZpX3GEWcHCSzqVkmmkKHlFxBbtxarr5j B+UW2LseJbEiOZR5kHeGMVRJ0hPWBcI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-170-w1ABBIxFMdyiLYyrUgCKOw-1; Fri, 08 May 2020 06:09:54 -0400 X-MC-Unique: w1ABBIxFMdyiLYyrUgCKOw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5E6F91005510; Fri, 8 May 2020 10:09:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-60.ams2.redhat.com [10.36.114.60]) by smtp.corp.redhat.com (Postfix) with ESMTP id 823201001920; Fri, 8 May 2020 10:09:52 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 00/18] Remove All UGA Support To: devel@edk2.groups.io, guomin.jiang@intel.com References: <20200508083824.1785-1-guomin.jiang@intel.com> Cc: "Ard Biesheuvel (ARM address)" From: "Laszlo Ersek" Message-ID: Date: Fri, 8 May 2020 12:09:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200508083824.1785-1-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Hello Guomin, On 05/08/20 10:38, Guomin Jiang wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2368 > > UGA is replaced by GOP and remove all related code. I'm responding under the cover letter. (1) You should have CC'd the cover letter to each person that is CC'd on at least one patch in the series. Please do that in the future. I'm CC'ing Ard now, at least. (2) The series currently has identical subjects for some of the patches. For example: [PATCH 08/18] OvmfPkg: Remove All UGA Support [PATCH 15/18] OvmfPkg: Remove All UGA Support [PATCH 10/18] ArmVirtPkg: Remove All UGA Support [PATCH 17/18] ArmVirtPkg: Remove All UGA Support This is extremely bad practice. Please don't do this. (3) The commit messages do a bad job explaining why the PCDs are being removed. If I understand correctly, the core modules are updated in this series as follows: - PcdConOutUgaSupport is assumed constant FALSE - PcdConOutGopSupport is assumed constant TRUE - conditions are simplified and dead code is eliminated The proper approach is therefore the following: - In patch#1, change the DEC default value of PcdConOutUgaSupport from TRUE to FALSE. - In patches #2 .. #n, remove the PCD settins from all the DSC files. Just one patch per package is sufficient. (No need for a separate patch per PCD per Package.) The commit messages on these patches should explain that the PCDs are going away, and by deleting the DSC settings, the platforms in question inherit the DEC defaults. And, at this point the DEC defaults are: PcdConOutUgaSupport = FALSE PcdConOutGopSupport = TRUE which is going to be the only configuration supported by edk2 in the future. For example, patch#2 could be for ArmVirtPkg, patch#3 could be for OvmfPkg. - In the rest of the patches, simplify code / eliminate dead code by substituting FALSE for PcdConOutUgaSupport, and TRUE for PcdConOutGopSupport. If you want, you can keep the code changes for each PCD separate, in every module. (This kind of separation may be a good idea.) - In the last patch in the series, remove both PCDs from the DEC file. Before posting v2, please verify that the series (including the platforms in edk2) build fine at every stage (at every patch in the series). Thanks Laszlo