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

[Julien Grall]

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 
---
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 



Cheers,

--
Julien Grall

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

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

[Stefano Stabellini]

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 
> > ---
> > 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 

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

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

[Julien Grall]

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 
---
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

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

[Julien Grall]

Hi Wei,

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.


This paragraph is a bit difficult to understand. I would reword to: 
"While the default option will be diverse today, this may change in the 
future. So we introduce " and then paste the paragraph below.




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 


With the change suggested above:

Acked-by: Julien Grall 

Cheers,

--
Julien Grall

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

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

[Wei Chen]

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 
---
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);
+
+return 0;
+}
+__initcall(update_serrors_cpu_caps);
+
 void init_traps(void)
 {
 /* Setup Hyp vector base */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index c0a25ae..ec3f9e5 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,8 +40,9 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3
 #define LIVEPATCH_FEATURE   4
+#define SKIP_CHECK_PENDING_VSERROR 5
 
-#define ARM_NCAPS   5
+#define ARM_NCAPS   6
 
 #ifndef __ASSEMBLY__
 
-- 
2.7.4


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