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

["Laszlo Ersek" <lersek@redhat.com>]

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.

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
> 
> 
>