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

[Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx>]

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?

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

> +
> +     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.

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

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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