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.9767.1603156116231886611 for ; Mon, 19 Oct 2020 18:08:37 -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 ; Tue, 20 Oct 2020 09:08:20 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: "'Laszlo Ersek'" , "'Tom Lendacky'" , 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> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IFtQQVRDSCB2MiAwMS8xMV0gTWRlUGtnLCBPdm1mUGtnOiBDbGVhbiB1cCBHSENCIGZpZWxkIG9mZnNldHMgYW5kIHNhdmUgYXJlYQ==?= Date: Tue, 20 Oct 2020 09:08:21 +0800 Message-ID: <006f01d6a67d$80b77d80$82267880$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQKfp3CfnkRussMBowKfV6mBTee23AEhTXMjAhC1oQABkbdHWQKBSxHFp9OBR4A= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn 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 >=20 > On 10/19/20 14:50, Tom Lendacky wrote: > > On 10/18/20 8:41 PM, gaoliming wrote: >=20 > >> 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. >=20 > I disagree. >=20 > 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.=20 Thanks Liming >=20 > > 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. >=20 > 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. >=20 > I wrote: >=20 > 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). >=20 > 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. >=20 > 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. >=20 > Thanks > Laszlo