Re: [PATCH 3/4] KVM: x86: SVM: use vmcb01 in avic_init_vmcb

[Maxim Levitsky]

On Tue, 2022-03-01 at 16:21 +0000, Sean Christopherson wrote:
> Just "KVM: SVM:" for the shortlog, please.
> 
> On Tue, Mar 01, 2022, Maxim Levitsky wrote:
> > Out of precation use vmcb01 when enabling host AVIC.
> > No functional change intended.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> > ---
> >  arch/x86/kvm/svm/avic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index e23159f3a62ba..9656e192c646b 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -167,7 +167,7 @@ int avic_vm_init(struct kvm *kvm)
> >  
> >  void avic_init_vmcb(struct vcpu_svm *svm)
> >  {
> > -	struct vmcb *vmcb = svm->vmcb;
> > +	struct vmcb *vmcb = svm->vmcb01.ptr;
> 
> I don't like this change.  It's not bad code, but it'll be confusing because it
> implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr
> when this is called.

Honestly I don't see how you had reached this conclusion.
 
I just think that code that always works on vmcb01
should use it, even if it happens that vmcb == vmcb01.
 
If you insist I can drop this patch or add WARN_ON instead,
I just think that this way is cleaner.
 
Best regards,
	Maxim Levitsky

> 
> If we want to guard AVIC, I'd much rather we do something like:
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7038c76fa841..dcc856bd628d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -992,8 +992,12 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu)
>  static void init_vmcb(struct kvm_vcpu *vcpu)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
> -       struct vmcb_control_area *control = &svm->vmcb->control;
> -       struct vmcb_save_area *save = &svm->vmcb->save;
> +       struct vmcb *vmcb = svm->vmcb01.ptr;
> +       struct vmcb_control_area *control = &vmcb->control;
> +       struct vmcb_save_area *save = &vmcb->save;
> +
> +       if (WARN_ON_ONCE(vmcb != svm->vmcb))
> +               svm_leave_nested(vcpu);
> 
>         svm_set_intercept(svm, INTERCEPT_CR0_READ);
>         svm_set_intercept(svm, INTERCEPT_CR3_READ);
> 
> 
> On a related topic, init_vmcb_after_set_cpuid() is broken for nested, it needs to
> play nice with being called when svm->vmcb == &svm->nested.vmcb02, e.g. update
> vmcb01 and re-merge (or just recalc?) vmcb02's intercepts.
> 
> >  	struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm);
> >  	phys_addr_t bpa = __sme_set(page_to_phys(svm->avic_backing_page));
> >  	phys_addr_t lpa = __sme_set(page_to_phys(kvm_svm->avic_logical_id_table_page));
> > -- 
> > 2.26.3
> >