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