Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     "Kirill A. Shutemov" <kirill@shutemov.name>
Cc:     David Rientjes <rientjes@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Kees Cook <keescook@chromium.org>,
        Will Drewry <wad@chromium.org>,
        "Edgecombe\, Rick P" <rick.p.edgecombe@intel.com>,
        "Kleen\, Andi" <andi.kleen@intel.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>
Subject: Re: [RFC 02/16] x86/kvm: Introduce KVM memory protection feature
In-Reply-To: <20200522125214.31348-3-kirill.shutemov@linux.intel.com>
References: <20200522125214.31348-1-kirill.shutemov@linux.intel.com> <20200522125214.31348-3-kirill.shutemov@linux.intel.com>
Date:   Mon, 25 May 2020 16:58:51 +0200
Message-ID: <87d06s83is.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> Provide basic helpers, KVM_FEATURE and a hypercall.
>
> Host side doesn't provide the feature yet, so it is a dead code for now.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/include/asm/kvm_para.h      |  5 +++++
>  arch/x86/include/uapi/asm/kvm_para.h |  3 ++-
>  arch/x86/kernel/kvm.c                | 16 ++++++++++++++++
>  include/uapi/linux/kvm_para.h        |  3 ++-
>  4 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index 9b4df6eaa11a..3ce84fc07144 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -10,11 +10,16 @@ extern void kvmclock_init(void);
>  
>  #ifdef CONFIG_KVM_GUEST
>  bool kvm_check_and_clear_guest_paused(void);
> +bool kvm_mem_protected(void);
>  #else
>  static inline bool kvm_check_and_clear_guest_paused(void)
>  {
>  	return false;
>  }
> +static inline bool kvm_mem_protected(void)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_KVM_GUEST */
>  
>  #define KVM_HYPERCALL \
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> index 2a8e0b6b9805..c3b499acc98f 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -28,9 +28,10 @@
>  #define KVM_FEATURE_PV_UNHALT		7
>  #define KVM_FEATURE_PV_TLB_FLUSH	9
>  #define KVM_FEATURE_ASYNC_PF_VMEXIT	10
> -#define KVM_FEATURE_PV_SEND_IPI	11
> +#define KVM_FEATURE_PV_SEND_IPI		11

Nit: spurrious change

>  #define KVM_FEATURE_POLL_CONTROL	12
>  #define KVM_FEATURE_PV_SCHED_YIELD	13
> +#define KVM_FEATURE_MEM_PROTECTED	14
>  
>  #define KVM_HINTS_REALTIME      0
>  
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 6efe0410fb72..bda761ca0d26 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -35,6 +35,13 @@
>  #include <asm/tlb.h>
>  #include <asm/cpuidle_haltpoll.h>
>  
> +static bool mem_protected;
> +
> +bool kvm_mem_protected(void)
> +{
> +	return mem_protected;
> +}
> +

Honestly, I don't see a need for kvm_mem_protected(), just rename the
bool if you need kvm_ prefix :-)

>  static int kvmapf = 1;
>  
>  static int __init parse_no_kvmapf(char *arg)
> @@ -727,6 +734,15 @@ static void __init kvm_init_platform(void)
>  {
>  	kvmclock_init();
>  	x86_platform.apic_post_init = kvm_apic_init;
> +
> +	if (kvm_para_has_feature(KVM_FEATURE_MEM_PROTECTED)) {
> +		if (kvm_hypercall0(KVM_HC_ENABLE_MEM_PROTECTED)) {
> +			pr_err("Failed to enable KVM memory protection\n");
> +			return;
> +		}
> +
> +		mem_protected = true;
> +	}
>  }

Personally, I'd prefer to do this via setting a bit in a KVM-specific
MSR instead. The benefit is that the guest doesn't need to remember if
it enabled the feature or not, it can always read the config msr. May
come handy for e.g. kexec/kdump.

>  
>  const __initconst struct hypervisor_x86 x86_hyper_kvm = {
> diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
> index 8b86609849b9..1a216f32e572 100644
> --- a/include/uapi/linux/kvm_para.h
> +++ b/include/uapi/linux/kvm_para.h
> @@ -27,8 +27,9 @@
>  #define KVM_HC_MIPS_EXIT_VM		7
>  #define KVM_HC_MIPS_CONSOLE_OUTPUT	8
>  #define KVM_HC_CLOCK_PAIRING		9
> -#define KVM_HC_SEND_IPI		10
> +#define KVM_HC_SEND_IPI			10

Same spurrious change detected.

>  #define KVM_HC_SCHED_YIELD		11
> +#define KVM_HC_ENABLE_MEM_PROTECTED	12
>  
>  /*
>   * hypercalls use architecture specific

-- 
Vitaly