[v3,9/9] KVM: Move instrumentation-safe annotations for enter/exit to x86 code
diff mbox series

Message ID 20210415222106.1643837-10-seanjc@google.com
State New
Headers show
Series
  • KVM: Fix tick-based accounting for x86 guests
Related show

Commit Message

Sean Christopherson April 15, 2021, 10:21 p.m. UTC
Drop the instrumentation_{begin,end}() annonations from the common KVM
guest enter/exit helpers, and massage the x86 code as needed to preserve
the necessary annotations.  x86 is the only architecture whose transition
flow is tagged as noinstr, and more specifically, it is the only
architecture for which instrumentation_{begin,end}() can be non-empty.

No other architecture supports CONFIG_STACK_VALIDATION=y, and s390 is the
only other architecture that support CONFIG_DEBUG_ENTRY=y.  For
instrumentation annontations to be meaningful, both aformentioned configs
must be enabled.

Letting x86 deal with the annotations avoids unnecessary nops by
squashing back-to-back instrumention-safe sequences.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.h       | 4 ++--
 include/linux/kvm_host.h | 9 +--------
 2 files changed, 3 insertions(+), 10 deletions(-)

Comments

Christian Borntraeger April 21, 2021, 8:09 a.m. UTC | #1
On 16.04.21 00:21, Sean Christopherson wrote:
> Drop the instrumentation_{begin,end}() annonations from the common KVM
> guest enter/exit helpers, and massage the x86 code as needed to preserve
> the necessary annotations.  x86 is the only architecture whose transition
> flow is tagged as noinstr, and more specifically, it is the only
> architecture for which instrumentation_{begin,end}() can be non-empty.
> 
> No other architecture supports CONFIG_STACK_VALIDATION=y, and s390 is the
> only other architecture that support CONFIG_DEBUG_ENTRY=y.  For
> instrumentation annontations to be meaningful, both aformentioned configs
> must be enabled.
> 
> Letting x86 deal with the annotations avoids unnecessary nops by
> squashing back-to-back instrumention-safe sequences.

We have considered implementing objtool for s390. Not sure where we
stand and if we will do this or not. Sven/Heiko?

So maybe drop this patch until every other arch agrees that there are
no plans to implement this.
Sven Schnelle April 22, 2021, 2:38 p.m. UTC | #2
Christian Borntraeger <borntraeger@de.ibm.com> writes:

> On 16.04.21 00:21, Sean Christopherson wrote:
>> Drop the instrumentation_{begin,end}() annonations from the common KVM
>> guest enter/exit helpers, and massage the x86 code as needed to preserve
>> the necessary annotations.  x86 is the only architecture whose transition
>> flow is tagged as noinstr, and more specifically, it is the only
>> architecture for which instrumentation_{begin,end}() can be non-empty.
>> No other architecture supports CONFIG_STACK_VALIDATION=y, and s390
>> is the
>> only other architecture that support CONFIG_DEBUG_ENTRY=y.  For
>> instrumentation annontations to be meaningful, both aformentioned configs
>> must be enabled.
>> Letting x86 deal with the annotations avoids unnecessary nops by
>> squashing back-to-back instrumention-safe sequences.
>
> We have considered implementing objtool for s390. Not sure where we
> stand and if we will do this or not. Sven/Heiko?

We are planning to support objtool on s390. Vasily is working on it -
maybe he has some thoughts about this.
Vasily Gorbik April 23, 2021, 9:32 a.m. UTC | #3
On Thu, Apr 22, 2021 at 04:38:24PM +0200, Sven Schnelle wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> writes:
> 
> > On 16.04.21 00:21, Sean Christopherson wrote:
> >> Drop the instrumentation_{begin,end}() annonations from the common KVM
> >> guest enter/exit helpers, and massage the x86 code as needed to preserve
> >> the necessary annotations.  x86 is the only architecture whose transition
> >> flow is tagged as noinstr, and more specifically, it is the only
> >> architecture for which instrumentation_{begin,end}() can be non-empty.
> >> No other architecture supports CONFIG_STACK_VALIDATION=y, and s390
> >> is the
> >> only other architecture that support CONFIG_DEBUG_ENTRY=y.  For
> >> instrumentation annontations to be meaningful, both aformentioned configs
> >> must be enabled.
> >> Letting x86 deal with the annotations avoids unnecessary nops by
> >> squashing back-to-back instrumention-safe sequences.
> >
> > We have considered implementing objtool for s390. Not sure where we
> > stand and if we will do this or not. Sven/Heiko?
> 
> We are planning to support objtool on s390. Vasily is working on it -
> maybe he has some thoughts about this.

We got CONFIG_DEBUG_ENTRY=y since 5.12, objtool runs on vmlinux.o but I have
not yet enabled --noinstr option in s390 objtool. So, it's hard to say in
advance if this particular change would make things better or worse.
In general, common code annotations are problematic, because arch
specific code is still not identical and this leads sometimes to different
needs for common code annotations.

I'll try to experiment with --noinstr on s390 shortly.

Patch
diff mbox series

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 285953e81777..b17857ac540b 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -25,9 +25,9 @@  static __always_inline void kvm_guest_enter_irqoff(void)
 	instrumentation_begin();
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
-	instrumentation_end();
-
 	guest_enter_irqoff();
+	instrumentation_end();
+
 	lockdep_hardirqs_on(CALLER_ADDR0);
 }
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 444d5f0225cb..e5eb64019f47 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -339,9 +339,7 @@  static __always_inline void guest_enter_irqoff(void)
 	 * This is running in ioctl context so its safe to assume that it's the
 	 * stime pending cputime to flush.
 	 */
-	instrumentation_begin();
 	vtime_account_guest_enter();
-	instrumentation_end();
 
 	/*
 	 * KVM does not hold any references to rcu protected data when it
@@ -351,21 +349,16 @@  static __always_inline void guest_enter_irqoff(void)
 	 * one time slice). Lets treat guest mode as quiescent state, just like
 	 * we do with user-mode execution.
 	 */
-	if (!context_tracking_guest_enter_irqoff()) {
-		instrumentation_begin();
+	if (!context_tracking_guest_enter_irqoff())
 		rcu_virt_note_context_switch(smp_processor_id());
-		instrumentation_end();
-	}
 }
 
 static __always_inline void guest_exit_irqoff(void)
 {
 	context_tracking_guest_exit_irqoff();
 
-	instrumentation_begin();
 	/* Flush the guest cputime we spent on the guest */
 	vtime_account_guest_exit();
-	instrumentation_end();
 }
 
 static inline void guest_exit(void)