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

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, 8 Jun 2016 14:54:06 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org.com>,
	"H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
	Rik van Riel <riel@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore


* Paolo Bonzini <pbonzini@...hat.com> 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