Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

[Ingo Molnar]

* Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:

> 
> 
> On 08/06/2016 14:16, Ingo Molnar wrote:
> > > The guest ones are not quite as consistent.  I can fix that later,
> > > there's no reason also to have guest context tracking split between
> > > include/linux/context_tracking.h and include/linux/kvm_host.h.
> >
> > Could we please first do the cleanups before complicating the code and applying 
> > more substantial changes?
> 
> The further cleanups wouldn't complicate the code.  It's just that
> guest_enter/guest_exit require IRQs off but don't have __.
> 
> I'm thinking of something like this (untested):
> 
> diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> index d259274238db..c2dc581ddb0e 100644
> --- a/include/linux/context_tracking.h
> +++ b/include/linux/context_tracking.h
> @@ -84,7 +84,7 @@ static inline void context_tracking_init(void) { }
>  
>  
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> -static inline void guest_enter(void)
> +static inline void __guest_enter(void)
>  {
>  	if (vtime_accounting_cpu_enabled())
>  		vtime_guest_enter(current);
> @@ -93,9 +93,19 @@ static inline void guest_enter(void)
>  
>  	if (context_tracking_is_enabled())
>  		__context_tracking_enter(CONTEXT_GUEST);
> +
> +	/* KVM does not hold any references to rcu protected data when it
> +	 * switches CPU into a guest mode. In fact switching to a guest mode

Nit, please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

> +	 * is very similar to exiting to userspace from rcu point of view. In

s/RCU

> +	 * addition CPU may stay in a guest mode for quite a long time (up to
> +	 * one time slice). Lets treat guest mode as quiescent state, just like
> +	 * we do with user-mode execution.
> +	 */
> +	if (!context_tracking_cpu_is_enabled())
> +		rcu_virt_note_context_switch(smp_processor_id());
>  }
>  
> -static inline void guest_exit(void)
> +static inline void __guest_exit(void)

> +static inline void guest_enter(void)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	__guest_enter();
> +	local_irq_restore(flags);

So I believe it would be cleaner to name the irqs-off code paths explicitly: 
__guest_enter_irqsoff(), and propagate that naming into other parts as well?

>  /* must be called with irqs disabled */
>  static inline void __kvm_guest_exit(void)

This way all these random comments about irqs-off requirements would become 
unnecessary - the code becomes self-documenting.

Thanks,

	Ingo