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

[Julien Grall <julien.grall@arm.com>]

Hi Wei,

On 22/03/17 08:53, Wei Chen wrote:
> 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@arm.com>
>>> ---
>>>  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.

Why don't you introduce a new flag in cpu_hwcaps? This would allow us to 
take advantage of alternative framework in the future and avoid a branch.

Cheers,

-- 
Julien Grall

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