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: <26b02f98e88b1097ce5823007a7bc06b25678252.camel@redhat.com>
Subject: Re: [PATCH v2 06/12] KVM: SVM: Do not support updating APIC ID when
 in 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:   Mon, 18 Apr 2022 14:09:12 +0300
In-Reply-To: <20220412115822.14351-7-suravee.suthikulpanit@amd.com>
References: <20220412115822.14351-1-suravee.suthikulpanit@amd.com>
         <20220412115822.14351-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: 8bit
Precedence: bulk

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> In X2APIC mode, the Logical Destination Register is read-only,
> which provides a fixed mapping between the logical and physical
> APIC IDs. Therefore, there is no Logical APIC ID table in X2AVIC
> and the processor uses the X2APIC ID in the backing page to create
> a vCPU’s logical ID.
> 
> In addition, KVM does not support updating APIC ID in x2APIC mode,
> which means AVIC does not need to handle this case.
> 
> Therefore, check x2APIC mode when handling physical and logical
> APIC ID update, and when invalidating logical APIC ID table.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 609dcbe52a86..22ee1098e2a5 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -424,8 +424,13 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	bool flat = svm->dfr_reg == APIC_DFR_FLAT;
> -	u32 *entry = avic_get_logical_id_entry(vcpu, svm->ldr_reg, flat);
> +	u32 *entry;
>  
> +	/* Note: x2AVIC does not use logical APIC ID table */
> +	if (apic_x2apic_mode(vcpu->arch.apic))
> +		return;
> +
> +	entry = avic_get_logical_id_entry(vcpu, svm->ldr_reg, flat);
>  	if (entry)
>  		clear_bit(AVIC_LOGICAL_ID_ENTRY_VALID_BIT, (unsigned long *)entry);
>  }
> @@ -437,6 +442,10 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
>  	u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
>  	u32 id = kvm_xapic_id(vcpu->arch.apic);
>  
> +	/* AVIC does not support LDR update for x2APIC */
> +	if (apic_x2apic_mode(vcpu->arch.apic))
> +		return 0;
> +
>  	if (ldr == svm->ldr_reg)
>  		return 0;
>  
> @@ -457,6 +466,14 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u32 id = kvm_xapic_id(vcpu->arch.apic);
>  
> +	/*
> +	 * KVM does not support apic ID update for x2APIC.
> +	 * Also, need to check if the APIC ID exceed 254.
> +	 */
> +	if (apic_x2apic_mode(vcpu->arch.apic) ||
> +	    (vcpu->vcpu_id >= APIC_BROADCAST))
> +		return 0;
> +
>  	if (vcpu->vcpu_id == id)
>  		return 0;
>  

Honestly I won't even bother with this - avic_handle_apic_id_update
zeros the initial apic id entry, after it moves it contents to new location.

If it is called again (and it will be called next time there is avic inhibition),
it will just fail as the initial entry will be zero, or copy wrong entry
if something else was written there meanwhile.

That code just needs to be removed, and AVIC be conditional on read-only apic id.

But overall this patch doesn't make things worse, so

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky