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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2017/3/22 20:29, Julien Grall wrote:
> 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@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.
>
> 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.
>

Ah, that would be a good option, but should we take too much resources
of cpu_hwcaps for SErrors?

> Cheers,
>


-- 
Regards,
Wei Chen

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