[PATCH 0/7] assorted (mostly) atomics changes

[PATCH 0/7] assorted (mostly) atomics changes

[Jan Beulich]

1: events: drop arch_evtchn_inject()
2: common: avoid atomic read-modify-write accesses in map_vcpu_info()
3: Arm/atomic: correct asm() constraints in build_add_sized()
4: Arm/atomic: drop uniformly used reg macro parameters
5: Arm/atomic: parameterize register modifier macro arguments
6: Arm/atomic: unify generation of u64 read/write functions
7: Arm/atomic: cosmetics

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/7] events: drop arch_evtchn_inject()

[Jan Beulich]

Have the only user call vcpu_mark_events_pending() instead, at the same
time arranging for correct ordering of the writes (evtchn_pending_sel
should be written before evtchn_upcall_pending).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -597,11 +597,6 @@ out:
     return;
 }
 
-void arch_evtchn_inject(struct vcpu *v)
-{
-    vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true);
-}
-
 bool vgic_evtchn_irq_pending(struct vcpu *v)
 {
     struct pending_irq *p;
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -692,11 +692,6 @@ void vgic_kick_vcpus(struct domain *d)
     }
 }
 
-void arch_evtchn_inject(struct vcpu *v)
-{
-    vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true);
-}
-
 bool vgic_evtchn_irq_pending(struct vcpu *v)
 {
     struct vgic_irq *irq;
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2724,9 +2724,3 @@ int allocate_and_map_msi_pirq(struct dom
 
     return ret;
 }
-
-void arch_evtchn_inject(struct vcpu *v)
-{
-    if ( is_hvm_vcpu(v) )
-        hvm_assert_evtchn_irq(v);
-}
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1306,10 +1306,9 @@ int map_vcpu_info(struct vcpu *v, unsign
      * Mark everything as being pending just to make sure nothing gets
      * lost.  The domain will get a spurious event, but it can cope.
      */
-    vcpu_info(v, evtchn_upcall_pending) = 1;
     for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
         set_bit(i, &vcpu_info(v, evtchn_pending_sel));
-    arch_evtchn_inject(v);
+    vcpu_mark_events_pending(v);
 
     return 0;
 }
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -91,9 +91,6 @@ int guest_enabled_event(struct vcpu *v,
 /* Notify remote end of a Xen-attached event channel.*/
 void notify_via_xen_event_channel(struct domain *ld, int lport);
 
-/* Inject an event channel notification into the guest */
-void arch_evtchn_inject(struct vcpu *v);
-
 /*
  * Internal event channel object storage.
  *





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/7] common: avoid atomic read-modify-write accesses in map_vcpu_info()

[Jan Beulich]

There's no need to set the evtchn_pending_sel bits one by one. Simply
write full words with all ones.

For Arm this requires extending write_atomic() to also handle 64-bit
values; for symmetry read_atomic() gets adjusted as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1253,7 +1253,6 @@ int map_vcpu_info(struct vcpu *v, unsign
     void *mapping;
     vcpu_info_t *new_info;
     struct page_info *page;
-    int i;
 
     if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
         return -EINVAL;
@@ -1306,8 +1305,12 @@ int map_vcpu_info(struct vcpu *v, unsign
      * Mark everything as being pending just to make sure nothing gets
      * lost.  The domain will get a spurious event, but it can cope.
      */
-    for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
-        set_bit(i, &vcpu_info(v, evtchn_pending_sel));
+#ifdef CONFIG_COMPAT
+    if ( !has_32bit_shinfo(d) )
+        write_atomic(&new_info->native.evtchn_pending_sel, ~0);
+    else
+#endif
+        write_atomic(&vcpu_info(v, evtchn_pending_sel), ~0);
     vcpu_mark_events_pending(v);
 
     return 0;
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -55,6 +55,19 @@ build_atomic_write(write_int_atomic, "",
 #if defined (CONFIG_ARM_64)
 build_atomic_read(read_u64_atomic, "", "", uint64_t, "=r")
 build_atomic_write(write_u64_atomic, "", "", uint64_t, "r")
+#elif defined (CONFIG_ARM_32)
+static inline uint64_t read_u64_atomic(const volatile uint64_t *addr)
+{
+    uint64_t val;
+
+    asm volatile ( "ldrd %0,%H0,%1" : "=r" (val) : "m" (*addr) );
+
+    return val;
+}
+static inline void write_u64_atomic(volatile uint64_t *addr, uint64_t val)
+{
+    asm volatile ( "strd %1,%H1,%0" : "=m" (*addr) : "r" (val) );
+}
 #endif
 
 build_add_sized(add_u8_sized, "b", BYTE, uint8_t, "ri")
@@ -69,6 +82,7 @@ void __bad_atomic_size(void);
     case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
     case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
     case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
+    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
     default: __x = 0; __bad_atomic_size(); break;                       \
     }                                                                   \
     __x;                                                                \
@@ -80,6 +94,7 @@ void __bad_atomic_size(void);
     case 1: write_u8_atomic((uint8_t *)p, (uint8_t)__x); break;         \
     case 2: write_u16_atomic((uint16_t *)p, (uint16_t)__x); break;      \
     case 4: write_u32_atomic((uint32_t *)p, (uint32_t)__x); break;      \
+    case 8: write_u64_atomic((uint64_t *)p, (uint64_t)__x); break;      \
     default: __bad_atomic_size(); break;                                \
     }                                                                   \
     __x;                                                                \





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/7] Arm/atomic: correct asm() constraints in build_add_sized()

