Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c5cb5f62818a0d5aa03bd730312b93541cdeb97d.camel@redhat.com>
Subject: Re: [PATCH v3 08/14] KVM: SVM: Update AVIC settings when changing
 APIC mode
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     pbonzini@redhat.com, seanjc@google.com, joro@8bytes.org,
        jon.grimm@amd.com, wei.huang2@amd.com, terry.bowman@amd.com
Date:   Thu, 05 May 2022 10:34:57 +0300
In-Reply-To: <f308ae5c-968d-eab4-2caa-29517e5ac982@amd.com>
References: <20220504073128.12031-1-suravee.suthikulpanit@amd.com>
         <20220504073128.12031-9-suravee.suthikulpanit@amd.com>
         <ff67344c0efe06d1422aa84e56738a0812c69bfc.camel@redhat.com>
         <f308ae5c-968d-eab4-2caa-29517e5ac982@amd.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2022-05-05 at 08:38 +0700, Suravee Suthikulpanit wrote:
> Maxim,
> 
> On 5/4/22 7:19 PM, Maxim Levitsky wrote:
> > On Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
> > > Update and refresh AVIC settings when guest APIC mode is updated
> > > (e.g. changing between disabled, xAPIC, or x2APIC).
> > > 
> > > Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
> > > ---
> > >   arch/x86/kvm/svm/avic.c | 16 ++++++++++++++++
> > >   arch/x86/kvm/svm/svm.c  |  1 +
> > >   2 files changed, 17 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > index 3ebeea19b487..d185dd8ddf17 100644
> > > --- a/arch/x86/kvm/svm/avic.c
> > > +++ b/arch/x86/kvm/svm/avic.c
> > > @@ -691,6 +691,22 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
> > >   	avic_handle_ldr_update(vcpu);
> > >   }
> > >   
> > > +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> > > +{
> > > +	struct vcpu_svm *svm = to_svm(vcpu);
> > > +
> > > +	if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
> > > +		return;
> > > +
> > > +	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> > > +		WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> > > +		return;
> > > +	}
> > > +
> > > +	kvm_vcpu_update_apicv(&svm->vcpu);
> > Why to have this call? I think that all that is needed is only to call the
> > avic_refresh_apicv_exec_ctrl.
> 
> When APIC mode is updated on each vCPU, we need to check and update
> vcpu->arch.apicv_active accordingly, which happens in the kvm_vcpu_update_apicv()

This makes sense, but IMHO it would be better then to call kvm_vcpu_update_apicv
from the common code when apic mode changes then, because this logic should apply
to APICv as well.

In fact that logic of not activating AVIC was added in patch 12 
(and on second thought I think it  should be split to a separate patch), 
was added to common code, thus calling kvm_vcpu_update_apicv when the condition
of 'apic is disabled on this vCPU' should also be done by the common code.

Best regards,
	Maxim Levitsky

> 
> One test case that would fail w/o the kvm_vcpu_update_apicv() is when
> we boot a Linux guest w/ guest kernel option _nox2apic_, which Linux forces APIC
> mode of vCPUs with APIC ID 255 and higher to disable. W/o this line of code, the VM
> would not boot w/ more than 255 vCPUs.
> 
> Regards,
> Suravee
>