Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Babu Moger <babu.moger@amd.com>]

On 2/11/21 2:56 AM, Paolo Bonzini wrote:
> On 29/01/21 01:43, Babu Moger wrote:
>> This support also fixes an issue where a guest may sometimes see an
>> inconsistent value for the SPEC_CTRL MSR on processors that support this
>> feature. With the current SPEC_CTRL support, the first write to
>> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
>> MSR is not updated.
> 
> This is a bit ugly, new features should always be enabled manually (AMD
> did it right for vVMLOAD/vVMSAVE for example, even though _in theory_
> assuming that all hypervisors were intercepting VMLOAD/VMSAVE would have
> been fine).
> 
> Also regarding nested virtualization:
> 
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 7a605ad8254d..9e51f9e4f631 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -534,6 +534,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
>>          hsave->save.cr3    = vmcb->save.cr3;
>>      else
>>          hsave->save.cr3    = kvm_read_cr3(&svm->vcpu);
>> +    hsave->save.spec_ctrl = vmcb->save.spec_ctrl;
>>  
>>      copy_vmcb_control_area(&hsave->control, &vmcb->control);
>>  
>> @@ -675,6 +676,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>>      kvm_rip_write(&svm->vcpu, hsave->save.rip);
>>      svm->vmcb->save.dr7 = DR7_FIXED_1;
>>      svm->vmcb->save.cpl = 0;
>> +    svm->vmcb->save.spec_ctrl = hsave->save.spec_ctrl;
>>      svm->vmcb->control.exit_int_info = 0;
>>  
>>      vmcb_mark_all_dirty(svm->vmcb);
> 
> I think this is incorrect.  Since we don't support this feature in the
> nested hypervisor, any writes to the SPEC_CTRL MSR while L2 (nested guest)
> runs have to be reflected to L1 (nested hypervisor).  In other words, this
> new field is more like VMLOAD/VMSAVE state, in that it doesn't change
> across VMRUN and VMEXIT.  These two hunks can be removed.

Makes sense. I have tested removing these two hunks and it worked fine.

> 
> If we want to do it, exposing this feature to the nested hypervisor will
> be a bit complicated, because one has to write host SPEC_CTRL |
> vmcb01.GuestSpecCtrl in the host MSR, in order to free the vmcb02
> GuestSpecCtrl for the vmcb12 GuestSpecCtrl.
> 
> It would also be possible to emulate it on processors that don't have it. 
> However I'm not sure it's a good idea because of the problem that you
> mentioned with running old kernels on new processors.
> 
> I have queued the patches with the small fix above.  However I plan to
> only include them in 5.13 because I have a bunch of other SVM patches,

Yes. 5.13 is fine.
thanks
Babu

> those have been tested already but I need to send them out for review
> before "officially" getting them in kvm.git.
> 
> Paolo
>