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.web12.3698.1603329966263977022 for ; Wed, 21 Oct 2020 18:26:07 -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 ; Thu, 22 Oct 2020 09:25:54 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , "'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> <004401d6a744$b6208e90$2261abb0$@byosoft.com.cn> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0g5Zue5aSNOiDlm57lpI06IOWbnuWkjTogW1BBVENIIHYyIDAxLzExXSBNZGVQa2csIE92bWZQa2c6IENsZWFuIHVwIEdIQ0IgZmllbGQgb2Zmc2V0cyBhbmQgc2F2ZSBhcmVh?= Date: Thu, 22 Oct 2020 09:25:55 +0800 Message-ID: <008b01d6a812$4a331df0$de9959d0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQKfp3CfnkRussMBowKfV6mBTee23AEhTXMjAhC1oQABkbdHWQKBSxHFAkw2+IcBgYSO7gGljQ5xAb05JFEB4uAvjKeOD6xA Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Tom: For the patch in UefiCpuPkg, this changes the library interface. So, the= consumer and producer code is required to be changed together. There is no= good compatible way to do it. I still prefer to separate them for the diff= erent package. Although the first commit breaks the tree, your patch is the= patch serial, they will be merged together. Its impact should be small.=20 Thanks Liming > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: bounce+27952+66512+4905953+8761045@groups.i= o > =E4=BB=A3=E8=A1=A8 Lendac= ky, > Thomas > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2020=E5=B9=B410=E6=9C=8822=E6=97= =A5 5:54 > =E6=94=B6=E4=BB=B6=E4=BA=BA: gaoliming ; 'Lasz= lo Ersek' > ; 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: [edk2-devel] =E5=9B=9E=E5=A4=8D: =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 7:54 PM, gaoliming wrote: > > Tom: >=20 > Hi Liming, >=20 > > > >> -----=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 ; gaolim= ing > >> ; 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: [PATC= H v2 01/11] MdePkg, OvmfPkg: Clean up > GHCB > >> field offsets and save area > >> > >> 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' ; 'Mich= ael 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] MdePk= g, 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, b= ut > 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. Th= at > >>> 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 (4t= h) > >>> patch. > >> > >> 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. > >> > > I am fine to keep current GHCB_REGISTER name. >=20 > This raises a question about patch 10 of the series. Since that patch > changes interfaces, I included both the UefiCpuPkg and OvmfPkg changes i= n > that one patch. Will that be acceptable? >=20 > Thanks, > Tom >=20 > > > > Thanks > > Liming > > > >> Thanks, > >> Tom > >> > >>> > >>> Thank you! > >>> Laszlo > >>> > >>>> > >>>> Thanks > >>>> Liming > >>>>> > >>>>>> Ok, then I'll resubmit without the name change. To me, it's not w= orth > >>>>>> 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 constan= ts > >>>>> (such as GhcbCpl). Because those identifiers are declared at f= ile > >>>>> 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 2= nd > >>>>> patch (introducing the new type, in addition to the old type) woul= d not > >>>>> compile. The new type could not reuse the old type's enum constant= s > >>>>> (their identifiers). So we'd either have to change the enum consta= nt > >>>>> names of the old type (and then we'd be back to square 1 -- the cl= ient > >>>>> sites would have to be migrated in the same patch), or introduce t= he > new > >>>>> type with new enum constant names as well. But then, the client si= tes > >>>>> 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 w= as > >>>>> acceptable. Because otherwise we could never rename an enum type > >> without > >>>>> also renaming the enum constants. > >>>>> > >>>>> Thanks > >>>>> Laszlo > >>>> > >>>> > >>>> > >>> > > > > >=20 >=20 >=20 >=20