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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 Mar 2017, Wei Chen wrote:
> 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?

No worries, we have plenty.

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