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

["gaoliming" <gaoliming@byosoft.com.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 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@groups.io
> <bounce+27952+66512+4905953+8761045@groups.io> 代表 Lendacky,
> Thomas
> 发送时间: 2020年10月22日 5:54
> 收件人: gaoliming <gaoliming@byosoft.com.cn>; 'Laszlo Ersek'
> <lersek@redhat.com>; devel@edk2.groups.io
> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> <ard.biesheuvel@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@amd.com>
> >> 发送时间: 2020年10月20日 21:10
> >> 收件人: Laszlo Ersek <lersek@redhat.com>; gaoliming
> >> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> >> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> >> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> >> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> >> <ard.biesheuvel@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@redhat.com>
> >>>>> 发送时间: 2020年10月20日 4:42
> >>>>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
> >>>>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> >>>>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> >>>>> <michael.d.kinney@intel.com>; 'Zhiguang Liu'
> <zhiguang.liu@intel.com>;
> >>>>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> >>>>> <ard.biesheuvel@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
> >>>>
> >>>>
> >>>>
> >>>
> >
> >
> 
> 
> 
>