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: <d990c42a3ab5e8b1cbfa7775eef37ad4957147f6.camel@redhat.com>
Subject: Re: [RFCv2 PATCH 07/12] KVM: SVM: Introduce helper function
 kvm_get_apic_id
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 Mar 2022 16:14:09 +0200
In-Reply-To: <20220308163926.563994-8-suravee.suthikulpanit@amd.com>
References: <20220308163926.563994-1-suravee.suthikulpanit@amd.com>
         <20220308163926.563994-8-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 Tue, 2022-03-08 at 10:39 -0600, Suravee Suthikulpanit wrote:
> This function returns the currently programmed guest physical
> APIC ID of a vCPU in both xAPIC and x2APIC modes.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/lapic.c    | 23 +++++++++++++++++++++++
>  arch/x86/kvm/lapic.h    |  5 +----
>  arch/x86/kvm/svm/avic.c | 21 +++++++++++++++++----
>  3 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 03d1b6325eb8..73a1e650a294 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -106,11 +106,34 @@ static inline int apic_enabled(struct kvm_lapic *apic)
>  	(LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
>  	 APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>  
> +static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
> +{
> +	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> +}
> +
>  static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
>  {
>  	return apic->vcpu->vcpu_id;
>  }
>  
> +int kvm_get_apic_id(struct kvm_vcpu *vcpu, u32 *id)
> +{
> +	if (!id)
> +		return -EINVAL;
> +
> +	if (!apic_x2apic_mode(vcpu->arch.apic)) {
> +		/* For xAPIC, APIC ID cannot be larger than 254. */
> +		if (vcpu->vcpu_id >= APIC_BROADCAST)
> +			return -EINVAL;
> +
> +		*id = kvm_xapic_id(vcpu->arch.apic);
> +	} else {
> +		*id = kvm_x2apic_id(vcpu->arch.apic);
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_get_apic_id);
> +
>  static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  {
>  	return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 2b44e533fc8d..2b9463da1528 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -254,9 +254,6 @@ static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
>  	return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>  }
>  
> -static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
> -{
> -	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> -}
> +int kvm_get_apic_id(struct kvm_vcpu *vcpu, u32 *id);
>  
>  #endif
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4d7a8743196e..7e5a39a8e698 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -441,14 +441,21 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
>  
>  static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
> -	u32 id = kvm_xapic_id(vcpu->arch.apic);
> +	u32 id;
> +
> +	ret = kvm_get_apic_id(vcpu, &id);
> +	if (ret)
> +		return ret;
What about apic_id == 0?

>  
>  	if (ldr == svm->ldr_reg)
>  		return 0;
>  
> +	if (id == X2APIC_BROADCAST)
> +		return -EINVAL;
> +

Why this is needed? avic_handle_ldr_update is called either
when guest writes to APIC_LDR (should not reach here),
or if LDR got changed while AVIC was inhibited (also
thankfully KVM doesn't allow it to be changed in x2APIC	mode,
and it does reset it when enabling x2apic).


>  	avic_invalidate_logical_id_entry(vcpu);
>  
>  	if (ldr)
> @@ -464,7 +471,12 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>  {
>  	u64 *old, *new;
>  	struct vcpu_svm *svm = to_svm(vcpu);
> -	u32 id = kvm_xapic_id(vcpu->arch.apic);
> +	u32 id;
> +	int ret;
> +
> +	ret = kvm_get_apic_id(vcpu, &id);
> +	if (ret)
> +		return 1;

Well this function is totally broken anyway and I woudn't even bother touching it,
maximum, just stick 'return 0' in the very start of this function if the apic is
in x2apic mode now.

Oh well...

>  
>  	if (vcpu->vcpu_id == id)
>  		return 0;
> @@ -484,7 +496,8 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>  	 * APIC ID table entry if already setup the LDR.
>  	 */
>  	if (svm->ldr_reg)
> -		avic_handle_ldr_update(vcpu);
> +		if (avic_handle_ldr_update(vcpu))
> +			return 1;
>  
>  	return 0;
>  }


Best regards,
	Maxim Levitsky