On 20/09/22 14:18, Alexey Kardashevskiy wrote:
> Adding Nikunj for his opinion on the reworking (at the very bottom of this mail).
>
>
> On 13/9/22 16:38, Sean Christopherson wrote:
>> On Tue, Sep 13, 2022, Alexey Kardashevskiy wrote:
>>> On 12/9/22 19:36, Sean Christopherson wrote:
>>>> On Mon, Sep 12, 2022, Alexey Kardashevskiy wrote:
>>>>> A recent renaming patch missed 1 spot, fix it.
>>>>>
>>>>> This should cause no behavioural change.
>>>>>
>>>>> Fixes: 23e5092b6e2a ("KVM: SVM: Rename hook implementations to conform to kvm_x86_ops' names")
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
>>>>> ---
>>>>> arch/x86/kvm/svm/sev.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>> index 28064060413a..3b99a690b60d 100644
>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>> @@ -3015,7 +3015,7 @@ void sev_es_prepare_switch_to_guest(struct sev_es_save_area *hostsa)
>>>>> /*
>>>>> * As an SEV-ES guest, hardware will restore the host state on VMEXIT,
>>>>> * of which one step is to perform a VMLOAD. KVM performs the
>>>>> - * corresponding VMSAVE in svm_prepare_guest_switch for both
>>>>> + * corresponding VMSAVE in svm_prepare_switch_to_guest for both
>>>>> * traditional and SEV-ES guests.
>>>>> */
>>>>
>>>> Rather than match the rename, what about tweaking the wording to not tie the comment
>>>> to the function name, e.g. "VMSAVE in common SVM code".
>>>
>>> Although I kinda like the pointer to the caller, it is not that useful with
>>> a single caller and working cscope :)
>>
>> Yeah, having exact function names is nice, but we always seem to end up with goofs
>> like this where a comment gets left behind and then they become stale and confusing.
>>
>>>> Even better, This would be a good opportunity to reword this comment to make it more
>>>> clear why SEV-ES needs a hook, and to absorb the somewhat useless comments below.
>>>>
>>>> Would something like this be accurate? Please modify and/or add details as necessary.
>>>>
>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>> index 3b99a690b60d..c50c6851aedb 100644
>>>> --- a/arch/x86/kvm/svm/sev.c
>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>> @@ -3013,19 +3013,14 @@ void sev_es_vcpu_reset(struct vcpu_svm *svm)
>>>> void sev_es_prepare_switch_to_guest(struct sev_es_save_area *hostsa)
>>>> {
>>>> /*
>>>> - * As an SEV-ES guest, hardware will restore the host state on VMEXIT,
>>>> - * of which one step is to perform a VMLOAD. KVM performs the
>>>> - * corresponding VMSAVE in svm_prepare_switch_to_guest for both
>>>> - * traditional and SEV-ES guests.
>>>> + * Manually save host state that is automatically loaded by hardware on
>>>> + * VM-Exit from SEV-ES guests, but that is not saved by VMSAVE (which is
>>>> + * performed by common SVM code). Hardware unconditionally restores
>>>> + * host state, and so KVM skips manually restoring this state in common
>>>> + * code.
>>>
>>> I am new to this arch so not sure :) The AMD spec calls these three "Type B
>>> swaps" from the VMSA's "Table B-3. Swap Types" so may be just say:
>>>
>>> ===
>>> These are Type B swaps which are not saved by VMSAVE (performed by common
>>> SVM code) but restored by VMEXIT unconditionally.
>>
>> Weird consistency nit: KVM refers to VM-Exit as an event and not a thing/action,
>> whereas the APM tends to refer to VMEXIT as a thing, thus the "on VM-Exit" stylization
>> versus "by VMEXIT". Similarly, when talking about the broader event of entering
>> the guest, KVM uses "VM-Enter".
>>
>> VMRUN and VMSAVE on the other hand are instructions and so are "things" in KVM's world.
>>
>> Using the VM-Enter/VM-Exit terminology consistently throughout KVM x86 makes it easy
>> to talk about KVM x86 behavior that is common to both SVM and VMX without getting
>> tripped up on naming differences between the two. So even though it's a little odd
>> odd when looking only at SVM code, using "on VM-Exit" instead of "by VMEXIT" is
>> preferred.
>>
>>> ===
>>
>> I want to avoid relying on the APM's arbitrary "Type B" classification. Having to
>> dig up and look at a manual to understand something that's conceptually quite simple
>> is frustrating. Providing references to "Type B" and the table in the changelog is
>> definitely welcome, e.g. so that someone who wants more details/background can easily
>> find that info via via git blame.
How about the following:
Save states are classified into three types (APM Volume 2: Table B-3. Swap Types)
A: VM-Enter: Host state are saved in host save area
VM-Exit: Host state are restored automatically from host save area
B: VM-Enter: Host state are _not_ saved to host save area, KVM needs to save
required states manually in the host save area
VM-Exit: Host state are restored automatically from host save area
C: VM-Enter: Host state are _not_ saved to host save area.
VM-Exit: Host state are initialized to default(reset) values.
Manually save state(type-B) that is loaded unconditionally by hardware on
VM-Exit for SEV-ES guests, but is not saved via VMRUN or VMSAVE (performed
by common SVM code).
>> But for a comment, providing all the information
>> in the comment itself is usually preferable.
>>
>> How about this?
>>
>> Save state that is loaded unconditionally by hardware on VM-Exit for SEV-ES
>> guests, but is not saved via VMRUN or VMSAVE (performed by common SVM code).
Regards
Nikunj
