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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 03/30/2017 10:28 PM, Stefano Stabellini wrote:
> 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.
Fair enough :). Wei, please ignore my request.

> For this patch:
>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cheers,

--
Julien Grall

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