On Tue, Apr 11, 2017 at 04:11:15PM +0200, Ladi Prosek wrote:
> If the guest takes advantage of the directed EOI feature by setting
> APIC_SPIV_DIRECTED_EOI, it is expected to signal EOI by writing to
> the EOI register of the respective IOAPIC.
>
> From Intel's x2APIC Specification:
> "following the EOI to the local x2APIC unit for a level triggered
> interrupt, perform a directed EOI to the IOxAPIC generating the
> interrupt by writing to its EOI register."
>
> Commit fc61b800f950 ("KVM: Add Directed EOI support to APIC emulation")
> inhibited EOI on LAPIC EOI register write but didn't add the IOAPIC
> part. IOAPIC_REG_EOI writes were handled only on IA64 and the code
> was later removed with the rest of IA64 support.
>
> The bug has gone undetected for a long time because Linux writes to
> IOAPIC_REG_EOI only if the IOAPIC version is >=0x20. Windows doesn't
> seem to perform such a check.
Hi, Ladi,
Not sure I'm understanding it correctly... I see "direct EOI" is a
feature for IOAPIC version 0x20, while "suppress EOI-broadcast" is
another feature for APIC. They are not the same feature, so it may not
be required to have them all together. IIUC current x86 kvm is just
the case - it supports EOI broadcast suppression on APIC, but it does
not support direct EOI on kernel IOAPIC.
I think the problem is why the guest setup APIC_SPIV_DIRECTED_EOI even
if IOAPIC does not support direct EOI (the guest can know it by
probing IOAPIC version). Please correct if I'm wrong.
>
> This commit re-adds IOAPIC_REG_EOI and implements it in terms of
> __kvm_ioapic_update_eoi.
>
> Fixes: fc61b800f950 ("KVM: Add Directed EOI support to APIC emulation")
> Signed-off-by: Ladi Prosek <lprosek@xxxxxxxxxx>
> ---
> arch/x86/kvm/ioapic.c | 46 ++++++++++++++++++++++++++++------------------
> arch/x86/kvm/ioapic.h | 1 +
> 2 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 289270a..8df1c6c 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -415,14 +415,15 @@ static void kvm_ioapic_eoi_inject_work(struct work_struct *work)
> #define IOAPIC_SUCCESSIVE_IRQ_MAX_COUNT 10000
>
> static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
> - struct kvm_ioapic *ioapic, int vector, int trigger_mode)
> + struct kvm_ioapic *ioapic, int vector, int trigger_mode,
> + bool directed)
> {
> struct dest_map *dest_map = &ioapic->rtc_status.dest_map;
> struct kvm_lapic *apic = vcpu->arch.apic;
> int i;
>
> /* RTC special handling */
> - if (test_bit(vcpu->vcpu_id, dest_map->map) &&
> + if (!directed && test_bit(vcpu->vcpu_id, dest_map->map) &&
> vector == dest_map->vectors[vcpu->vcpu_id])
> rtc_irq_eoi(ioapic, vcpu);
>
> @@ -432,21 +433,23 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
> if (ent->fields.vector != vector)
> continue;
>
> - /*
> - * We are dropping lock while calling ack notifiers because ack
> - * notifier callbacks for assigned devices call into IOAPIC
> - * recursively. Since remote_irr is cleared only after call
> - * to notifiers if the same vector will be delivered while lock
> - * is dropped it will be put into irr and will be delivered
> - * after ack notifier returns.
> - */
> - spin_unlock(&ioapic->lock);
> - kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i);
> - spin_lock(&ioapic->lock);
> -
> - if (trigger_mode != IOAPIC_LEVEL_TRIG ||
> - kvm_lapic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)
> - continue;
> + if (!directed) {
Could I ask why we need to skip this if the EOI is sent via direct EOI
register of IOAPIC?
Thanks,
> + /*
> + * We are dropping lock while calling ack notifiers because ack
> + * notifier callbacks for assigned devices call into IOAPIC
> + * recursively. Since remote_irr is cleared only after call
> + * to notifiers if the same vector will be delivered while lock
> + * is dropped it will be put into irr and will be delivered
> + * after ack notifier returns.
> + */
> + spin_unlock(&ioapic->lock);
> + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i);
> + spin_lock(&ioapic->lock);
> +
> + if (trigger_mode != IOAPIC_LEVEL_TRIG ||
> + kvm_lapic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)
> + continue;
> + }
>
> ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
> ent->fields.remote_irr = 0;
> @@ -478,7 +481,7 @@ void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector, int trigger_mode)
> struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
>
> spin_lock(&ioapic->lock);
> - __kvm_ioapic_update_eoi(vcpu, ioapic, vector, trigger_mode);
> + __kvm_ioapic_update_eoi(vcpu, ioapic, vector, trigger_mode, false);
> spin_unlock(&ioapic->lock);
> }
>
> @@ -540,6 +543,7 @@ static int ioapic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> gpa_t addr, int len, const void *val)
> {
> struct kvm_ioapic *ioapic = to_ioapic(this);
> + struct kvm_lapic *apic = vcpu->arch.apic;
> u32 data;
> if (!ioapic_in_range(ioapic, addr))
> return -EOPNOTSUPP;
> @@ -575,6 +579,12 @@ static int ioapic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> ioapic_write_indirect(ioapic, data);
> break;
>
> + case IOAPIC_REG_EOI:
> + if (kvm_lapic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)
> + __kvm_ioapic_update_eoi(vcpu, ioapic, data,
> + IOAPIC_LEVEL_TRIG, true);
> + break;
> +
> default:
> break;
> }
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index 1cc6e54..251b61b 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -20,6 +20,7 @@ struct kvm_vcpu;
> /* Direct registers. */
> #define IOAPIC_REG_SELECT 0x00
> #define IOAPIC_REG_WINDOW 0x10
> +#define IOAPIC_REG_EOI 0x40
>
> /* Indirect registers. */
> #define IOAPIC_REG_APIC_ID 0x00 /* x86 IOAPIC only */
> --
> 2.9.3
>
--
Peter Xu
