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: <334aadda53c7837d71e7c9d11b772a4a66b58df3.camel@redhat.com>
Subject: Re: [RFC PATCH 06/13] KVM: SVM: Add logic to determine x2APIC 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, 24 Feb 2022 19:29:15 +0200
In-Reply-To: <20220221021922.733373-7-suravee.suthikulpanit@amd.com>
References: <20220221021922.733373-1-suravee.suthikulpanit@amd.com>
         <20220221021922.733373-7-suravee.suthikulpanit@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 Sun, 2022-02-20 at 20:19 -0600, Suravee Suthikulpanit wrote:
> Introduce avic_update_vapic_bar(), which checks the x2APIC enable bit
> of the APIC Base register and update the new struct vcpu_svm.x2apic_enabled
> to keep track of current APIC mode of each vCPU,
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 13 +++++++++++++
>  arch/x86/kvm/svm/svm.c  |  4 ++++
>  arch/x86/kvm/svm/svm.h  |  2 ++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 1999076966fd..60f30e48d816 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -609,6 +609,19 @@ void avic_post_state_restore(struct kvm_vcpu *vcpu)
>  	avic_handle_ldr_update(vcpu);
>  }
>  
> +void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data)
> +{
> +	svm->vmcb->control.avic_vapic_bar = data & VMCB_AVIC_APIC_BAR_MASK;
> +
> +	/* Set x2APIC mode bit if guest enable x2apic mode. */
> +	svm->x2apic_enabled = (avic_mode == AVIC_MODE_X2 &&
> +			       kvm_apic_mode(data) == LAPIC_MODE_X2APIC);
> +	pr_debug("vcpu_id:%d switch to %s\n", svm->vcpu.vcpu_id,
> +		 svm->x2apic_enabled ? "x2APIC" : "xAPIC");
> +	vmcb_mark_dirty(svm->vmcb, VMCB_AVIC);
> +	kvm_vcpu_update_apicv(&svm->vcpu);
> +}
> +
>  void svm_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>  {
>  	return;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 3687026f2859..4e6dc1feeac7 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2867,6 +2867,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		svm->msr_decfg = data;
>  		break;
>  	}
> +	case MSR_IA32_APICBASE:
> +		if (kvm_vcpu_apicv_active(vcpu))
> +			avic_update_vapic_bar(to_svm(vcpu), data);
> +		fallthrough;
>  	default:
>  		return kvm_set_msr_common(vcpu, msr);
>  	}
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 1a0bf6b853df..bfbebb933da2 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -225,6 +225,7 @@ struct vcpu_svm {
>  	u32 dfr_reg;
>  	struct page *avic_backing_page;
>  	u64 *avic_physical_id_cache;
> +	bool x2apic_enabled;
>  
>  	/*
>  	 * Per-vcpu list of struct amd_svm_iommu_ir:
> @@ -566,6 +567,7 @@ void avic_init_vmcb(struct vcpu_svm *svm);
>  int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu);
>  int avic_unaccelerated_access_interception(struct kvm_vcpu *vcpu);
>  int avic_init_vcpu(struct vcpu_svm *svm);
> +void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data);
>  void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
>  void avic_vcpu_put(struct kvm_vcpu *vcpu);
>  void avic_post_state_restore(struct kvm_vcpu *vcpu);


Have you looked at how this is done on Intel's APICv side?
You need to implement .set_virtual_apic_mode instead.
(look at vmx_set_virtual_apic_mode)

I also don't think you need x2apic_enabled boolean.
You can already know if a vCPU uses apic or x2apic via

kvm_get_apic_mode(vcpu);

in fact I don't think avic code should have any bookeeping in regard to x2apic/x2avic mode,
but rather kvm's apic mode  (which is read directly from apic base msr (vcpu->arch.apic_base),
should enable avic, or x2avic if possible, or inhibit avic if not possible.

e.g it should drive the bits in vmcb and such.

Best regards,
	Maxim Levitsky