linux-kernel - Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

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: <20190408143952.GF15689@zn.tnic>
Date:   Mon, 8 Apr 2019 16:39:52 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Zhao Yakui <yakui.zhao@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
        Jason Chen CJ <jason.cj.chen@...el.com>
Subject: Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

On Mon, Apr 08, 2019 at 04:12:09PM +0800, Zhao Yakui wrote:
> ACRN is an open-source hypervisor maintained by Linuxfoundation.

I think tglx wanted to say "by the Linux Foundation" here.

> This is to add the Linux guest support on acrn-hypervisor.

I think you were told already:

"Please do not use 'This is to add' or 'This adds'. Just say:

Add ...."

So please take your time, work in *all* review feedback instead of
hurrying the next version out without addressing all review comments.

> Add x86_hyper_acrn into supported hypervisors array, which enables
> Linux ACRN guest running on ACRN hypervisor. It is restricted to X86_64.

So this all talks about *what* the patch does. But that is visible from
the diff below and doesn't belong in the commit message.

Rather, your commit message should talk about *why* a change is being
done.

> Co-developed-by: Jason Chen CJ <jason.cj.chen@...el.com>
> Signed-off-by: Jason Chen CJ <jason.cj.chen@...el.com>
> Signed-off-by: Zhao Yakui <yakui.zhao@...el.com>
> ---
> v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
> understand.
>         Remove the export of x86_hyper_acrn.
> 
> v2->v3: Remove the unnecessary dependency of PARAVIRT
> ---
>  arch/x86/Kconfig                  |  8 ++++++++
>  arch/x86/include/asm/hypervisor.h |  1 +
>  arch/x86/kernel/cpu/Makefile      |  1 +
>  arch/x86/kernel/cpu/acrn.c        | 35 +++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
>  5 files changed, 49 insertions(+)
>  create mode 100644 arch/x86/kernel/cpu/acrn.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 54d1fbc..d77d215 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -845,6 +845,14 @@ config JAILHOUSE_GUEST
>  	  cell. You can leave this option disabled if you only want to start
>  	  Jailhouse and run Linux afterwards in the root cell.
>  
> +config ACRN_GUEST
> +	bool "ACRN Guest support"
> +	depends on X86_64
> +	help
> +	  This option allows to run Linux as guest in ACRN hypervisor. Enabling
> +	  this will allow the kernel to boot in virtualized environment under
> +	  the ACRN hypervisor.

WARNING: please write a paragraph that describes the config symbol fully
#47: FILE: arch/x86/Kconfig:848:
+config ACRN_GUEST

That help text above could use some of the explanation what ACRN is from
your 0/4 message.

> +
>  endif #HYPERVISOR_GUEST
>  
>  source "arch/x86/Kconfig.cpu"
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index 8c5aaba..50a30f6 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -29,6 +29,7 @@ enum x86_hypervisor_type {
>  	X86_HYPER_XEN_HVM,
>  	X86_HYPER_KVM,
>  	X86_HYPER_JAILHOUSE,
> +	X86_HYPER_ACRN,
>  };
>  
>  #ifdef CONFIG_HYPERVISOR_GUEST
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index cfd24f9..17a7cdf 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_X86_CPU_RESCTRL)		+= resctrl/
>  obj-$(CONFIG_X86_LOCAL_APIC)		+= perfctr-watchdog.o
>  
>  obj-$(CONFIG_HYPERVISOR_GUEST)		+= vmware.o hypervisor.o mshyperv.o
> +obj-$(CONFIG_ACRN_GUEST)		+= acrn.o
>  
>  ifdef CONFIG_X86_FEATURE_NAMES
>  quiet_cmd_mkcapflags = MKCAP   $@
> diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
> new file mode 100644
> index 0000000..3956567
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/acrn.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACRN detection support
> + *
> + * Copyright (C) 2019 Intel Corporation. All rights reserved.
> + *
> + * Jason Chen CJ <jason.cj.chen@...el.com>
> + * Zhao Yakui <yakui.zhao@...el.com>
> + *
> + */
> +
> +#include <asm/hypervisor.h>
> +
> +static uint32_t __init acrn_detect(void)
> +{
> +	return hypervisor_cpuid_base("ACRNACRNACRN\0\0", 0);
> +}
> +
> +static void __init acrn_init_platform(void)
> +{
> +}
> +
> +static bool acrn_x2apic_available(void)
> +{
> +	/* do not support x2apic */

Why?

This comment could explain why that choice has been made.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.