[Jan Beulich]

The memory operand is an in/out one, and the auxiliary register gets
written to early.

Take the opportunity and also drop the redundant cast (the inline
functions' parameters are already of the casted-to type).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -30,7 +30,7 @@ static inline void name(volatile type *a
     asm volatile("ldr" size " %"width"1,%0\n"                           \
                  "add %"width"1,%"width"1,%"width"2\n"                  \
                  "str" size " %"width"1,%0"                             \
-                 : "=m" (*(volatile type *)addr), "=r" (t)              \
+                 : "+m" (*addr), "=&r" (t)                              \
                  : reg (val));                                          \
 }
 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] events: drop arch_evtchn_inject()

[Andrew Cooper]

On 11/03/2019 16:48, Jan Beulich wrote:
> Have the only user call vcpu_mark_events_pending() instead, at the same
> time arranging for correct ordering of the writes (evtchn_pending_sel
> should be written before evtchn_upcall_pending).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/7] Arm/atomic: drop uniformly used reg macro parameters

[Jan Beulich]

There's no point in parameterizing these when all use sites pass the
same arguments.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -5,25 +5,25 @@
 #include <xen/prefetch.h>
 #include <asm/system.h>
 
-#define build_atomic_read(name, size, width, type, reg)\
+#define build_atomic_read(name, size, width, type) \
 static inline type name(const volatile type *addr) \
 {                                                  \
     type ret;                                      \
     asm volatile("ldr" size " %" width "0,%1"      \
-                 : reg (ret)                       \
+                 : "=r" (ret)                      \
                  : "m" (*(volatile type *)addr));  \
     return ret;                                    \
 }
 
-#define build_atomic_write(name, size, width, type, reg) \
+#define build_atomic_write(name, size, width, type)    \
 static inline void name(volatile type *addr, type val) \
 {                                                      \
     asm volatile("str" size " %"width"1,%0"            \
                  : "=m" (*(volatile type *)addr)       \
-                 : reg (val));                         \
+                 : "r" (val));                         \
 }
 
-#define build_add_sized(name, size, width, type, reg) \
+#define build_add_sized(name, size, width, type)                        \
 static inline void name(volatile type *addr, type val)                  \
 {                                                                       \
     type t;                                                             \
@@ -31,7 +31,7 @@ static inline void name(volatile type *a
                  "add %"width"1,%"width"1,%"width"2\n"                  \
                  "str" size " %"width"1,%0"                             \
                  : "+m" (*addr), "=&r" (t)                              \
-                 : reg (val));                                          \
+                 : "ri" (val));                                         \
 }
 
 #if defined (CONFIG_ARM_32)
