[PATCH v2 0/3] KVM: Properly account for guest CPU time

[PATCH v2 0/3] KVM: Properly account for guest CPU time

[Wanpeng Li]

The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
reported that the guest time remains 0 when running a while true
loop in the guest.

The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
belongs") moves guest_exit_irqoff() close to vmexit breaks the
tick-based time accouting when the ticks that happen after IRQs are
disabled are incorrectly accounted to the host/system time. This is
because we exit the guest state too early.

This patchset splits both context tracking logic and the time accounting 
logic from guest_enter/exit_irqoff(), keep context tracking around the 
actual vmentry/exit code, have the virt time specific helpers which 
can be placed at the proper spots in kvm. In addition, it will not 
break the world outside of x86.

v1 -> v2:
 * split context_tracking from guest_enter/exit_irqoff
 * provide separate vtime accounting functions for consistent
 * place the virt time specific helpers at the proper splot 

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>

Wanpeng Li (3):
  context_tracking: Split guest_enter/exit_irqoff
  context_tracking: Provide separate vtime accounting functions
  x86/kvm: Fix vtime accounting

 arch/x86/kvm/svm/svm.c           |  6 ++-
 arch/x86/kvm/vmx/vmx.c           |  6 ++-
 arch/x86/kvm/x86.c               |  1 +
 include/linux/context_tracking.h | 84 +++++++++++++++++++++++++++++++---------
 4 files changed, 74 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

Split context_tracking part from guest_enter/exit_irqoff, it will be 
called separately in later patches.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 include/linux/context_tracking.h | 42 +++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index bceb064..d8ad844 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -104,16 +104,8 @@ static inline void context_tracking_init(void) { }
 
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
-/* must be called with irqs disabled */
-static __always_inline void guest_enter_irqoff(void)
+static __always_inline void context_guest_enter_irqoff(void)
 {
-	instrumentation_begin();
-	if (vtime_accounting_enabled_this_cpu())
-		vtime_guest_enter(current);
-	else
-		current->flags |= PF_VCPU;
-	instrumentation_end();
-
 	if (context_tracking_enabled())
 		__context_tracking_enter(CONTEXT_GUEST);
 
@@ -131,10 +123,28 @@ static __always_inline void guest_enter_irqoff(void)
 	}
 }
 
-static __always_inline void guest_exit_irqoff(void)
+/* must be called with irqs disabled */
+static __always_inline void guest_enter_irqoff(void)
+{
+	instrumentation_begin();
+	if (vtime_accounting_enabled_this_cpu())
+		vtime_guest_enter(current);
+	else
+		current->flags |= PF_VCPU;
+	instrumentation_end();
+
+	context_guest_enter_irqoff();
+}
+
+static __always_inline void context_guest_exit_irqoff(void)
 {
 	if (context_tracking_enabled())
 		__context_tracking_exit(CONTEXT_GUEST);
+}
+
+static __always_inline void guest_exit_irqoff(void)
+{
+	context_guest_exit_irqoff();
 
 	instrumentation_begin();
 	if (vtime_accounting_enabled_this_cpu())
@@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
 }
 
 #else
