回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

Thu Oct 22 01:25:55 UTC 2020

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 different package. Although the first commit breaks the tree, your patch is the patch serial, they will be merged together. Its impact should be small. 

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+66512+4905953+8761045 at groups.io
> <bounce+27952+66512+4905953+8761045 at groups.io> 代表 Lendacky,
> Thomas
> 发送时间: 2020年10月22日 5:54
> 收件人: gaoliming <gaoliming at byosoft.com.cn>; 'Laszlo Ersek'
> <lersek at redhat.com>; devel at edk2.groups.io
> 抄送: 'Brijesh Singh' <brijesh.singh at amd.com>; 'Michael D Kinney'
> <michael.d.kinney at intel.com>; 'Zhiguang Liu' <zhiguang.liu at intel.com>;
> 'Jordan Justen' <jordan.l.justen at intel.com>; 'Ard Biesheuvel'
> <ard.biesheuvel at arm.com>
> 主题: Re: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg,
> OvmfPkg: Clean up GHCB field offsets and save area
> 
> On 10/20/20 7:54 PM, gaoliming wrote:
> > Tom:
> 
> Hi Liming,
> 
> >
> >> -----邮件原件-----
> >> 发件人: Tom Lendacky <thomas.lendacky at amd.com>
> >> 发送时间: 2020年10月20日 21:10
> >> 收件人: Laszlo Ersek <lersek at redhat.com>; gaoliming
> >> <gaoliming at byosoft.com.cn>; devel at edk2.groups.io
> >> 抄送: 'Brijesh Singh' <brijesh.singh at amd.com>; 'Michael D Kinney'
> >> <michael.d.kinney at intel.com>; 'Zhiguang Liu' <zhiguang.liu at intel.com>;
> >> 'Jordan Justen' <jordan.l.justen at intel.com>; 'Ard Biesheuvel'
> >> <ard.biesheuvel at arm.com>
> >> 主题: Re: 回复: 回复: [PATCH 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:
> >>>>
> >>>>> -----邮件原件-----
> >>>>> 发件人: Laszlo Ersek <lersek at redhat.com>
> >>>>> 发送时间: 2020年10月20日 4:42
> >>>>> 收件人: Tom Lendacky <thomas.lendacky at amd.com>; gaoliming
> >>>>> <gaoliming at byosoft.com.cn>; devel at edk2.groups.io
> >>>>> 抄送: 'Brijesh Singh' <brijesh.singh at amd.com>; 'Michael D Kinney'
> >>>>> <michael.d.kinney at intel.com>; 'Zhiguang Liu'
> <zhiguang.liu at intel.com>;
> >>>>> 'Jordan Justen' <jordan.l.justen at intel.com>; 'Ard Biesheuvel'
> >>>>> <ard.biesheuvel at arm.com>
> >>>>> 主题: Re: 回复: [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.
> >>
> >> 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.
> 
> This raises a question about patch 10 of the series. Since that patch
> changes interfaces, I included both the UefiCpuPkg and OvmfPkg changes in
> that one patch. Will that be acceptable?
> 
> Thanks,
> Tom
> 
> >
> > Thanks
> > Liming
> >
> >> Thanks,
> >> Tom
> >>
> >>>
> >>> 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
> >>>>
> >>>>
> >>>>
> >>>
> >
> >
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66518): https://edk2.groups.io/g/devel/message/66518
Mute This Topic: https://groups.io/mt/77721999/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-