Re: [PATCH LINUX v5] xen: event channel arrays are xen_ulong_t andnot unsigned long

[Ian Campbell]

> > diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
> > index 94b4e90..5c27696 100644
> > --- a/arch/arm/include/asm/xen/events.h
> > +++ b/arch/arm/include/asm/xen/events.h
> > @@ -15,4 +15,26 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
> >  	return raw_irqs_disabled_flags(regs->ARM_cpsr);
> >  }
> >  
> > +/*
> > + * We cannot use xchg because it does not support 8-byte
> > + * values. However it is safe to use {ldr,dtd}exd directly because all
> > + * platforms which Xen can run on support those instructions.
> 
> Why does atomic64_cmpxchg not work here?

Just that we don't want/need the cmp aspect, we don't mind if an extra
bit gets set as we read the value, so long as we atomically read and set
to zero.

> > + */
> > +static inline xen_ulong_t xchg_xen_ulong(xen_ulong_t *ptr, xen_ulong_t val)
> > +{
> > +	xen_ulong_t oldval;
> > +	unsigned int tmp;
> > +
> > +	wmb();
> 
> Based on atomic64_cmpxchg implementation, you could use smp_mb here
> which avoids an outer cache flush.

Good point.

> > +	asm volatile("@ xchg_xen_ulong\n"
> > +		"1:     ldrexd  %0, %H0, [%3]\n"
> > +		"       strexd  %1, %2, %H2, [%3]\n"
> > +		"       teq     %1, #0\n"
> > +		"       bne     1b"
> > +		: "=&r" (oldval), "=&r" (tmp)
> > +		: "r" (val), "r" (ptr)
> > +		: "memory", "cc");
> 
> And a smp_mb is needed here.

I think for the specific caller which we have here it isn't strictly
necessary, but for generic correctness I think you are right.

Thanks for reviewing.

Konrad, IIRC you have already picked this up (and sent to Linus?) so an
incremental fix is required? See below.

Ian.

8<------------------------------------