+static __always_inline void context_guest_enter_irqoff(void)
+{
+	instrumentation_begin();
+	rcu_virt_note_context_switch(smp_processor_id());
+	instrumentation_end();
+}
+
 static __always_inline void guest_enter_irqoff(void)
 {
 	/*
@@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
 	instrumentation_begin();
 	vtime_account_kernel(current);
 	current->flags |= PF_VCPU;
-	rcu_virt_note_context_switch(smp_processor_id());
 	instrumentation_end();
+
+	context_guest_enter_irqoff();
 }
 
+static __always_inline void context_guest_exit_irqoff(void) { }
+
 static __always_inline void guest_exit_irqoff(void)
 {
 	instrumentation_begin();
-- 
2.7.4

[PATCH v2 2/3] context_tracking: Provide separate vtime accounting functions

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

Provide separate vtime accounting functions, because having proper 
wrappers for that case would be too consistent and less confusing.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 include/linux/context_tracking.h | 50 ++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index d8ad844..491f889 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -102,6 +102,40 @@ extern void context_tracking_init(void);
 static inline void context_tracking_init(void) { }
 #endif /* CONFIG_CONTEXT_TRACKING_FORCE */
 
+static __always_inline void vtime_account_guest_enter(void)
+{
+	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_GEN)) {
+		if (vtime_accounting_enabled_this_cpu())
+			vtime_guest_enter(current);
+		else
+			current->flags |= PF_VCPU;
+	} else {
+		vtime_account_kernel(current);
+		current->flags |= PF_VCPU;
+	}
+}
+
+static __always_inline void __vtime_account_guest_exit(void)
+{
+	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_GEN)) {
+		if (vtime_accounting_enabled_this_cpu())
+			vtime_guest_exit(current);
+	} else {
+		vtime_account_kernel(current);
+	}
+}
+
+static __always_inline void vtime_account_guest_exit(void)
+{
+	__vtime_account_guest_exit();
+	current->flags &= ~PF_VCPU;
+}
+
+static __always_inline void vcpu_account_guest_exit(void)
+{
+	if (!vtime_accounting_enabled_this_cpu())
+		current->flags &= ~PF_VCPU;
+}
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
 static __always_inline void context_guest_enter_irqoff(void)
@@ -127,10 +161,7 @@ static __always_inline void context_guest_enter_irqoff(void)
 static __always_inline void guest_enter_irqoff(void)
 {
 	instrumentation_begin();
-	if (vtime_accounting_enabled_this_cpu())
-		vtime_guest_enter(current);
-	else
-		current->flags |= PF_VCPU;
+	vtime_account_guest_enter();
 	instrumentation_end();
 
 	context_guest_enter_irqoff();
@@ -147,10 +178,7 @@ static __always_inline void guest_exit_irqoff(void)
 	context_guest_exit_irqoff();
 
 	instrumentation_begin();
-	if (vtime_accounting_enabled_this_cpu())
-		vtime_guest_exit(current);
-	else
-		current->flags &= ~PF_VCPU;
+	vtime_account_guest_exit();
 	instrumentation_end();
 }
 
@@ -170,8 +198,7 @@ static __always_inline void guest_enter_irqoff(void)
 	 * to flush.
 	 */
 	instrumentation_begin();
-	vtime_account_kernel(current);
-	current->flags |= PF_VCPU;
+	vtime_account_guest_enter();
 	instrumentation_end();
 
 	context_guest_enter_irqoff();
@@ -183,8 +210,7 @@ static __always_inline void guest_exit_irqoff(void)
 {
 	instrumentation_begin();
 	/* Flush the guest cputime we spent on the guest */
-	vtime_account_kernel(current);
-	current->flags &= ~PF_VCPU;
+	vtime_account_guest_exit();
 	instrumentation_end();
 }
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_GEN */
-- 
2.7.4

[PATCH v2 3/3] x86/kvm: Fix vtime accounting

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
reported that the guest time remains 0 when running a while true
loop in the guest.

The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
belongs") moves guest_exit_irqoff() close to vmexit breaks the
tick-based time accouting when the ticks that happen after IRQs are
disabled are incorrectly accounted to the host/system time. This is
because we exit the guest state too early.

Keep context tracking around the actual vmentry/exit code, the time 
accounting logic is separated out by preposed patch from 
guest_enter/exit_irqoff(). Keep vtime-based time accounting around 
the actual vmentry/exit code when vtime_accounting_enabled_this_cpu() 
is true, leave PF_VCPU clearing after handle_exit_irqoff() and explicit 
IRQ window for tick-based time accouting.

Fixes: 87fa7f3e98a131 ("x86/kvm: Move context tracking where it belongs")
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Cc: stable@vger.kernel.org#v5.9-rc1+
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/svm/svm.c | 6 ++++--
 arch/x86/kvm/vmx/vmx.c | 6 ++++--
 arch/x86/kvm/x86.c     | 1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 48b396f3..2a4e284 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3726,11 +3726,12 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu)
 	 * accordingly.
 	 */
 	instrumentation_begin();
+	vtime_account_guest_enter();
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	instrumentation_end();
 
