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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 30 Mar 2017, Julien Grall wrote:
> 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@xxxxxxx>
> > ---
> > 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?

I would not do that: warnings are good to warn the user about something
unexpected or potentially unknown. In this case the user has to look up
the docs to find about the other options, where it's clearly written
what they are for. We have to expect taht they know what their are
doing.

For this patch:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

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