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@xxxxxxxxxx>
Best regards,
Maxim Levitsky
>
> Reported-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> 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);
