From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web10.4711.1603241671557189036 for ; Tue, 20 Oct 2020 17:54:34 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Wed, 21 Oct 2020 08:54:19 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: "'Tom Lendacky'" , "'Laszlo Ersek'" , Cc: "'Brijesh Singh'" , "'Michael D Kinney'" , "'Zhiguang Liu'" , "'Jordan Justen'" , "'Ard Biesheuvel'" References: <523f270e4e6f7a62cdbebc541b442bd766e7ab3a.1602864557.git.thomas.lendacky@amd.com> <000d01d6a5b9$03d14ef0$0b73ecd0$@byosoft.com.cn> <93c3b271-ccdb-6242-ce23-957eb73a66c0@amd.com> <006f01d6a67d$80b77d80$82267880$@byosoft.com.cn> <8734b948-e36c-81f4-43ae-6c63fba1374b@amd.com> In-Reply-To: <8734b948-e36c-81f4-43ae-6c63fba1374b@amd.com> Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IOWbnuWkjTogW1BBVENIIHYyIDAxLzExXSBNZGVQa2csIE92bWZQa2c6IENsZWFuIHVwIEdIQ0IgZmllbGQgb2Zmc2V0cyBhbmQgc2F2ZSBhcmVh?= Date: Wed, 21 Oct 2020 08:54:20 +0800 Message-ID: <004401d6a744$b6208e90$2261abb0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQKfp3CfnkRussMBowKfV6mBTee23AEhTXMjAhC1oQABkbdHWQKBSxHFAkw2+IcBgYSO7gGljQ5xp6l39EA= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Tom: > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: Tom Lendacky > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: = 2020=E5=B9=B410=E6=9C=8820=E6=97=A5 21:10 > =E6=94=B6=E4=BB=B6=E4=BA=BA: Laszlo Ersek ; = gaoliming > ; devel@edk2.groups.io > =E6=8A=84=E9=80=81: 'Brijesh Singh' ; 'Michael = D Kinney' > ; 'Zhiguang Liu' ; > 'Jordan Justen' ; 'Ard Biesheuvel' > > =E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: =E5=9B=9E=E5=A4=8D: [PATCH = v2 01/11] MdePkg, OvmfPkg: Clean up GHCB > field offsets and save area >=20 > On 10/20/20 3:31 AM, Laszlo Ersek wrote: > > On 10/20/20 03:08, gaoliming wrote: > >> Laszlo and Tom: > >> > >>> -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > >>> =E5=8F=91=E4=BB=B6=E4=BA=BA: Laszlo Ersek > >>> =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: = 2020=E5=B9=B410=E6=9C=8820=E6=97=A5 4:42 > >>> =E6=94=B6=E4=BB=B6=E4=BA=BA: Tom Lendacky = ; gaoliming > >>> ; devel@edk2.groups.io > >>> =E6=8A=84=E9=80=81: 'Brijesh Singh' ; = 'Michael D Kinney' > >>> ; 'Zhiguang Liu' = ; > >>> 'Jordan Justen' ; 'Ard Biesheuvel' > >>> > >>> =E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: [PATCH v2 01/11] = MdePkg, OvmfPkg: Clean up GHCB > field > >>> offsets and save area > >>> > >>> On 10/19/20 14:50, Tom Lendacky wrote: > >>>> On 10/18/20 8:41 PM, gaoliming wrote: > >>> > >>>>> Please separate the patch for the change in OvmfPkg. > >>>>> One commit can't cross the different packages. > >>>>> I understand this is the incompatible change. But, we still need = to > follow > >>>>> this rule. > >>> > >>> I disagree. > >>> > >>> We should do whatever we can for avoiding cross-package patches, = but in > >>> some cases, they are unavoidable. This is one of those cases. > >> > >> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef > GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h > >> The comments can be added here to describe it is for compatibility. = The old > one is not recommend. > >> > >> Then, the change in MdePkg is compatible. Next patch is to update > OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET. > > > > Ah, I totally missed that we could use typedef to bridge the gap. = That > > indeed allows us to do the rename in three steps (only for the type > > name, the enum constant identifiers can stay the same). After the > > rename, the enum constant values can be cleaned up in a separate = (4th) > > patch. >=20 > It seems like a lot of churn for just renaming. If there are no > objections, I'll keep the GHCB_REGISTER name. The important piece is = the > change from hardcoding the offset values to using a calculated value. >=20 I am fine to keep current GHCB_REGISTER name.=20 Thanks Liming > Thanks, > Tom >=20 > > > > Thank you! > > Laszlo > > > >> > >> Thanks > >> Liming > >>> > >>>> Ok, then I'll resubmit without the name change. To me, it's not = worth > >>>> doing 3 commits just to accomplish the rename from GHCB_REGISTER = to > >>>> GHCB_QWORD_OFFSET. > >>> > >>> When reviewing v1, I immediately thought of doing the rename in 3 > >>> commits (introduce new type, rebase client sites, remove old = type). I > >>> didn't suggest it because it wouldn't work. > >>> > >>> I wrote: > >>> > >>> we are not changing the identifiers of the enumeration = constants > >>> (such as GhcbCpl). Because those identifiers are declared at = file > >>> scope, having both GHCB_REGISTER and GHCB_QWORD_OFFSET > >>> declare > >>> (e.g.) GhcbCpl would cause a compilation failure. Therefore we = can't > >>> implement this in multiple stages (first introduce > >>> GHCB_QWORD_OFFSET, then remove GHCB_REGISTER separately). > >>> > >>> In other words, if we attempted to do this in 3 stages, then the = 2nd > >>> patch (introducing the new type, in addition to the old type) = would not > >>> compile. The new type could not reuse the old type's enum = constants > >>> (their identifiers). So we'd either have to change the enum = constant > >>> names of the old type (and then we'd be back to square 1 -- the = client > >>> sites would have to be migrated in the same patch), or introduce = the new > >>> type with new enum constant names as well. But then, the client = sites > >>> would have to be migrated to the new enum constant names as well, = not > >>> just the new enum type name. > >>> > >>> This is why I said that, IMO, in this case a cross-package patch = was > >>> acceptable. Because otherwise we could never rename an enum type > without > >>> also renaming the enum constants. > >>> > >>> Thanks > >>> Laszlo > >> > >> > >> > >