linux-kernel - Re: [patch 3/3] x86: kvm guest side support for KVM_HC_RT

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]

Message-ID: <20170921133653.GO26248@char.us.oracle.com>
Date:   Thu, 21 Sep 2017 09:36:53 -0400
From:   Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:     Marcelo Tosatti <mtosatti@...hat.com>, peterz@...radead.org,
        mingo@...hat.com
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 3/3] x86: kvm guest side support for KVM_HC_RT_PRIO
 hypercall

On Thu, Sep 21, 2017 at 08:38:38AM -0300, Marcelo Tosatti wrote:
> Add hypercalls to spinlock/unlock to set/unset FIFO priority
> for the vcpu, protected by a static branch to avoid performance
> increase in the normal kernels.
> 
> Enable option by "kvmfifohc" kernel command line parameter (disabled
> by default).

Wouldn't be better if there was a global 'kvm=' which could have the
various overrides?

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@...hat.com>
> 
> ---
>  arch/x86/kernel/kvm.c            |   31 +++++++++++++++++++++++++++++++
>  include/linux/spinlock.h         |    2 +-
>  include/linux/spinlock_api_smp.h |   17 +++++++++++++++++

Hm. It looks like you forgot to CC the maintainers:

$ scripts/get_maintainer.pl -f include/linux/spinlock.h
Peter Zijlstra <peterz@...radead.org> (maintainer:LOCKING PRIMITIVES)
Ingo Molnar <mingo@...hat.com> (maintainer:LOCKING PRIMITIVES)
linux-kernel@...r.kernel.org (open list:LOCKING PRIMITIVES)

Doing that for you.

>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> Index: kvm.fifopriohc-submit/arch/x86/kernel/kvm.c
> ===================================================================
> --- kvm.fifopriohc-submit.orig/arch/x86/kernel/kvm.c
> +++ kvm.fifopriohc-submit/arch/x86/kernel/kvm.c
> @@ -37,6 +37,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/nmi.h>
>  #include <linux/swait.h>
> +#include <linux/static_key.h>
>  #include <asm/timer.h>
>  #include <asm/cpu.h>
>  #include <asm/traps.h>
> @@ -321,6 +322,36 @@ static notrace void kvm_guest_apic_eoi_w
>  	apic->native_eoi_write(APIC_EOI, APIC_EOI_ACK);
>  }
>  
> +static int kvmfifohc;
> +
> +static int parse_kvmfifohc(char *arg)
> +{
> +	kvmfifohc = 1;
> +	return 0;
> +}
> +
> +early_param("kvmfifohc", parse_kvmfifohc);
> +
> +DEFINE_STATIC_KEY_FALSE(kvm_fifo_hc_key);
> +
> +static void kvm_init_fifo_hc(void)
> +{
> +	long ret;
> +
> +	ret = kvm_hypercall1(KVM_HC_RT_PRIO, 0);
> +
> +	if (ret == 0 && kvmfifohc == 1)
> +		static_branch_enable(&kvm_fifo_hc_key);
> +}
> +
> +static __init int kvmguest_late_init(void)
> +{
> +	kvm_init_fifo_hc();
> +	return 0;
> +}
> +
> +late_initcall(kvmguest_late_init);
> +
>  static void kvm_guest_cpu_init(void)
>  {
>  	if (!kvm_para_available())
> Index: kvm.fifopriohc-submit/include/linux/spinlock_api_smp.h
> ===================================================================
> --- kvm.fifopriohc-submit.orig/include/linux/spinlock_api_smp.h
> +++ kvm.fifopriohc-submit/include/linux/spinlock_api_smp.h
> @@ -136,11 +136,28 @@ static inline void __raw_spin_lock_bh(ra
>  	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
>  }
>  
> +#ifdef CONFIG_KVM_GUEST
> +DECLARE_STATIC_KEY_FALSE(kvm_fifo_hc_key);
> +#endif
> +
>  static inline void __raw_spin_lock(raw_spinlock_t *lock)
>  {
>  	preempt_disable();
> +
> +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_SMP)
> +	/* enable FIFO priority */
> +	if (static_branch_unlikely(&kvm_fifo_hc_key))
> +		kvm_hypercall1(KVM_HC_RT_PRIO, 0x1);
> +#endif

I am assuming the reason you choose not to wrap this in a pvops
or any other structure that is more of hypervisor agnostic is
that only KVM exposes this. But what if other hypervisors expose
something similar? Or some other mechanism similar to this?

> +
>  	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
>  	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> +
> +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_SMP)
> +	/* disable FIFO priority */
> +	if (static_branch_unlikely(&kvm_fifo_hc_key))
> +		kvm_hypercall1(KVM_HC_RT_PRIO, 0);
> +#endif
>  }
>  
>  #endif /* !CONFIG_GENERIC_LOCKBREAK || CONFIG_DEBUG_LOCK_ALLOC */
> Index: kvm.fifopriohc-submit/include/linux/spinlock.h
> ===================================================================
> --- kvm.fifopriohc-submit.orig/include/linux/spinlock.h
> +++ kvm.fifopriohc-submit/include/linux/spinlock.h
> @@ -56,7 +56,7 @@
>  #include <linux/stringify.h>
>  #include <linux/bottom_half.h>
>  #include <asm/barrier.h>
> -
> +#include <uapi/linux/kvm_para.h>
>  
>  /*
>   * Must define these before including other files, inline functions need them
> 
>