-	guest_enter_irqoff();
+	context_guest_enter_irqoff();
 	lockdep_hardirqs_on(CALLER_ADDR0);
 
 	if (sev_es_guest(vcpu->kvm)) {
@@ -3758,10 +3759,11 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu)
 	 * into world and some more.
 	 */
 	lockdep_hardirqs_off(CALLER_ADDR0);
-	guest_exit_irqoff();
+	context_guest_exit_irqoff();
 
 	instrumentation_begin();
 	trace_hardirqs_off_finish();
+	__vtime_account_guest_exit();
 	instrumentation_end();
 }
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c05e6e2..23be956 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6613,11 +6613,12 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 	 * accordingly.
 	 */
 	instrumentation_begin();
+	vtime_account_guest_enter();
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	instrumentation_end();
 
-	guest_enter_irqoff();
+	context_guest_enter_irqoff();
 	lockdep_hardirqs_on(CALLER_ADDR0);
 
 	/* L1D Flush includes CPU buffer clear to mitigate MDS */
@@ -6647,10 +6648,11 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 	 * into world and some more.
 	 */
 	lockdep_hardirqs_off(CALLER_ADDR0);
-	guest_exit_irqoff();
+	context_guest_exit_irqoff();
 
 	instrumentation_begin();
 	trace_hardirqs_off_finish();
+	__vtime_account_guest_exit();
 	instrumentation_end();
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ce9a1d2..0d2dd3f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9245,6 +9245,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	++vcpu->stat.exits;
 	local_irq_disable();
 	kvm_after_interrupt(vcpu);
