Re: [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

From: Julien Grall <julien.grall@arm.com>
To: Wei Chen <Wei.Chen@arm.com>, xen-devel@lists.xen.org
Cc: Kaly.Xin@arm.com, nd@arm.com, sstabellini@kernel.org,
	steve.capper@arm.com
Subject: Re: [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op
Date: Thu, 30 Mar 2017 19:02:32 +0100	[thread overview]
Message-ID: <3da9a479-43d4-6d82-f481-d5d7de15dc4f@arm.com> (raw)
In-Reply-To: <1490865209-18283-10-git-send-email-Wei.Chen@arm.com>

On 30/03/17 10:13, Wei Chen wrote:
> In the later patches of this series, we want to use the alternative
> patching framework to avoid checking serror_op in every entries.
> So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
> serror_op. When serror_op is not equal to SERROR_DIVERSE, this
> feature will be set to cpu_hwcaps.
>
> Currently, the default serror_op is SERROR_DIVERSE, if we want to
> change the serror_op value we have to place the serror parameter
> in command line. It seems no problem to update cpu_hwcaps directly
> in the serror parameter parsing function.
>
> But one day, if we change the default serror_op to SERROR_PANIC or
> SERROR_FORWARD by some security policy. We may not place the serror
> parameter in command line. In this case, if we rely on the serror
> parameter parsing function to update cpu_hwcaps, this function would
> not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
> set in cpu_hwcaps.
>
> So, we introduce this initcall to guarantee the cpu_hwcaps can be
> updated no matter the serror parameter is placed in the command line
> or not.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
> v1->v2:
> 1. Explain this initcall is to future-proof the code in commit
>    message.
> 2. Fix a coding style of this initcall.
> ---
>  xen/arch/arm/traps.c             | 9 +++++++++
>  xen/include/asm-arm/cpufeature.h | 3 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 955d97c..dafb730 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -120,6 +120,15 @@ static void __init parse_serrors_behavior(const char *str)
>  }
>  custom_param("serrors", parse_serrors_behavior);
>
> +static int __init update_serrors_cpu_caps(void)
> +{
> +    if ( serrors_op != SERRORS_DIVERSE )
> +        cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);

Thinking a bit more of this. I am wondering if we should add a warning 
(see warning_add) if the user is selecting an option other than diverse. 
Two reasons for that:
	1) The user is fully aware that he is not classifying the SError at his 
own risks
	2) If someone send an e-mail saying: "My guest crashed the hypervisor 
with an SError". We can directly know from the log.

Any opinions?

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel