Re: [Xen-devel] [PATCH 16/18] xen/arm: Isolate the SError between the context switch of 2 vCPUs

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Stefano,

On 2017/3/21 5:46, Stefano Stabellini wrote:
> On Mon, 13 Mar 2017, Wei Chen wrote:
>> If there is a pending SError while we are doing context switch, if the
>> SError handle option is "FORWARD", We have to guranatee this serror to
>> be caught by current vCPU, otherwise it will be caught by next vCPU and
>> be forwarded to this wrong vCPU.
>>
>> We don't want to export serror_op accessing to other source files and
>> use serror_op every place, so we add a helper to synchronize SError for
>> context switching. The synchronize_serror has been used in this helper,
>> so the "#if 0" can be removed.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
>> ---
>>  xen/arch/arm/domain.c           |  2 ++
>>  xen/arch/arm/traps.c            | 14 ++++++++++++--
>>  xen/include/asm-arm/processor.h |  2 ++
>>  3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 69c2854..a547fcd 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -312,6 +312,8 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>
>>      local_irq_disable();
>>
>> +    prevent_forward_serror_to_next_vcpu();
>> +
>>      set_current(next);
>>
>>      prev = __context_switch(prev, next);
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index ee7865b..b8c8389 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2899,7 +2899,6 @@ asmlinkage void do_trap_hypervisor(struct 
>> cpu_user_regs *regs)
>>      }
>>  }
>>
>> -#if 0
>>  static void synchronize_serror(void)
>>  {
>>      /* Synchronize against in-flight ld/st. */
>> @@ -2908,7 +2907,18 @@ static void synchronize_serror(void)
>>      /* A single instruction exception window */
>>      isb();
>>  }
>> -#endif
>> +
>> +/*
>> + * If the SErrors option is "FORWARD", we have to prevent forwarding
>> + * serror to wrong vCPU. So before context switch, we have to use the
>> + * synchronize_serror to guarantee that the pending serror would be
>> + * caught by current vCPU.
>> + */
>> +void prevent_forward_serror_to_next_vcpu(void)
>> +{
>> +    if ( serrors_op == SERRORS_FORWARD )
>> +        synchronize_serror();
>> +}
>
> I dislike introducing so many 2 lines functions. I would get rid of
> prevent_forward_serror_to_next_vcpu and just add
>
>    if ( serrors_op == SERRORS_FORWARD )
>        synchronize_serror();
>
> to context_switch, and I would make synchronize_serror a static inline.
>

I had done it before, but that made the serrors_op appear everywhere.
If export serrors_op to other files is acceptable, I would re-do it.

>
>>  asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
>>  {
>> diff --git a/xen/include/asm-arm/processor.h 
>> b/xen/include/asm-arm/processor.h
>> index afad78c..3b43234 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -711,6 +711,8 @@ void do_trap_hyp_serror(struct cpu_user_regs *regs);
>>
>>  void do_trap_guest_serror(struct cpu_user_regs *regs);
>>
>> +void prevent_forward_serror_to_next_vcpu(void);
>> +
>>  /* Functions for pending virtual abort checking window. */
>>  void abort_guest_exit_start(void);
>>  void abort_guest_exit_end(void);
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel
>>
>


-- 
Regards,
Wei Chen

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