On 2013-06-26 08:15, Gleb Natapov wrote:
> On Wed, Jun 26, 2013 at 07:49:37AM +0200, Jan Kiszka wrote:
>> On 2013-06-24 14:19, Gleb Natapov wrote:
>>> This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
>>> the commit kvm_apic_set_irq() no longer returns accurate information
>>> about interrupt injection status if injection is done into disabled
>>> APIC. RTC interrupt coalescing tracking relies on the information to be
>>> accurate and cannot recover if it is not.
>>>
>>> Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 9d75193..9f4bea8 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
>>> return highest_irr;
>>> }
>>>
>>> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> - int vector, int level, int trig_mode,
>>> - unsigned long *dest_map);
>>> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> + int vector, int level, int trig_mode,
>>> + unsigned long *dest_map);
>>
>> I still think __apic_accept_irq should unconditionally inject, and the
>> test for acpi_enabled belongs into kvm_apic_set_irq. Why should
>> __apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
>> See below for another reason to refactor this part of the interface.
>>
> 10.4.7.2 Local APIC State After It Has Been Software Disabled
>
> The local APIC will respond normally to INIT, NMI, SMI, and SIPI
> messages.
OK, I see.
>
>>>
>>> -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
>>> - unsigned long *dest_map)
>>> +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
>>> + unsigned long *dest_map)
>>> {
>>> struct kvm_lapic *apic = vcpu->arch.apic;
>>>
>>> - __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
>>> - irq->level, irq->trig_mode, dest_map);
>>> + return __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
>>> + irq->level, irq->trig_mode, dest_map);
>>> }
>>>
>>> static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
>>> @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>>> *r = -1;
>>>
>>> if (irq->shorthand == APIC_DEST_SELF) {
>>> - kvm_apic_set_irq(src->vcpu, irq, dest_map);
>>> - *r = 1;
>>> + *r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
>>> return true;
>>> }
>>>
>>> @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>>> continue;
>>> if (*r < 0)
>>> *r = 0;
>>> - kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
>>> - *r += 1;
>>> + *r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
>>> }
>>>
>>> ret = true;
>>> @@ -664,11 +662,15 @@ out:
>>> return ret;
>>> }
>>>
>>> -/* Set an IRQ pending in the lapic. */
>>> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> - int vector, int level, int trig_mode,
>>> - unsigned long *dest_map)
>>> +/*
>>> + * Add a pending IRQ into lapic.
>>> + * Return 1 if successfully added and 0 if discarded.
>>> + */
>>> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> + int vector, int level, int trig_mode,
>>> + unsigned long *dest_map)
>>> {
>>> + int result = 0;
>>> struct kvm_vcpu *vcpu = apic->vcpu;
>>>
>>> switch (delivery_mode) {
>>> @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>> if (dest_map)
>>> __set_bit(vcpu->vcpu_id, dest_map);
>>>
>>> - if (kvm_x86_ops->deliver_posted_interrupt)
>>> + if (kvm_x86_ops->deliver_posted_interrupt) {
>>> + result = 1;
>>> kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
>>> - else {
>>> - if (apic_test_and_set_irr(vector, apic)) {
>>> + } else {
>>> + result = !apic_test_and_set_irr(vector, apic);
>>
>> This part of the revert makes no sense. If deliver_posted_interrupt is
>> on, we don't have this feedback anymore, thus we decided to remove it, no?
>>
> Agree, but I wanted to do clear revert and fix on top.
Fine with me, let's write a separate fix.
Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
