Re: [Xen-devel] [PATCH RESEND v4] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 22 Dec 2015, Russell King - ARM Linux wrote:
> On Tue, Dec 22, 2015 at 02:17:17PM +0000, Stefano Stabellini wrote:
> > Hello Russell,
> > 
> > Would you please consider this patch for the next merge window? It
> > is a very old patch (March 2014) which has been left over.
> 
> This patch has some obvious problems...
> 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 34e1569..e09ed94 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1807,8 +1807,7 @@ config XEN_DOM0
> >  config XEN
> >     bool "Xen guest support on ARM"
> >     depends on ARM && AEABI && OF
> > -   depends on CPU_V7 && !CPU_V6
> > -   depends on !GENERIC_ATOMIC64
> > +   depends on CPU_V7
> 
> How sure are we that this won't cause regressions?  How much testing
> has been done with these changed dependencies?

I did build-test a range of combinations of those options, sorry for not
spotting the error below.


> > diff --git a/arch/arm/include/asm/sync_bitops.h 
> > b/arch/arm/include/asm/sync_bitops.h
> > index 9732b8e..4103319 100644
> > --- a/arch/arm/include/asm/sync_bitops.h
> > +++ b/arch/arm/include/asm/sync_bitops.h
> > @@ -20,7 +20,29 @@
> >  #define sync_test_and_clear_bit(nr, p)     _test_and_clear_bit(nr, p)
> >  #define sync_test_and_change_bit(nr, p)    _test_and_change_bit(nr, p)
> >  #define sync_test_bit(nr, addr)            test_bit(nr, addr)
> > -#define sync_cmpxchg                       cmpxchg
> >  
> > +static inline unsigned long sync_cmpxchg(volatile void *ptr,
> > +                                                                           
> >  unsigned long old,
> > +                                                                           
> >  unsigned long new)
> > +{
> > +   unsigned long oldval;
> > +   int size = sizeof(*(ptr));
> 
> This is buggy.  You're doing sizeof(void) here, which on GCC will always
> be 1:
> 
>    A consequence of this is that `sizeof' is also allowed on `void' and
>   on function types, and returns 1.

You are right, thank you very much for looking at the patch and finding
this bug. I think the issue can be solved with something like:

+#define sync_cmpxchg(ptr, o, n) ({                                     \
+       (__typeof(*ptr))__sync_cmpxchg((ptr),                           \
+                                       (unsigned long)(o),             \
+                                       (unsigned long)(n),             \
+                                       sizeof(*(ptr)));                \
+})
 
+static inline unsigned long __sync_cmpxchg(volatile void *ptr,
+                                                                               
 unsigned long old,
+                                                                               
 unsigned long new,
+                                                                               
 int size)
+{


> > +
> > +   smp_mb();
> > +   switch (size) {
> > +   case 1:
> > +           oldval = __cmpxchg8(ptr, old, new);
> > +           break;
> > +   case 2:
> > +           oldval = __cmpxchg16(ptr, old, new);
> > +           break;
> 
> The ldrexb/ldrexh instructions are not available on ARMv6 CPUs.  __xchg()
> and friends avoided these for ARMv6 CPUs, but this does not.
> 
> I'd expect anything that uses sync_cmpxchg() will fail to build when a
> kernel including ARMv6 is attempted.

The code builds, but of course it could not actually run on CPU_V6. But
the use case for me is building drivers/xen/grant-table.c, which can
only run on CPU_V7 anyway, so it is not a problem.

Is that acceptable for you? If not, do you have any other suggestions?


> So... I don't think this patch is ready.

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