| Message ID | 20220606180829.102503-5-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: AVIC/APICv patch queue | expand |
On 6/6/22 20:08, Maxim Levitsky wrote: > There are two issues in avic_kick_target_vcpus_fast > > 1. It is legal to issue an IPI request with APIC_DEST_NOSHORT > and a physical destination of 0xFF (or 0xFFFFFFFF in case of x2apic), > which must be treated as a broadcast destination. > > Fix this by explicitly checking for it. > Also don’t use ‘index’ in this case as it gives no new information. > > 2. It is legal to issue a logical IPI request to more than one target. > Index field only provides index in physical id table of first > such target and therefore can't be used before we are sure > that only a single target was addressed. > > Instead, parse the ICRL/ICRH, double check that a unicast interrupt > was requested, and use that info to figure out the physical id > of the target vCPU. > At that point there is no need to use the index field as well. > > > In addition to fixing the above issues, also skip the call to > kvm_apic_match_dest. > > It is possible to do this now, because now as long as AVIC is not > inhibited, it is guaranteed that none of the vCPUs changed their > apic id from its default value. > > > This fixes boot of windows guest with AVIC enabled because it uses > IPI with 0xFF destination and no destination shorthand. > > Fixes: 7223fd2d5338 ("KVM: SVM: Use target APIC ID to complete AVIC IRQs when possible") > Cc: stable@vger.kernel.org > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> Is it possible to use kvm_intr_is_single_vcpu_fast, or am I missing something? Series queued, thanks. Paolo
On Wed, 2022-06-08 at 15:21 +0200, Paolo Bonzini wrote: > On 6/6/22 20:08, Maxim Levitsky wrote: > > There are two issues in avic_kick_target_vcpus_fast > > > > 1. It is legal to issue an IPI request with APIC_DEST_NOSHORT > > and a physical destination of 0xFF (or 0xFFFFFFFF in case of x2apic), > > which must be treated as a broadcast destination. > > > > Fix this by explicitly checking for it. > > Also don’t use ‘index’ in this case as it gives no new information. > > > > 2. It is legal to issue a logical IPI request to more than one target. > > Index field only provides index in physical id table of first > > such target and therefore can't be used before we are sure > > that only a single target was addressed. > > > > Instead, parse the ICRL/ICRH, double check that a unicast interrupt > > was requested, and use that info to figure out the physical id > > of the target vCPU. > > At that point there is no need to use the index field as well. > > > > > > In addition to fixing the above issues, also skip the call to > > kvm_apic_match_dest. > > > > It is possible to do this now, because now as long as AVIC is not > > inhibited, it is guaranteed that none of the vCPUs changed their > > apic id from its default value. > > > > > > This fixes boot of windows guest with AVIC enabled because it uses > > IPI with 0xFF destination and no destination shorthand. > > > > Fixes: 7223fd2d5338 ("KVM: SVM: Use target APIC ID to complete AVIC IRQs when possible") > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > Is it possible to use kvm_intr_is_single_vcpu_fast, or am I missing > something? Yes, except that it needs 'struct kvm_lapic_irq' which we won't have when we emulate guest<->guest interrupts, and also it goes over apic map and such, which can be be skipped. It also does more unneeded things like dealing with low priority mode for example, which thankfully AVIC doenst' support and if attempted will still VM exit with 'incomplete IPI' but with AVIC_IPI_FAILURE_INVALID_INT_TYPE subreason, which goes through full APIC register emulation. I do think about the fact that ICRL/H parsing in the case of logical ID, (which depends on cluser mode and x2apic mode) can be moved to some common code, but I wasn't able yet to find a clean way to do it. BTW: there is another case where AVIC must be inhibited: in xapic mode, logical ids, don't have to have a single bit set in the mask area of the logical id, (low 4 bits in cluster mode and all 8 bits in flat mode) and neither there is a guarnantee that multilple CPUs don't share these bits. AVIC however has a logical ID table which maps each (bit x cluster value) to a physical id, and therefore a single vCPU, so tha later is not possible to support with AVIC. I haven't studied the code that is responsible for this, I will do this soon. Thankfully IPIv only supports physical IPI mode (this is what I heard, don't know for sure). I also will write a unit test for this very soon, to test various logical id IPIs, messing with logical id registers, etc, etc. Best regards, Maxim Levitsky > > Series queued, thanks. > > Paolo >
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 072e2c8cc66aa..5d98ac575dedc 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -291,58 +291,91 @@ void avic_ring_doorbell(struct kvm_vcpu *vcpu) static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source, u32 icrl, u32 icrh, u32 index) { - u32 dest, apic_id; - struct kvm_vcpu *vcpu; + u32 l1_physical_id, dest; + struct kvm_vcpu *target_vcpu; int dest_mode = icrl & APIC_DEST_MASK; int shorthand = icrl & APIC_SHORT_MASK; struct kvm_svm *kvm_svm = to_kvm_svm(kvm); - u32 *avic_logical_id_table = page_address(kvm_svm->avic_logical_id_table_page); if (shorthand != APIC_DEST_NOSHORT) return -EINVAL; - /* - * The AVIC incomplete IPI #vmexit info provides index into - * the physical APIC ID table, which can be used to derive - * guest physical APIC ID. - */ + if (apic_x2apic_mode(source)) + dest = icrh; + else + dest = GET_APIC_DEST_FIELD(icrh); + if (dest_mode == APIC_DEST_PHYSICAL) { - apic_id = index; + /* broadcast destination, use slow path */ + if (apic_x2apic_mode(source) && dest == X2APIC_BROADCAST) + return -EINVAL; + if (!apic_x2apic_mode(source) && dest == APIC_BROADCAST) + return -EINVAL; + + l1_physical_id = dest; + + if (WARN_ON_ONCE(l1_physical_id != index)) + return -EINVAL; + } else { - if (!apic_x2apic_mode(source)) { - /* For xAPIC logical mode, the index is for logical APIC table. */ - apic_id = avic_logical_id_table[index] & 0x1ff; + u32 bitmap, cluster; + int logid_index; + + if (apic_x2apic_mode(source)) { + /* 16 bit dest mask, 16 bit cluster id */ + bitmap = dest & 0xFFFF0000; + cluster = (dest >> 16) << 4; + } else if (kvm_lapic_get_reg(source, APIC_DFR) == APIC_DFR_FLAT) { + /* 8 bit dest mask*/ + bitmap = dest; + cluster = 0; } else { - return -EINVAL; + /* 4 bit desk mask, 4 bit cluster id */ + bitmap = dest & 0xF; + cluster = (dest >> 4) << 2; } - } - /* - * Assuming vcpu ID is the same as physical apic ID, - * and use it to retrieve the target vCPU. - */ - vcpu = kvm_get_vcpu_by_id(kvm, apic_id); - if (!vcpu) - return -EINVAL; + if (unlikely(!bitmap)) + /* guest bug: nobody to send the logical interrupt to */ + return 0; - if (apic_x2apic_mode(vcpu->arch.apic)) - dest = icrh; - else - dest = GET_APIC_DEST_FIELD(icrh); + if (!is_power_of_2(bitmap)) + /* multiple logical destinations, use slow path */ + return -EINVAL; - /* - * Try matching the destination APIC ID with the vCPU. - */ - if (kvm_apic_match_dest(vcpu, source, shorthand, dest, dest_mode)) { - vcpu->arch.apic->irr_pending = true; - svm_complete_interrupt_delivery(vcpu, - icrl & APIC_MODE_MASK, - icrl & APIC_INT_LEVELTRIG, - icrl & APIC_VECTOR_MASK); - return 0; + logid_index = cluster + __ffs(bitmap); + + if (apic_x2apic_mode(source)) { + l1_physical_id = logid_index; + } else { + u32 *avic_logical_id_table = + page_address(kvm_svm->avic_logical_id_table_page); + + u32 logid_entry = avic_logical_id_table[logid_index]; + + if (WARN_ON_ONCE(index != logid_index)) + return -EINVAL; + + /* guest bug: non existing/reserved logical destination */ + if (unlikely(!(logid_entry & AVIC_LOGICAL_ID_ENTRY_VALID_MASK))) + return 0; + + l1_physical_id = logid_entry & + AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK; + } } - return -EINVAL; + target_vcpu = kvm_get_vcpu_by_id(kvm, l1_physical_id); + if (unlikely(!target_vcpu)) + /* guest bug: non existing vCPU is a target of this IPI*/ + return 0; + + target_vcpu->arch.apic->irr_pending = true; + svm_complete_interrupt_delivery(target_vcpu, + icrl & APIC_MODE_MASK, + icrl & APIC_INT_LEVELTRIG, + icrl & APIC_VECTOR_MASK); + return 0; } static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
There are two issues in avic_kick_target_vcpus_fast 1. It is legal to issue an IPI request with APIC_DEST_NOSHORT and a physical destination of 0xFF (or 0xFFFFFFFF in case of x2apic), which must be treated as a broadcast destination. Fix this by explicitly checking for it. Also don’t use ‘index’ in this case as it gives no new information. 2. It is legal to issue a logical IPI request to more than one target. Index field only provides index in physical id table of first such target and therefore can't be used before we are sure that only a single target was addressed. Instead, parse the ICRL/ICRH, double check that a unicast interrupt was requested, and use that info to figure out the physical id of the target vCPU. At that point there is no need to use the index field as well. In addition to fixing the above issues, also skip the call to kvm_apic_match_dest. It is possible to do this now, because now as long as AVIC is not inhibited, it is guaranteed that none of the vCPUs changed their apic id from its default value. This fixes boot of windows guest with AVIC enabled because it uses IPI with 0xFF destination and no destination shorthand. Fixes: 7223fd2d5338 ("KVM: SVM: Use target APIC ID to complete AVIC IRQs when possible") Cc: stable@vger.kernel.org Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/avic.c | 105 ++++++++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 36 deletions(-)