linux-kernel - Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151223171932.GB7061@potion.redhat.com>
Date:	Wed, 23 Dec 2015 18:19:33 +0100
From:	Radim Krčmář <rkrcmar@...hat.com>
To:	Feng Wu <feng.wu@...el.com>
Cc:	pbonzini@...hat.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver
 lowest-priority interrupts

2015-12-16 09:37+0800, Feng Wu:
> Use vector-hashing to deliver lowest-priority interrupts, As an
> example, modern Intel CPUs in server platform use this method to
> handle lowest-priority interrupts.
> 
> Signed-off-by: Feng Wu <feng.wu@...el.com>
> ---
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -78,13 +83,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  				r = 0;
>  			r += kvm_apic_set_irq(vcpu, irq, dest_map);
>  		} else if (kvm_lapic_enabled(vcpu)) {
> -			if (!lowest)
> -				lowest = vcpu;
> -			else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> -				lowest = vcpu;
> +			if (!kvm_vector_hashing_enabled()) {
> +				if (!lowest)
> +					lowest = vcpu;
> +				else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> +					lowest = vcpu;
> +			} else {
> +				__set_bit(vcpu->vcpu_id, dest_vcpu_bitmap);
> +				dest_vcpus++;
> +			}
>  		}
>  	}
>  
> +	if (dest_vcpus != 0) {
> +		idx = kvm_vector_2_index(irq->vector, dest_vcpus,
> +					 dest_vcpu_bitmap, KVM_MAX_VCPUS);
> +
> +		lowest = kvm_get_vcpu(kvm, idx - 1);

This is going to fail with sparse topologies (e.g. 3 cores per socket).
vcpu_id = initial APIC ID and kvm_get_vcpu() uses a compressed array
that has kvm->online_vcpus elements, so we could overflow.

The 'i' in kvm_for_each_vcpu() could be used for the bitmap.
(kvm_get_vcpu_by_id() instead of kvm_get_vcpu() is slightly worse.)

> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -678,6 +678,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>  {
> @@ -731,17 +747,38 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> +			if (!kvm_vector_hashing_enabled()) {
| [...]
> +			} else {
> +				int idx = 0;
> +				unsigned int dest_vcpus = 0;

Now that we don't need to check for present/enabled LAPICs, I think it
would be better to solve this by assuming that all selected LAPICs are
enabled, so the n-th target is decided only based on vector and
destination.

> +				for_each_set_bit(i, &bitmap, 16) {
> +					if (!dst[i] && !kvm_lapic_enabled(dst[i]->vcpu)) {
> +						__clear_bit(i, &bitmap);
> +						continue;
> +					}
> +				}

=> we could skip this loop.

> +
> +				dest_vcpus = hweight16(bitmap);
> +
> +				if (dest_vcpus != 0) {
> +					idx = kvm_vector_2_index(irq->vector,
> +						dest_vcpus, &bitmap, 16);
> +
> +					bitmap = 0;
> +					__set_bit(idx-1, &bitmap);

And set just this bit.

The drawback is that buggy software that included hardware disabled
APICs to lowest priority destinations could stop working ...
Do you think it's too risky?

> +				}
>  			}

(This is basically the same as converting the message to a fixed delivery
 to n-th bit beforehand, so it might be reasonable to to apply something
 similar to simplify the slow path as well.  Mixed flat/cluster/x2APIC
 mode makes me suspect that it won't be reasonable.)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/