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

[Julien Grall <julien.grall@xxxxxxx>]

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.
Cheers,

--
Julien Grall

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