linux-kernel - Re: [PATCH v2] kvm: ioapic: Introduce arch-specific check for lazy update EOI mechanism

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] [day] [month] [year] [list]

Message-ID: <a78c88fa-5d2b-1329-9200-922e1c95e730@redhat.com>
Date:   Mon, 4 May 2020 11:19:56 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     rkrcmar@...hat.com, joro@...tes.org, jon.grimm@....com,
        borisvk@...net.org
Subject: Re: [PATCH v2] kvm: ioapic: Introduce arch-specific check for lazy
 update EOI mechanism

On 04/05/20 08:31, Suravee Suthikulpanit wrote:
>>
>>
>> The re-entrancy happens because the irq state is the OR of
>> the interrupt state and the resamplefd state.  That is, we don't
>> want to show the state as 0 until we've had a chance to set the
>> resamplefd.  But if the interrupt has _not_ gone low then we get an
>> infinite loop.
> 
> I'm not too familiar w/ the resamplefd.  I must have missed this part.
> Could you please point out to me where the OR logic is?

It's because kvm_ioapic_set_irq gets the actual state of the interrupt
line from __kvm_irq_line_state before calling ioapic_set_irq.

>> But in the case of level-triggered interrupts the VMEXIT already
>> happens because TMR is set; only edge-triggered interrupts need
>> the lazy invocation of the ack notifier.  
> 
> For AVIC, EOI write for level-triggered would have also be trapped.

Yes, I was talking about AVIC only there.

> And yes, edge-triggered needs lazy ack notifier.
> 
>> So this should be the fix:
>>
>> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
>> index 7668fed1ce65..ca2d73cd00a3 100644
>> --- a/arch/x86/kvm/ioapic.c
>> +++ b/arch/x86/kvm/ioapic.c
>> @@ -225,12 +225,12 @@ static int ioapic_set_irq(struct kvm_ioapic
>> *ioapic, unsigned int irq,
>>       }
>>         /*
>> -     * AMD SVM AVIC accelerate EOI write and do not trap,
>> -     * in-kernel IOAPIC will not be able to receive the EOI.
>> +     * AMD SVM AVIC accelerate EOI write iff the interrupt is level
>> +     * triggered, in-kernel IOAPIC will not be able to receive the EOI.
> 
> Actually, it should be "AMD SVM AVIC accelerate EOI write iff the
> interrupt is _edge_ triggered".

Of course, thanks.

Paolo