@@ -42,19 +42,19 @@ static inline void name(volatile type *a
 #define WORD "w"
 #endif
 
-build_atomic_read(read_u8_atomic,  "b", BYTE, uint8_t, "=r")
-build_atomic_read(read_u16_atomic, "h", WORD, uint16_t, "=r")
-build_atomic_read(read_u32_atomic, "",  WORD, uint32_t, "=r")
-build_atomic_read(read_int_atomic, "",  WORD, int, "=r")
-
-build_atomic_write(write_u8_atomic,  "b", BYTE, uint8_t, "r")
-build_atomic_write(write_u16_atomic, "h", WORD, uint16_t, "r")
-build_atomic_write(write_u32_atomic, "",  WORD, uint32_t, "r")
-build_atomic_write(write_int_atomic, "",  WORD, int, "r")
+build_atomic_read(read_u8_atomic,  "b", BYTE, uint8_t)
+build_atomic_read(read_u16_atomic, "h", WORD, uint16_t)
+build_atomic_read(read_u32_atomic, "",  WORD, uint32_t)
+build_atomic_read(read_int_atomic, "",  WORD, int)
+
+build_atomic_write(write_u8_atomic,  "b", BYTE, uint8_t)
+build_atomic_write(write_u16_atomic, "h", WORD, uint16_t)
+build_atomic_write(write_u32_atomic, "",  WORD, uint32_t)
+build_atomic_write(write_int_atomic, "",  WORD, int)
 
 #if defined (CONFIG_ARM_64)
-build_atomic_read(read_u64_atomic, "", "", uint64_t, "=r")
-build_atomic_write(write_u64_atomic, "", "", uint64_t, "r")
+build_atomic_read(read_u64_atomic, "", "", uint64_t)
+build_atomic_write(write_u64_atomic, "", "", uint64_t)
 #elif defined (CONFIG_ARM_32)
 static inline uint64_t read_u64_atomic(const volatile uint64_t *addr)
 {
@@ -70,9 +70,9 @@ static inline void write_u64_atomic(vola
 }
 #endif
 
-build_add_sized(add_u8_sized, "b", BYTE, uint8_t, "ri")
-build_add_sized(add_u16_sized, "h", WORD, uint16_t, "ri")
-build_add_sized(add_u32_sized, "", WORD, uint32_t, "ri")
+build_add_sized(add_u8_sized, "b", BYTE, uint8_t)
+build_add_sized(add_u16_sized, "h", WORD, uint16_t)
+build_add_sized(add_u32_sized, "", WORD, uint32_t)
 
 void __bad_atomic_size(void);
 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] common: avoid atomic read-modify-write accesses in map_vcpu_info()

[Andrew Cooper]

On 11/03/2019 16:48, Jan Beulich wrote:
> There's no need to set the evtchn_pending_sel bits one by one. Simply
> write full words with all ones.
>
> For Arm this requires extending write_atomic() to also handle 64-bit
> values; for symmetry read_atomic() gets adjusted as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

(I'll leave the ARM assembly to Julien to check over)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/7] Arm/atomic: parameterize register modifier macro arguments

[Jan Beulich]

Make the abstracting macros take the asm() operand specifier as
argument, in preparation of doing away with the split u64 read/write
definitions.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -9,7 +9,7 @@
 static inline type name(const volatile type *addr) \
 {                                                  \
     type ret;                                      \
-    asm volatile("ldr" size " %" width "0,%1"      \
+    asm volatile("ldr" size " %" width(0) ",%1"    \
                  : "=r" (ret)                      \
                  : "m" (*(volatile type *)addr));  \
     return ret;                                    \
@@ -18,7 +18,7 @@ static inline type name(const volatile t
 #define build_atomic_write(name, size, width, type)    \
 static inline void name(volatile type *addr, type val) \
 {                                                      \
-    asm volatile("str" size " %"width"1,%0"            \
+    asm volatile("str" size " %" width(1) ",%0"        \
                  : "=m" (*(volatile type *)addr)       \
                  : "r" (val));                         \
 }
@@ -27,19 +27,20 @@ static inline void name(volatile type *a
 static inline void name(volatile type *addr, type val)                  \
 {                                                                       \
     type t;                                                             \
-    asm volatile("ldr" size " %"width"1,%0\n"                           \
-                 "add %"width"1,%"width"1,%"width"2\n"                  \
-                 "str" size " %"width"1,%0"                             \
+    asm volatile("ldr" size " %" width(1) ",%0\n"                       \
+                 "add %" width(1) ",%" width(1) ",%" width(2) "\n"      \
+                 "str" size " %" width(1) ",%0"                         \
                  : "+m" (*addr), "=&r" (t)                              \
                  : "ri" (val));                                         \
 }
 
 #if defined (CONFIG_ARM_32)
-#define BYTE ""
-#define WORD ""
+#define BYTE(n) #n
+#define WORD(n) #n
 #elif defined (CONFIG_ARM_64)
-#define BYTE "w"
-#define WORD "w"
+#define BYTE(n)  "w" #n
+#define WORD(n)  "w" #n
+#define DWORD(n) "" #n
 #endif
 
 build_atomic_read(read_u8_atomic,  "b", BYTE, uint8_t)
@@ -53,8 +54,8 @@ build_atomic_write(write_u32_atomic, "",
 build_atomic_write(write_int_atomic, "",  WORD, int)
 
 #if defined (CONFIG_ARM_64)
-build_atomic_read(read_u64_atomic, "", "", uint64_t)
-build_atomic_write(write_u64_atomic, "", "", uint64_t)
+build_atomic_read(read_u64_atomic, "", DWORD, uint64_t)
+build_atomic_write(write_u64_atomic, "", DWORD, uint64_t)
 #elif defined (CONFIG_ARM_32)
 static inline uint64_t read_u64_atomic(const volatile uint64_t *addr)
 {





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/7] Arm/atomic: unify generation of u64 read/write functions

[Jan Beulich]

By adding another suitable abstracting macro the need for explicit
inline function definitions in the 32-bit case goes away.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The selected name for the new macro (PAIR) is subject to improvement
suggestions.

--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -35,42 +35,29 @@ static inline void name(volatile type *a
 }
 
 #if defined (CONFIG_ARM_32)
-#define BYTE(n) #n
-#define WORD(n) #n
+#define BYTE(n)  #n
+#define WORD(n)  #n
+#define DWORD(n) "" #n ",%H" #n
+#define PAIR     "d"
 #elif defined (CONFIG_ARM_64)
 #define BYTE(n)  "w" #n
 #define WORD(n)  "w" #n
 #define DWORD(n) "" #n
+#define PAIR     ""
 #endif
 
 build_atomic_read(read_u8_atomic,  "b", BYTE, uint8_t)
 build_atomic_read(read_u16_atomic, "h", WORD, uint16_t)
 build_atomic_read(read_u32_atomic, "",  WORD, uint32_t)
+build_atomic_read(read_u64_atomic, PAIR, DWORD, uint64_t)
 build_atomic_read(read_int_atomic, "",  WORD, int)
 
 build_atomic_write(write_u8_atomic,  "b", BYTE, uint8_t)
 build_atomic_write(write_u16_atomic, "h", WORD, uint16_t)
 build_atomic_write(write_u32_atomic, "",  WORD, uint32_t)
+build_atomic_write(write_u64_atomic, PAIR, DWORD, uint64_t)
 build_atomic_write(write_int_atomic, "",  WORD, int)
 
-#if defined (CONFIG_ARM_64)
-build_atomic_read(read_u64_atomic, "", DWORD, uint64_t)
-build_atomic_write(write_u64_atomic, "", DWORD, uint64_t)
-#elif defined (CONFIG_ARM_32)
-static inline uint64_t read_u64_atomic(const volatile uint64_t *addr)
-{
-    uint64_t val;
-
-    asm volatile ( "ldrd %0,%H0,%1" : "=r" (val) : "m" (*addr) );
-
-    return val;
-}
-static inline void write_u64_atomic(volatile uint64_t *addr, uint64_t val)
-{
-    asm volatile ( "strd %1,%H1,%0" : "=m" (*addr) : "r" (val) );
-}
-#endif
-
 build_add_sized(add_u8_sized, "b", BYTE, uint8_t)
 build_add_sized(add_u16_sized, "h", WORD, uint16_t)
 build_add_sized(add_u32_sized, "", WORD, uint32_t)





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 7/7] Arm/atomic: cosmetics

[Jan Beulich]

Correct coding style of asm() invocations. Drop redundant casts.
Un-define no longer needed macros after use.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -9,29 +9,29 @@
 static inline type name(const volatile type *addr) \
 {                                                  \
     type ret;                                      \
-    asm volatile("ldr" size " %" width(0) ",%1"    \
-                 : "=r" (ret)                      \
-                 : "m" (*(volatile type *)addr));  \
+    asm volatile ( "ldr" size " %" width(0) ",%1"  \
+                   : "=r" (ret)                    \
+                   : "m" (*addr) );                \
     return ret;                                    \
 }
 
 #define build_atomic_write(name, size, width, type)    \
 static inline void name(volatile type *addr, type val) \
 {                                                      \
-    asm volatile("str" size " %" width(1) ",%0"        \
-                 : "=m" (*(volatile type *)addr)       \
-                 : "r" (val));                         \
+    asm volatile ( "str" size " %" width(1) ",%0"      \
+                   : "=m" (*addr)                      \
+                   : "r" (val) );                      \
 }
 
-#define build_add_sized(name, size, width, type)                        \
-static inline void name(volatile type *addr, type val)                  \
-{                                                                       \
-    type t;                                                             \
-    asm volatile("ldr" size " %" width(1) ",%0\n"                       \
-                 "add %" width(1) ",%" width(1) ",%" width(2) "\n"      \
-                 "str" size " %" width(1) ",%0"                         \
-                 : "+m" (*addr), "=&r" (t)                              \
-                 : "ri" (val));                                         \
+#define build_add_sized(name, size, width, type)                     \
+static inline void name(volatile type *addr, type val)               \
+{                                                                    \
+    type t;                                                          \
+    asm volatile ( "ldr" size " %" width(1) ",%0\n"                  \
+                   "add %" width(1) ",%" width(1) ",%" width(2) "\n" \
+                   "str" size " %" width(1) ",%0"                    \
+                   : "+m" (*addr), "=&r" (t)                         \
+                   : "ri" (val) );                                   \
 }
 
 #if defined (CONFIG_ARM_32)
@@ -62,6 +62,15 @@ build_add_sized(add_u8_sized, "b", BYTE,
 build_add_sized(add_u16_sized, "h", WORD, uint16_t)
 build_add_sized(add_u32_sized, "", WORD, uint32_t)
 
+#undef BYTE
+#undef WORD
+#undef DWORD
+#undef PAIR
+
+#undef build_atomic_read
+#undef build_atomic_write
+#undef build_add_sized
+
 void __bad_atomic_size(void);
 
 #define read_atomic(p) ({                                               \





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] events: drop arch_evtchn_inject()

[Julien Grall]

Hi,

On 11/03/2019 16:48, Jan Beulich wrote:
> Have the only user call vcpu_mark_events_pending() instead, at the same
> time arranging for correct ordering of the writes (evtchn_pending_sel
> should be written before evtchn_upcall_pending).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] common: avoid atomic read-modify-write accesses in map_vcpu_info()

[Julien Grall]

Hi,

On 11/03/2019 16:50, Andrew Cooper wrote:
> On 11/03/2019 16:48, Jan Beulich wrote:
>> There's no need to set the evtchn_pending_sel bits one by one. Simply
>> write full words with all ones.
>>
>> For Arm this requires extending write_atomic() to also handle 64-bit
>> values; for symmetry read_atomic() gets adjusted as well.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> (I'll leave the ARM assembly to Julien to check over)

Reviewed-by: Julien Grall <julien.grall@arm.com>

I can queue this series in my next branch once it is fully acked.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] Arm/atomic: correct asm() constraints in build_add_sized()

[Julien Grall]

Hi Jan,

On 11/03/2019 16:49, Jan Beulich wrote:
> The memory operand is an in/out one, and the auxiliary register gets
> written to early.
> 
> Take the opportunity and also drop the redundant cast (the inline
> functions' parameters are already of the casted-to type).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I guess this is candidate for backporting.

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

> 
> --- a/xen/include/asm-arm/atomic.h
> +++ b/xen/include/asm-arm/atomic.h
> @@ -30,7 +30,7 @@ static inline void name(volatile type *a
>       asm volatile("ldr" size " %"width"1,%0\n"                           \
>                    "add %"width"1,%"width"1,%"width"2\n"                  \
>                    "str" size " %"width"1,%0"                             \
> -                 : "=m" (*(volatile type *)addr), "=r" (t)              \
> +                 : "+m" (*addr), "=&r" (t)                              \
>                    : reg (val));                                          \
>   }
>   
> 
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] Arm/atomic: drop uniformly used reg macro parameters

[Julien Grall]

Hi Jan,

On 11/03/2019 16:50, Jan Beulich wrote:
> There's no point in parameterizing these when all use sites pass the
> same arguments.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] Arm/atomic: parameterize register modifier macro arguments

[Julien Grall]

Hi,

On 11/03/2019 16:51, Jan Beulich wrote:
> Make the abstracting macros take the asm() operand specifier as
> argument, in preparation of doing away with the split u64 read/write
> definitions.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/7] Arm/atomic: unify generation of u64 read/write functions

[Julien Grall]

Hi,

On 11/03/2019 16:51, Jan Beulich wrote:
> By adding another suitable abstracting macro the need for explicit
> inline function definitions in the 32-bit case goes away.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The selected name for the new macro (PAIR) is subject to improvement
> suggestions.

I don't have a better name.

> 
> --- a/xen/include/asm-arm/atomic.h
> +++ b/xen/include/asm-arm/atomic.h
> @@ -35,42 +35,29 @@ static inline void name(volatile type *a
>   }
>   
>   #if defined (CONFIG_ARM_32)
> -#define BYTE(n) #n
> -#define WORD(n) #n
> +#define BYTE(n)  #n
> +#define WORD(n)  #n
> +#define DWORD(n) "" #n ",%H" #n
> +#define PAIR     "d"

Can you please avoid changes that just help to keep things aligned. This is just 
making the review a bit more difficult for not real benefits.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] Arm/atomic: cosmetics

[Julien Grall]

Hi Jan,

On 11/03/2019 16:52, Jan Beulich wrote:
> Correct coding style of asm() invocations.
Coding style from where? Most of the Arm code does not contain space before ( 
and after ) for asm invocations. I also can't find anywhere in CODING_STYLE 
imposing this style. So please remove this change.

>  Drop redundant casts.
> Un-define no longer needed macros after use.

I am happy with the those two changes.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] common: avoid atomic read-modify-write accesses in map_vcpu_info()

[Jan Beulich]

>>> On 11.03.19 at 18:53, <julien.grall@arm.com> wrote:
> Hi,
> 
> On 11/03/2019 16:50, Andrew Cooper wrote:
>> On 11/03/2019 16:48, Jan Beulich wrote:
>>> There's no need to set the evtchn_pending_sel bits one by one. Simply
>>> write full words with all ones.
>>>
>>> For Arm this requires extending write_atomic() to also handle 64-bit
>>> values; for symmetry read_atomic() gets adjusted as well.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> 
>> (I'll leave the ARM assembly to Julien to check over)
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> I can queue this series in my next branch once it is fully acked.

I was meaning to apply right away whatever initial parts of it are
fully acked, as this isn't a major change, and it doesn't have a
significant risk of conflicts with potential last minute changes for
4.12.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] Arm/atomic: cosmetics

[Jan Beulich]

>>> On 11.03.19 at 19:19, <julien.grall@arm.com> wrote:
> On 11/03/2019 16:52, Jan Beulich wrote:
>> Correct coding style of asm() invocations.
> Coding style from where? Most of the Arm code does not contain space before ( 
> and after ) for asm invocations. I also can't find anywhere in CODING_STYLE 
> imposing this style. So please remove this change.

"asm" is a keyword just like "if" or "while", so the general "White space"
section is as applicable here. I can split the change if this part is
controversial, but if you want me to drop the change, then an Arm
specific annex to ./CODING_STYLE is going to be needed.

Jan

>>  Drop redundant casts.
>> Un-define no longer needed macros after use.
> 
> I am happy with the those two changes.
> 
> Cheers,
> 
> -- 
> Julien Grall





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] common: avoid atomic read-modify-write accesses in map_vcpu_info()

[Julien Grall]

Hi Jan,

On 3/12/19 9:30 AM, Jan Beulich wrote:
>>>> On 11.03.19 at 18:53, <julien.grall@arm.com> wrote:
>> Hi,
>>
>> On 11/03/2019 16:50, Andrew Cooper wrote:
>>> On 11/03/2019 16:48, Jan Beulich wrote:
>>>> There's no need to set the evtchn_pending_sel bits one by one. Simply
>>>> write full words with all ones.
>>>>
>>>> For Arm this requires extending write_atomic() to also handle 64-bit
>>>> values; for symmetry read_atomic() gets adjusted as well.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> (I'll leave the ARM assembly to Julien to check over)
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>>
>> I can queue this series in my next branch once it is fully acked.
> 
> I was meaning to apply right away whatever initial parts of it are
> fully acked, as this isn't a major change, and it doesn't have a
> significant risk of conflicts with potential last minute changes for
> 4.12.

This works for me.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] Arm/atomic: cosmetics

[Julien Grall]

Hi Jan,

On 3/12/19 9:35 AM, Jan Beulich wrote:
>>>> On 11.03.19 at 19:19, <julien.grall@arm.com> wrote:
>> On 11/03/2019 16:52, Jan Beulich wrote:
>>> Correct coding style of asm() invocations.
>> Coding style from where? Most of the Arm code does not contain space before (
>> and after ) for asm invocations. I also can't find anywhere in CODING_STYLE
>> imposing this style. So please remove this change.
> 
> "asm" is a keyword just like "if" or "while", so the general "White space"
> section is as applicable here. I can split the change if this part is
> controversial, but if you want me to drop the change, then an Arm
> specific annex to ./CODING_STYLE is going to be needed.

Strictly speaking, the section you refer mentions space between 
"keywords" and "conditions". So I don't feel the section applies for 
"asm volatile".

For Arm, I would still prefer to keep without space as we have most of 
the code following that.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] Arm/atomic: cosmetics

[Jan Beulich]

>>> On 12.03.19 at 11:23, <julien.grall@arm.com> wrote:
> On 3/12/19 9:35 AM, Jan Beulich wrote:
>>>>> On 11.03.19 at 19:19, <julien.grall@arm.com> wrote:
>>> On 11/03/2019 16:52, Jan Beulich wrote:
>>>> Correct coding style of asm() invocations.
>>> Coding style from where? Most of the Arm code does not contain space before 
>>> and after ) for asm invocations. I also can't find anywhere in CODING_STYLE
>>> imposing this style. So please remove this change.
>> 
>> "asm" is a keyword just like "if" or "while", so the general "White space"
>> section is as applicable here. I can split the change if this part is
>> controversial, but if you want me to drop the change, then an Arm
>> specific annex to ./CODING_STYLE is going to be needed.
> 
> Strictly speaking, the section you refer mentions space between 
> "keywords" and "conditions". So I don't feel the section applies for 
> "asm volatile".

Well, if you take it to the word then it wouldn't be applicable to
for(;;) either, and perhaps also not to switch(). To me this
paragraph talks about (control) keywords in general that are
followed by what one may call "operands" in parentheses.

> For Arm, I would still prefer to keep without space as we have most of 
> the code following that.

Well, as said - I'll split the patch then, but I'll refrain from re-
submitting the asm() part only if an Arm specific amendment to
the style guide appears. Otherwise Arm code should imo be
brought in sync with what we (try to) consistently do in x86
and common code. And of course such bringing in sync can
happen over time, as code gets touched anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] Arm/atomic: cosmetics