+	vcpu_account_guest_exit();
 
 	if (lapic_in_kernel(vcpu)) {
 		s64 delta = vcpu->arch.apic->lapic_timer.advance_expire_delta;
-- 
2.7.4

Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff

[Christian Borntraeger]

On 13.04.21 09:16, Wanpeng Li wrote:
[...]

> @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
>   }
> 
>   #else
> +static __always_inline void context_guest_enter_irqoff(void)
> +{
> +	instrumentation_begin();
> +	rcu_virt_note_context_switch(smp_processor_id());
> +	instrumentation_end();
> +}
> +
>   static __always_inline void guest_enter_irqoff(void)
>   {
>   	/*
> @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
>   	instrumentation_begin();
>   	vtime_account_kernel(current);
>   	current->flags |= PF_VCPU;
> -	rcu_virt_note_context_switch(smp_processor_id());
>   	instrumentation_end();
> +
> +	context_guest_enter_irqoff();

So we now do instrumentation_begin 2 times?

Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff

[Wanpeng Li]

On Tue, 13 Apr 2021 at 15:35, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
>
>
> On 13.04.21 09:16, Wanpeng Li wrote:
> [...]
>
> > @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
> >   }
> >
> >   #else
> > +static __always_inline void context_guest_enter_irqoff(void)
> > +{
> > +     instrumentation_begin();
> > +     rcu_virt_note_context_switch(smp_processor_id());
> > +     instrumentation_end();
> > +}
> > +
> >   static __always_inline void guest_enter_irqoff(void)
> >   {
> >       /*
> > @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
> >       instrumentation_begin();
> >       vtime_account_kernel(current);
> >       current->flags |= PF_VCPU;
> > -     rcu_virt_note_context_switch(smp_processor_id());
> >       instrumentation_end();
> > +
> > +     context_guest_enter_irqoff();
>
> So we now do instrumentation_begin 2 times?

Similar to context_guest_enter_irqoff() ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN.

    Wanpeng

Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff

[Christian Borntraeger]

On 13.04.21 09:38, Wanpeng Li wrote:
> On Tue, 13 Apr 2021 at 15:35, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>>
>>
>>
>> On 13.04.21 09:16, Wanpeng Li wrote:
>> [...]
>>
>>> @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
>>>    }
>>>
>>>    #else
THis is the else part


>>> +static __always_inline void context_guest_enter_irqoff(void)
>>> +{
>>> +     instrumentation_begin();

2nd on
>>> +     rcu_virt_note_context_switch(smp_processor_id());
>>> +     instrumentation_end();
2nd off
>>> +}
>>> +
>>>    static __always_inline void guest_enter_irqoff(void)
>>>    {
>>>        /*
>>> @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
>>>        instrumentation_begin();

first on
>>>        vtime_account_kernel(current);
>>>        current->flags |= PF_VCPU;
>>> -     rcu_virt_note_context_switch(smp_processor_id());
>>>        instrumentation_end();

first off
>>> +
>>> +     context_guest_enter_irqoff();
here we call the 2nd on and off.
>>
>> So we now do instrumentation_begin 2 times?
> 
> Similar to context_guest_enter_irqoff() ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN.

For the
ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN part
context_guest_enter_irqoff()
does not have instrumentation_begin/end.

Or did I miss anything.

Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff

[Wanpeng Li]

On Tue, 13 Apr 2021 at 15:48, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
>
>
> On 13.04.21 09:38, Wanpeng Li wrote:
> > On Tue, 13 Apr 2021 at 15:35, Christian Borntraeger
> > <borntraeger@de.ibm.com> wrote:
> >>
> >>
> >>
> >> On 13.04.21 09:16, Wanpeng Li wrote:
> >> [...]
> >>
> >>> @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
> >>>    }
> >>>
> >>>    #else
> THis is the else part
>
>
> >>> +static __always_inline void context_guest_enter_irqoff(void)
> >>> +{
> >>> +     instrumentation_begin();
>
> 2nd on
> >>> +     rcu_virt_note_context_switch(smp_processor_id());
> >>> +     instrumentation_end();
> 2nd off
> >>> +}
> >>> +
> >>>    static __always_inline void guest_enter_irqoff(void)
> >>>    {
> >>>        /*
> >>> @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
> >>>        instrumentation_begin();
>
> first on
> >>>        vtime_account_kernel(current);
> >>>        current->flags |= PF_VCPU;
> >>> -     rcu_virt_note_context_switch(smp_processor_id());
> >>>        instrumentation_end();
>
> first off
> >>> +
> >>> +     context_guest_enter_irqoff();
> here we call the 2nd on and off.
> >>
> >> So we now do instrumentation_begin 2 times?
> >
> > Similar to context_guest_enter_irqoff() ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN.
>
> For the
> ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN part
> context_guest_enter_irqoff()
> does not have instrumentation_begin/end.
>
> Or did I miss anything.

I mean the if (!context_tracking_enabled_this_cpu()) part in the
function context_guest_enter_irqoff() ifdef
CONFIG_VIRT_CPU_ACCOUNTING_GEN. :)

    Wanpeng

Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff

[Christian Borntraeger]

On 13.04.21 09:52, Wanpeng Li wrote:
>> Or did I miss anything.
> 
> I mean the if (!context_tracking_enabled_this_cpu()) part in the
> function context_guest_enter_irqoff() ifdef
> CONFIG_VIRT_CPU_ACCOUNTING_GEN. :)

Ah I missed that. Thanks.

Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time

[Christian Borntraeger]

On 13.04.21 09:16, Wanpeng Li wrote:
> The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> reported that the guest time remains 0 when running a while true
> loop in the guest.
> 
> The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> belongs") moves guest_exit_irqoff() close to vmexit breaks the
> tick-based time accouting when the ticks that happen after IRQs are
> disabled are incorrectly accounted to the host/system time. This is
> because we exit the guest state too early.
> 
> This patchset splits both context tracking logic and the time accounting
> logic from guest_enter/exit_irqoff(), keep context tracking around the
> actual vmentry/exit code, have the virt time specific helpers which
> can be placed at the proper spots in kvm. In addition, it will not
> break the world outside of x86.
> 
> v1 -> v2:
>   * split context_tracking from guest_enter/exit_irqoff
>   * provide separate vtime accounting functions for consistent
>   * place the virt time specific helpers at the proper splot
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> 
> Wanpeng Li (3):
>    context_tracking: Split guest_enter/exit_irqoff
>    context_tracking: Provide separate vtime accounting functions
>    x86/kvm: Fix vtime accounting
> 
>   arch/x86/kvm/svm/svm.c           |  6 ++-
>   arch/x86/kvm/vmx/vmx.c           |  6 ++-
>   arch/x86/kvm/x86.c               |  1 +
>   include/linux/context_tracking.h | 84 +++++++++++++++++++++++++++++++---------
>   4 files changed, 74 insertions(+), 23 deletions(-)
> 

The non CONFIG_VIRT_CPU_ACCOUNTING_GEN look good.

Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time

[Sean Christopherson]

On Tue, Apr 13, 2021, Wanpeng Li wrote:
> The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> reported that the guest time remains 0 when running a while true
> loop in the guest.
> 
> The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> belongs") moves guest_exit_irqoff() close to vmexit breaks the
> tick-based time accouting when the ticks that happen after IRQs are
> disabled are incorrectly accounted to the host/system time. This is
> because we exit the guest state too early.
> 
> This patchset splits both context tracking logic and the time accounting 
> logic from guest_enter/exit_irqoff(), keep context tracking around the 
> actual vmentry/exit code, have the virt time specific helpers which 
> can be placed at the proper spots in kvm. In addition, it will not 
> break the world outside of x86.

IMO, this is going in the wrong direction.  Rather than separate context tracking,
vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
logic baked into it.

Rather than smush everything into context_tracking.h, I think we can cleanly
split the context tracking and vtime accounting code into separate pieces, which
will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
done, splitting the context tracking and time accounting logic for KVM x86
becomes a KVM detail as opposed to requiring dedicated logic in the context
tracking code.

I have untested code that compiles on x86, I'll send an RFC shortly.

> v1 -> v2:
>  * split context_tracking from guest_enter/exit_irqoff
>  * provide separate vtime accounting functions for consistent
>  * place the virt time specific helpers at the proper splot 
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> 
> Wanpeng Li (3):
>   context_tracking: Split guest_enter/exit_irqoff
>   context_tracking: Provide separate vtime accounting functions
>   x86/kvm: Fix vtime accounting
> 
>  arch/x86/kvm/svm/svm.c           |  6 ++-
>  arch/x86/kvm/vmx/vmx.c           |  6 ++-
>  arch/x86/kvm/x86.c               |  1 +
>  include/linux/context_tracking.h | 84 +++++++++++++++++++++++++++++++---------
>  4 files changed, 74 insertions(+), 23 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time

[Wanpeng Li]

On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > reported that the guest time remains 0 when running a while true
> > loop in the guest.
> >
> > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > tick-based time accouting when the ticks that happen after IRQs are
> > disabled are incorrectly accounted to the host/system time. This is
> > because we exit the guest state too early.
> >
> > This patchset splits both context tracking logic and the time accounting
> > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > actual vmentry/exit code, have the virt time specific helpers which
> > can be placed at the proper spots in kvm. In addition, it will not
> > break the world outside of x86.
>
> IMO, this is going in the wrong direction.  Rather than separate context tracking,
> vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> logic baked into it.
>
> Rather than smush everything into context_tracking.h, I think we can cleanly
> split the context tracking and vtime accounting code into separate pieces, which
> will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> done, splitting the context tracking and time accounting logic for KVM x86
> becomes a KVM detail as opposed to requiring dedicated logic in the context
> tracking code.
>
> I have untested code that compiles on x86, I'll send an RFC shortly.

We need an easy to backport fix and then we might have some further
cleanups on top.

    Wanpeng

Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time

[Sean Christopherson]

On Wed, Apr 14, 2021, Wanpeng Li wrote:
> On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > > reported that the guest time remains 0 when running a while true
> > > loop in the guest.
> > >
> > > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > > tick-based time accouting when the ticks that happen after IRQs are
> > > disabled are incorrectly accounted to the host/system time. This is
> > > because we exit the guest state too early.
> > >
> > > This patchset splits both context tracking logic and the time accounting
> > > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > > actual vmentry/exit code, have the virt time specific helpers which
> > > can be placed at the proper spots in kvm. In addition, it will not
> > > break the world outside of x86.
> >
> > IMO, this is going in the wrong direction.  Rather than separate context tracking,
> > vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> > context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> > logic baked into it.
> >
> > Rather than smush everything into context_tracking.h, I think we can cleanly
> > split the context tracking and vtime accounting code into separate pieces, which
> > will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> > done, splitting the context tracking and time accounting logic for KVM x86
> > becomes a KVM detail as opposed to requiring dedicated logic in the context
> > tracking code.
> >
> > I have untested code that compiles on x86, I'll send an RFC shortly.
> 
> We need an easy to backport fix and then we might have some further
> cleanups on top.

I fiddled with this a bit today, I think I have something workable that will be
a relatively clean and short backport.  With luck, I'll get it posted tomorrow.

Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time

[Wanpeng Li]

On Thu, 15 Apr 2021 at 08:49, Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Apr 14, 2021, Wanpeng Li wrote:
> > On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > > > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > > > reported that the guest time remains 0 when running a while true
> > > > loop in the guest.
> > > >
> > > > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > > > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > > > tick-based time accouting when the ticks that happen after IRQs are
> > > > disabled are incorrectly accounted to the host/system time. This is
> > > > because we exit the guest state too early.
> > > >
> > > > This patchset splits both context tracking logic and the time accounting
> > > > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > > > actual vmentry/exit code, have the virt time specific helpers which
> > > > can be placed at the proper spots in kvm. In addition, it will not
> > > > break the world outside of x86.
> > >
> > > IMO, this is going in the wrong direction.  Rather than separate context tracking,
> > > vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> > > context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> > > logic baked into it.
> > >
> > > Rather than smush everything into context_tracking.h, I think we can cleanly
> > > split the context tracking and vtime accounting code into separate pieces, which
> > > will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> > > done, splitting the context tracking and time accounting logic for KVM x86
> > > becomes a KVM detail as opposed to requiring dedicated logic in the context
> > > tracking code.
> > >
> > > I have untested code that compiles on x86, I'll send an RFC shortly.
> >
> > We need an easy to backport fix and then we might have some further
> > cleanups on top.
>
> I fiddled with this a bit today, I think I have something workable that will be
> a relatively clean and short backport.  With luck, I'll get it posted tomorrow.

I think we should improve my posted version instead of posting a lot
of alternative versions to save everybody's time.

    Wanpeng

Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time

[Sean Christopherson]

On Thu, Apr 15, 2021, Wanpeng Li wrote:
> On Thu, 15 Apr 2021 at 08:49, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Apr 14, 2021, Wanpeng Li wrote:
> > > On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > > > > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > > > > reported that the guest time remains 0 when running a while true
> > > > > loop in the guest.
> > > > >
> > > > > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > > > > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > > > > tick-based time accouting when the ticks that happen after IRQs are
> > > > > disabled are incorrectly accounted to the host/system time. This is
> > > > > because we exit the guest state too early.
> > > > >
> > > > > This patchset splits both context tracking logic and the time accounting
> > > > > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > > > > actual vmentry/exit code, have the virt time specific helpers which
> > > > > can be placed at the proper spots in kvm. In addition, it will not
> > > > > break the world outside of x86.
> > > >
> > > > IMO, this is going in the wrong direction.  Rather than separate context tracking,
> > > > vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> > > > context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> > > > logic baked into it.
> > > >
> > > > Rather than smush everything into context_tracking.h, I think we can cleanly
> > > > split the context tracking and vtime accounting code into separate pieces, which
> > > > will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> > > > done, splitting the context tracking and time accounting logic for KVM x86
> > > > becomes a KVM detail as opposed to requiring dedicated logic in the context
> > > > tracking code.
> > > >
> > > > I have untested code that compiles on x86, I'll send an RFC shortly.
> > >
> > > We need an easy to backport fix and then we might have some further
> > > cleanups on top.
> >
> > I fiddled with this a bit today, I think I have something workable that will be
> > a relatively clean and short backport.  With luck, I'll get it posted tomorrow.
> 
> I think we should improve my posted version instead of posting a lot
> of alternative versions to save everybody's time.

Ya, definitely not looking to throw out more variants.  I'm trying to stack my
cleanups on your code, while also stripping down your patches to the bare minimum
to minimize both the backports and the churn across the cleanups.  It looks like
it's going to work?  Fingers crossed :-)