linux-kernel - Re: [PATCH v1] KVM: x86: avoid redundant REQ

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]

Date:   Mon, 12 Dec 2016 23:20:35 +0300
From:   Roman Kagan <rkagan@...tuozzo.com>
To:     Radim Krčmář <rkrcmar@...hat.com>
CC:     Denis Plotnikov <dplotnikov@...tuozzo.com>, <den@...tuozzo.com>,
        <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] KVM: x86: avoid redundant REQ_EVENT

On Mon, Dec 12, 2016 at 05:29:43PM +0100, Radim Krčmář wrote:
> 2016-12-12 17:02+0300, Denis Plotnikov:
> > When processing KVM_REQ_EVENT, apic_update_ppr is called which may set
> > KVM_REQ_EVENT again if the recalculated value of PPR becomes smaller
> > than the previous one. This results in cancelling the guest entry and
> > reiterating in vcpu_enter_guest.
> > 
> > However this is unnecessary because at this point KVM_REQ_EVENT is
> > already being processed and there are no other changes in the lapic
> > that may require full-fledged state recalculation.
> > 
> > This situation is often hit on systems with TPR shadow, where the
> > TPR can be updated by the guest without a vmexit, so that the first
> > apic_update_ppr to notice it is exactly the one called while
> > processing KVM_REQ_EVENT.
> > 
> > To avoid it, introduce a parameter in apic_update_ppr allowing to
> > suppress setting of KVM_REQ_EVENT, and use it on the paths called from
> > KVM_REQ_EVENT processing.
> 
> We also call:
> 
>   kvm_cpu_get_interrupt() in nested_vmx_vmexit()
>     - that path is intended without KVM_REQ_EVENT
>   kvm_cpu_has_interrupt() in vmx_check_nested_events(),
>     - I think it does no harm
>   kvm_cpu_has_interrupt() in kvm_vcpu_has_events()
>   kvm_cpu_has_interrupt() in kvm_vcpu_ready_for_interrupt_injection()
>     - both seem safe as we should not have an interrupt between TPR
>       threshold and the new PPR value, so the KVM_REQ_EVENT was useless.
> 
> I would prefer we made sure that only callers from KVM_REQ_EVENT used
> the function we are changing -- it is really easy to make a hard-to-find
> mistake in interrupt delivery.

Indeed, that was my concern as well.  How about introducing a parameter
to kvm_cpu_{has,get}_interrupt() with the same meaning, and pass it down
to apic_update_ppr()?  Then only the call sites under KVM_REQ_EVENT
processing would pass "false" there, and the rest would remain with
"true"?

Roman.