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

[Xinlong Lin]

On 2021-01-07 at 01:11, Sean Christopherson wrote:
>On Wed, Jan 06, 2021, Vitaly Kuznetsov wrote:
>> Nitesh Narayan Lal <nitesh@xxxxxxxxxx> 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. 

If I don't specify "nohz_full" in boot command line when using
CONFIG_VIRT_CPU_ACCOUNTING_GEN=y, Will the problem still exist?

>
>> 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@xxxxxxxxxx>
>>
>> --
>> Vitaly
>>