linux-kernel - Re: [PATCH] Revert "KVM: x86: Unconditionally enable irqs in guest context"

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:   Wed, 6 Jan 2021 09:11:52 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     Nitesh Narayan Lal <nitesh@...hat.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        w90p710@...il.com, pbonzini@...hat.com,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] Revert "KVM: x86: Unconditionally enable irqs in guest
 context"

On Wed, Jan 06, 2021, Vitaly Kuznetsov wrote:
> Nitesh Narayan Lal <nitesh@...hat.com> writes:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 3f7c1fc7a3ce..3e17c9ffcad8 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9023,18 +9023,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  
> >  	kvm_x86_ops.handle_exit_irqoff(vcpu);
> >  
> > -	/*
> > -	 * Consume any pending interrupts, including the possible source of
> > -	 * VM-Exit on SVM 
> 
> I kind of liked this part of the comment, the new (old) one in
> svm_handle_exit_irqoff() doesn't actually explain what's going on.
> 
> > and any ticks that occur between VM-Exit and now.
> 
> Looking back, I don't quite understand why we wanted to account ticks
> between vmexit and exiting guest context as 'guest' in the first place;
> to my understanging 'guest time' is time spent within VMX non-root
> operation, the rest is KVM overhead (system).

With tick-based accounting, if the tick IRQ is received after PF_VCPU is cleared
then that tick will be accounted to the host/system.  The motivation for opening
an IRQ window after VM-Exit is to handle the case where the guest is constantly
exiting for a different reason _just_ before the tick arrives, e.g. if the guest
has its tick configured such that the guest and host ticks get synchronized
in a bad way.

This is a non-issue when using CONFIG_VIRT_CPU_ACCOUNTING_GEN=y, at least with a
stable TSC, as the accounting happens during guest_exit_irqoff() itself.
Accounting might be less-than-stellar if TSC is unstable, but I don't think it
would be as binary of a failure as tick-based accounting.

> It seems to match how the accounting is done nowadays after Tglx's
> 87fa7f3e98a1 ("x86/kvm: Move context tracking where it belongs").
> 
> > -	 * An instruction is required after local_irq_enable() to fully unblock
> > -	 * interrupts on processors that implement an interrupt shadow, the
> > -	 * stat.exits increment will do nicely.
> > -	 */
> > -	kvm_before_interrupt(vcpu);
> > -	local_irq_enable();
> >  	++vcpu->stat.exits;
> > -	local_irq_disable();
> > -	kvm_after_interrupt(vcpu);
> >  
> >  	if (lapic_in_kernel(vcpu)) {
> >  		s64 delta = vcpu->arch.apic->lapic_timer.advance_expire_delta;
> 
> FWIW,
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> 
> -- 
> Vitaly
>