Re: [Xen-devel] [PATCH for-4.5 v6 17/17] tools/tests: Enable xen-access on ARM

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-09-22 at 20:48 +0200, Tamas K Lengyel wrote:
> 
> 
> On Thu, Sep 18, 2014 at 9:02 PM, Ian Campbell
> <ian.campbell@xxxxxxxxxx> wrote:
>         On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrote:
>         > +static inline int test_and_set_bit(int nr, volatile void
>         *addr)
>         > +{
>         > +        unsigned int mask = BIT_MASK(nr);
>         > +        volatile unsigned int *p =
>         > +                ((volatile unsigned int *)addr) +
>         BIT_WORD(nr);
>         > +        unsigned int old = *p;
>         > +
>         > +        *p = old | mask;
>         > +        return (old & mask) != 0;
>         > +}
>         
>         This doesn't need to be / is not intended to be atomic, right?
> 
> 
> This function I took from xen/include/asm-arm/bitops.h directly. Since
> the x86 assembly is missing "lock;" I assume this doesn't need to be
> atomic but I may be wrong.

Looks like you have taken __test_and_set_bit (the non-atomic version)
and renamed it. Plain test_and_set_bit is supposed to be atomic I think.

>  However, in case the x86 assembly should actually have "lock;"
> similarly to how it is in xen/include/asm-x86/bitops.h, then the ARM
> side should be atomic too AFAIU.

I don't know how xen-access.c uses them so I don't know if they are
supposed to be atomic or not, but the comment above the x86 version says
they are and then defines one which is not... No idea if it is the
comment or the code which is wrong though.

It seems this is used to define some sort of lock spinlock
implementation. The code isn't multithreaded so I don't know why a lock
is needed (or if it is why a normal userspace pthread lock isn't good
enough). I hope this isn't sharing a lock with the hypervisor or
something! Maybe all that locking can just be ditched, or converted to a
standard lock.

CCing Aravindh who's been touching this most recently. 
>  Do we have an atomic test_and_set_bit implementation for ARM laying
> around somewhere?

xen/arch/arm/arm32/lib has testsetbit.S which uses bitops.S. It's a bit
non-trivial. IIRC gcc these days has some intrinsics which might be
usable. As above I hope it won't be needed though...



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