[Julien Grall]

Hi Jan,

On 12/03/2019 12:53, Jan Beulich wrote:
>>>> On 12.03.19 at 11:23, <julien.grall@arm.com> wrote:
>> On 3/12/19 9:35 AM, Jan Beulich wrote:
>>>>>> On 11.03.19 at 19:19, <julien.grall@arm.com> wrote:
>>>> On 11/03/2019 16:52, Jan Beulich wrote:
>>>>> Correct coding style of asm() invocations.
>>>> Coding style from where? Most of the Arm code does not contain space before
>>>> and after ) for asm invocations. I also can't find anywhere in CODING_STYLE
>>>> imposing this style. So please remove this change.
>>>
>>> "asm" is a keyword just like "if" or "while", so the general "White space"
>>> section is as applicable here. I can split the change if this part is
>>> controversial, but if you want me to drop the change, then an Arm
>>> specific annex to ./CODING_STYLE is going to be needed.
>>
>> Strictly speaking, the section you refer mentions space between
>> "keywords" and "conditions". So I don't feel the section applies for
>> "asm volatile".`
> 
> Well, if you take it to the word then it wouldn't be applicable to
> for(;;) either, and perhaps also not to switch(). To me this
> paragraph talks about (control) keywords in general that are
> followed by what one may call "operands" in parentheses.

The CODING_STYLE file has always been a set of rough guidelines with 
various interpretation.

> 
>> For Arm, I would still prefer to keep without space as we have most of
>> the code following that.
> 
> Well, as said - I'll split the patch then, but I'll refrain from re-
> submitting the asm() part only if an Arm specific amendment to
> the style guide appears. Otherwise Arm code should imo be
> brought in sync with what we (try to) consistently do in x86
> and common code. And of course such bringing in sync can
> happen over time, as code gets touched anyway.

As said above, the CODING_STYLE file can lead to multiple 
interpretations. If you are not happy with my interpretation then feel 
free to send a patch to clarify it.

But then you need to explain why we should use the x86 coding style for 
"asm" and not the arm one...

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] Arm/atomic: correct asm() constraints in build_add_sized()

[Jan Beulich]

>>> On 11.03.19 at 19:08, <julien.grall@arm.com> wrote:
> On 11/03/2019 16:49, Jan Beulich wrote:
>> The memory operand is an in/out one, and the auxiliary register gets
>> written to early.
>> 
>> Take the opportunity and also drop the redundant cast (the inline
>> functions' parameters are already of the casted-to type).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I guess this is candidate for backporting.

Btw, do you want me to queue this, or will you or Stefano want to
take care?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

BACKPORT Re: [PATCH 3/7] Arm/atomic: correct asm() constraints in build_add_sized()

[Julien Grall]

On 13/03/2019 10:21, Jan Beulich wrote:
>>>> On 11.03.19 at 19:08, <julien.grall@arm.com> wrote:
>> On 11/03/2019 16:49, Jan Beulich wrote:
>>> The memory operand is an in/out one, and the auxiliary register gets
>>> written to early.
>>>
>>> Take the opportunity and also drop the redundant cast (the inline
>>> functions' parameters are already of the casted-to type).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I guess this is candidate for backporting.
> 
> Btw, do you want me to queue this, or will you or Stefano want to
> take care?

Stefano is taking care of backporting. Stefano, can you queue it for all the 
tree you can backport to?

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: BACKPORT Re: [PATCH 3/7] Arm/atomic: correct asm() constraints in build_add_sized()

[Stefano Stabellini]

On Wed, 13 Mar 2019, Julien Grall wrote:
> On 13/03/2019 10:21, Jan Beulich wrote:
> > > > > On 11.03.19 at 19:08, <julien.grall@arm.com> wrote:
> > > On 11/03/2019 16:49, Jan Beulich wrote:
> > > > The memory operand is an in/out one, and the auxiliary register gets
> > > > written to early.
> > > > 
> > > > Take the opportunity and also drop the redundant cast (the inline
> > > > functions' parameters are already of the casted-to type).
> > > > 
> > > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > I guess this is candidate for backporting.
> > 
> > Btw, do you want me to queue this, or will you or Stefano want to
> > take care?
> 
> Stefano is taking care of backporting. Stefano, can you queue it for all the
> tree you can backport to?

Done, and sorry for the delay.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] BACKPORT Re: [PATCH 3/7] Arm/atomic: correct asm() constraints in build_add_sized()

[Stefano Stabellini]

On Wed, 13 Mar 2019, Julien Grall wrote:
> On 13/03/2019 10:21, Jan Beulich wrote:
> > > > > On 11.03.19 at 19:08, <julien.grall@arm.com> wrote:
> > > On 11/03/2019 16:49, Jan Beulich wrote:
> > > > The memory operand is an in/out one, and the auxiliary register gets
> > > > written to early.
> > > > 
> > > > Take the opportunity and also drop the redundant cast (the inline
> > > > functions' parameters are already of the casted-to type).
> > > > 
> > > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > I guess this is candidate for backporting.
> > 
> > Btw, do you want me to queue this, or will you or Stefano want to
> > take care?
> 
> Stefano is taking care of backporting. Stefano, can you queue it for all the
> tree you can backport to?

Done, and sorry for the delay.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel