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: <95dfb8f37cd218ea706ed8ee7c105656c282fcba.camel@redhat.com>
Subject: Re: [PATCH v4 20/32] KVM: x86: Skip redundant x2APIC logical mode
 optimized cluster setup
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        Li RongQing <lirongqing@baidu.com>
Date:   Thu, 08 Dec 2022 23:57:52 +0200
In-Reply-To: <20221001005915.2041642-21-seanjc@google.com>
References: <20221001005915.2041642-1-seanjc@google.com>
         <20221001005915.2041642-21-seanjc@google.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 Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> Skip the optimized cluster[] setup for x2APIC logical mode, as KVM reuses
> the optimized map's phys_map[] and doesn't actually need to insert the
> target apic into the cluster[].  The LDR is derived from the x2APIC ID,
> and both are read-only in KVM, thus the vCPU's cluster[ldr] is guaranteed
> to be the same entry as the vCPU's phys_map[x2apic_id] entry.
> 
> Skipping the unnecessary setup will allow a future fix for aliased xAPIC
> logical IDs to simply require that cluster[ldr] is non-NULL, i.e. won't
> have to special case x2APIC.

> Alternatively, the future check could allow "cluster[ldr] == apic", but
> that ends up being terribly confusing because cluster[ldr] is only set
> at the very end, i.e. it's only possible due to x2APIC's shenanigans.
> 
> Another alternative would be to send x2APIC down a separate path _after_
> the calculation and then assert that all of the above, but the resulting
> code is rather messy, and it's arguably unnecessary since asserting that
> the actual LDR matches the expected LDR means that simply testing that
> interrupts are delivered correctly provides the same guarantees.


Yes, I think it makes sense to not touch the cluser map in x2apic mode.
The map even explicitly named as for xapic (xapic_flat_map, xapic_cluster_map)

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

Best regards,
	Maxim Levitsky


> 
> Reported-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9989893fef69..fca8bbb44375 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -166,6 +166,11 @@ static bool kvm_use_posted_timer_interrupt(struct kvm_vcpu *vcpu)
>  	return kvm_can_post_timer_interrupt(vcpu) && vcpu->mode == IN_GUEST_MODE;
>  }
>  
> +static inline u32 kvm_apic_calc_x2apic_ldr(u32 id)
> +{
> +	return ((id >> 4) << 16) | (1 << (id & 0xf));
> +}
> +
>  static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
>  		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
>  	switch (map->logical_mode) {
> @@ -315,6 +320,18 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>  		}
>  		new->logical_mode = logical_mode;
>  
> +		/*
> +		 * In x2APIC mode, the LDR is read-only and derived directly
> +		 * from the x2APIC ID, thus is guaranteed to be addressable.
> +		 * KVM reuses kvm_apic_map.phys_map to optimize logical mode
> +		 * x2APIC interrupts by reversing the LDR calculation to get
> +		 * cluster of APICs, i.e. no additional work is required.
> +		 */
> +		if (apic_x2apic_mode(apic)) {
> +			WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(x2apic_id));
> +			continue;
> +		}
> +
>  		if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr,
>  								&cluster, &mask))) {
>  			new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
> @@ -381,11 +398,6 @@ static inline void kvm_apic_set_dfr(struct kvm_lapic *apic, u32 val)
>  	atomic_set_release(&apic->vcpu->kvm->arch.apic_map_dirty, DIRTY);
>  }
>  
> -static inline u32 kvm_apic_calc_x2apic_ldr(u32 id)
> -{
> -	return ((id >> 4) << 16) | (1 << (id & 0xf));
> -}
> -
>  static inline void kvm_apic_set_x2apic_id(struct kvm_lapic *apic, u32 id)
>  {
>  	u32 ldr = kvm_apic_calc_x2apic_ldr(id);