Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-31 Thread Wei Chen 
On 2017/3/31 19:06, Julien Grall wrote:
>
>
> On 31/03/17 11:55, Wei Chen wrote:
>> Hi Julien,
>
> Hi Wei,
>
>> On 2017/3/31 2:38, Julien Grall wrote:
>>>
>>>
>>> On 30/03/17 19:32, Julien Grall wrote:
 On 30/03/17 19:28, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:13, Wei Chen wrote:
>> +void synchronize_serror(void)
>
> Sorry for been late in the party. Looking at the way you use the
> function, you execute depending on the behavior chosen by the user when
> an SErrors happen. This behavior will not change at runtime, so always
> checking the option chosen in the hot path does not sound very efficient.
>
> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
> I.e
>
> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
> the place.

 To complete what I was suggestion, you could define:

 /* Synchronize SError unless the feature is selected */
 #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>>>
>>> Or even:
>>>
>>> /*
>>>   * Synchronize SError unless the feature is selected.
>>>   * This is relying on the SErrors are currently unmasked.
>>>   */
>>> #define SYNCHRONIZE_SERROR(feat)\
>>>do {  \
>>>  ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
>>>  ALTERNATIVE("dsb sy; isb", "nop; nop");  \
>>>while (0)
>>>
>>> The ASSERT is here to check that we have abort enabled. Otherwise, doing
>>> the synchronization would be pointless.
>>>
>>
>> Think a bit more about it. This macro will import more check than read
>> serror_op. This macro seems as a generic macro, it can be used in any
>> place. So you enable the feature check and abort check. But in this
>> patch, we know where the macro will be used, we know the abort is
>> enabled. And want to reduce the overhead as much as possible.
>
> ASSERTs are used to verify your assumption is correct, for non-debug
> built they will be turned into a NOP.
>
> I care about overhead in non-debug build, it is not much a concern for
> debug build. In the latter it is more important to verify the correctness.
>
> The abort bit is enabled at the entry in the hypervisor, and neither you
> nor I can tell if we need to disable abort in the future in some path.
> This ASSERT will catch those changes.
>
> So please give me a reason to remove it, because I see only benefits so far.

Ok, thanks, I understand know. As it just affects the debug built. I
will keep them in the macro.

>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-31 Thread Julien Grall 



On 31/03/17 11:55, Wei Chen wrote:

Hi Julien,


Hi Wei,


On 2017/3/31 2:38, Julien Grall wrote:



On 30/03/17 19:32, Julien Grall wrote:

On 30/03/17 19:28, Julien Grall wrote:

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:

+void synchronize_serror(void)


Sorry for been late in the party. Looking at the way you use the
function, you execute depending on the behavior chosen by the user when
an SErrors happen. This behavior will not change at runtime, so always
checking the option chosen in the hot path does not sound very efficient.

I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
I.e

ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
the place.


To complete what I was suggestion, you could define:

/* Synchronize SError unless the feature is selected */
#define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");


Or even:

/*
  * Synchronize SError unless the feature is selected.
  * This is relying on the SErrors are currently unmasked.
  */
#define SYNCHRONIZE_SERROR(feat)\
   do {  \
 ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
 ALTERNATIVE("dsb sy; isb", "nop; nop");  \
   while (0)

The ASSERT is here to check that we have abort enabled. Otherwise, doing
the synchronization would be pointless.



Think a bit more about it. This macro will import more check than read
serror_op. This macro seems as a generic macro, it can be used in any
place. So you enable the feature check and abort check. But in this
patch, we know where the macro will be used, we know the abort is
enabled. And want to reduce the overhead as much as possible.


ASSERTs are used to verify your assumption is correct, for non-debug 
built they will be turned into a NOP.


I care about overhead in non-debug build, it is not much a concern for 
debug build. In the latter it is more important to verify the correctness.


The abort bit is enabled at the entry in the hypervisor, and neither you 
nor I can tell if we need to disable abort in the future in some path. 
This ASSERT will catch those changes.


So please give me a reason to remove it, because I see only benefits so far.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-31 Thread Wei Chen 
Hi Julien,

On 2017/3/31 2:38, Julien Grall wrote:
>
>
> On 30/03/17 19:32, Julien Grall wrote:
>> On 30/03/17 19:28, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
 +void synchronize_serror(void)
>>>
>>> Sorry for been late in the party. Looking at the way you use the
>>> function, you execute depending on the behavior chosen by the user when
>>> an SErrors happen. This behavior will not change at runtime, so always
>>> checking the option chosen in the hot path does not sound very efficient.
>>>
>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>> I.e
>>>
>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>> the place.
>>
>> To complete what I was suggestion, you could define:
>>
>> /* Synchronize SError unless the feature is selected */
>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>
> Or even:
>
> /*
>   * Synchronize SError unless the feature is selected.
>   * This is relying on the SErrors are currently unmasked.
>   */
> #define SYNCHRONIZE_SERROR(feat)\
>   do {  \
> ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
> ALTERNATIVE("dsb sy; isb", "nop; nop"); \
>   while (0)
>
> The ASSERT is here to check that we have abort enabled. Otherwise, doing
> the synchronization would be pointless.
>

Think a bit more about it. This macro will import more check than read
serror_op. This macro seems as a generic macro, it can be used in any
place. So you enable the feature check and abort check. But in this
patch, we know where the macro will be used, we know the abort is
enabled. And want to reduce the overhead as much as possible.

I prefer to define a similar macro as the following:
#define SYNCHRONIZE_SERROR(feat)\
 do {\
 ALTERNATIVE("dsb sy; isb" : : : "memory", "nop; nop", feat);\
 } while (0)

> Note that the function local_abort_is_enabled is not implemented. But it
> is easy to write it. Looking how local_irq_is_enabled was implemented.
>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-30 Thread Wei Chen 
On 2017/3/31 2:38, Julien Grall wrote:
>
>
> On 30/03/17 19:32, Julien Grall wrote:
>> On 30/03/17 19:28, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
 +void synchronize_serror(void)
>>>
>>> Sorry for been late in the party. Looking at the way you use the
>>> function, you execute depending on the behavior chosen by the user when
>>> an SErrors happen. This behavior will not change at runtime, so always
>>> checking the option chosen in the hot path does not sound very efficient.
>>>
>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>> I.e
>>>
>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>> the place.
>>
>> To complete what I was suggestion, you could define:
>>
>> /* Synchronize SError unless the feature is selected */
>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>
> Or even:
>
> /*
>   * Synchronize SError unless the feature is selected.
>   * This is relying on the SErrors are currently unmasked.
>   */
> #define SYNCHRONIZE_SERROR(feat)\
>   do {  \
> ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
> ALTERNATIVE("dsb sy; isb", "nop; nop"); \
>   while (0)
>
> The ASSERT is here to check that we have abort enabled. Otherwise, doing
> the synchronization would be pointless.
>
> Note that the function local_abort_is_enabled is not implemented. But it
> is easy to write it. Looking how local_irq_is_enabled was implemented.
>

This is a better solution than function, I would do it.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-30 Thread Julien Grall 



On 30/03/17 19:32, Julien Grall wrote:

On 30/03/17 19:28, Julien Grall wrote:

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:

+void synchronize_serror(void)


Sorry for been late in the party. Looking at the way you use the
function, you execute depending on the behavior chosen by the user when
an SErrors happen. This behavior will not change at runtime, so always
checking the option chosen in the hot path does not sound very efficient.

I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
I.e

ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
the place.


To complete what I was suggestion, you could define:

/* Synchronize SError unless the feature is selected */
#define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");


Or even:

/*
 * Synchronize SError unless the feature is selected.
 * This is relying on the SErrors are currently unmasked.
 */
#define SYNCHRONIZE_SERROR(feat)  \
do {  \
  ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
  ALTERNATIVE("dsb sy; isb", "nop; nop"); \
while (0)

The ASSERT is here to check that we have abort enabled. Otherwise, doing 
the synchronization would be pointless.


Note that the function local_abort_is_enabled is not implemented. But it 
is easy to write it. Looking how local_irq_is_enabled was implemented.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-30 Thread Julien Grall 

On 30/03/17 19:28, Julien Grall wrote:

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:

+void synchronize_serror(void)


Sorry for been late in the party. Looking at the way you use the
function, you execute depending on the behavior chosen by the user when
an SErrors happen. This behavior will not change at runtime, so always
checking the option chosen in the hot path does not sound very efficient.

I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb). I.e

ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
the place.


To complete what I was suggestion, you could define:

/* Synchronize SError unless the feature is selected */
#define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-30 Thread Julien Grall 

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:

+void synchronize_serror(void)


Sorry for been late in the party. Looking at the way you use the 
function, you execute depending on the behavior chosen by the user when 
an SErrors happen. This behavior will not change at runtime, so always 
checking the option chosen in the hot path does not sound very efficient.


I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb). I.e

ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of 
the place.



+{
+/* Synchronize against in-flight ld/st. */
+dsb(sy);
+
+/* A single instruction exception window */
+isb();
+}
+


Cheers,

--
Julien Grall

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

[Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

2017-03-30 Thread Wei Chen 
In previous patches, we have provided the ability to synchronize
SErrors in exception entries. But we haven't synchronized SErrors
while returning to guest and doing context switch.

So we still have two risks:
1. Slipping hypervisor SErrors to guest. For example, hypervisor
   triggers a SError while returning to guest, but this SError may be
   delivered after entering guest. In "DIVERSE" option, this SError
   would be routed back to guest and panic the guest. But actually,
   we should crash the whole system due to this hypervisor SError.
2. Slipping previous guest SErrors to the next guest. In "FORWARD"
   option, if hypervisor triggers a SError while context switching.
   This SError may be delivered after switching to next vCPU. In this
   case, this SError will be forwarded to next vCPU and may panic
   an incorrect guest.

So we have have to introduce this helper to synchronize SErrors while
returning to guest and doing context switch.

This function should be used out of trap.c in later patch of this
series. We have to export this helper in header file.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
v1->v2:
1. Update commit message to explain why we introduce this helper.
2. Remove static for this helper.
3. Add Stefano's Reviewed-by tag.
---
 xen/arch/arm/traps.c| 9 +
 xen/include/asm-arm/processor.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f454fb5..55d85ed 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2881,6 +2881,15 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs 
*regs)
 }
 }
 
+void synchronize_serror(void)
+{
+/* Synchronize against in-flight ld/st. */
+dsb(sy);
+
+/* A single instruction exception window */
+isb();
+}
+
 asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
 {
 enter_hypervisor_head(regs);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index ead6ad3..3ebbe57 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -721,6 +721,8 @@ void abort_guest_exit_end(void);
 ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
 )
 
+void synchronize_serror(void);
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*
-- 
2.7.4


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