arm64: 32-bit tolerant sync bitops

Signed-off-by Vladimir Murzin <[email protected]>

---
For those who whant to play with original issue [1]

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

 drivers/xen/events/events_fifo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/xen/events/events_fifo.c +0 -0

index 96109a9..91248d8 100644

Signed-off-by Vladimir Murzin <[email protected]>

---
For those who whant to play with original issue [1]

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

 drivers/xen/events/events_fifo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/xen/events/events_fifo.c +0 -0

index 96109a9..91248d8 100644

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

bool sync_test_bit32(uint32_t *v, unsigned bit)

{
     if (sizeof(unsigned long) == 8 && (unsigned long)v & 0x4)
         return sync_test_bit((unsigned long *)(v - 1), bit + 32);
     else
         return sync_test_bit((unsigned long *)v, bit);
}

David

Quoted 27 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

bool sync_test_bit32(uint32_t *v, unsigned bit)

Quoted 8 lines

{
     if (sizeof(unsigned long) == 8 && (unsigned long)v & 0x4)
         return sync_test_bit((unsigned long *)(v - 1), bit + 32);
     else
         return sync_test_bit((unsigned long *)v, bit);
}

David

Talking about separate 32-bit ops I mean arch specific bitops which are targeting for 32-bit size/alignment.
With separate API for Xen it looks awkward for me, because currently there are only a few users for sync_*_bit ops - Xen and HV. Xen assumes that these ops are 32 bit and looks like never try to deal with 64-bit (am I overlooking something?). So, sync ops are 32-bit de-facto, having separate version means there is support for both 32-bit and 64-bit ops, but (by now) there is no user for 64-bit ops. All this lead to obvious question why we need API conversion now? Is not it easier to turn assumptions to requirements?

Quoted 37 lines · Vladimir Murzin

Quoted 27 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

bool sync_test_bit32(uint32_t *v, unsigned bit)

Quoted 8 lines

{
     if (sizeof(unsigned long) == 8 && (unsigned long)v & 0x4)
         return sync_test_bit((unsigned long *)(v - 1), bit + 32);
     else
         return sync_test_bit((unsigned long *)v, bit);
}

David

Quoted 10 lines

Talking about separate 32-bit ops I mean arch specific bitops which are targeting for 32-bit size/alignment.
With separate API for Xen it looks awkward for me, because currently there are only a few users for sync_*_bit ops - Xen and HV. Xen assumes that these ops are 32 bit and looks like never try to deal with 64-bit (am I overlooking something?). So, sync ops are 32-bit de-facto, having separate version means there is support for both 32-bit and 64-bit ops, but (by now) there is no user for 64-bit ops. All this lead to obvious question why we need API conversion now? Is not it easier to turn assumptions to requirements?

Xen does use the sync bit ops on 64-bit values (for 2-level event channels).

Quoted 37 lines · Vladimir Murzin

Quoted 27 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

bool sync_test_bit32(uint32_t *v, unsigned bit)

Quoted 8 lines

{
     if (sizeof(unsigned long) == 8 && (unsigned long)v & 0x4)
         return sync_test_bit((unsigned long *)(v - 1), bit + 32);
     else
         return sync_test_bit((unsigned long *)v, bit);
}

David

Quoted 10 lines

Talking about separate 32-bit ops I mean arch specific bitops which are targeting for 32-bit size/alignment.
With separate API for Xen it looks awkward for me, because currently there are only a few users for sync_*_bit ops - Xen and HV. Xen assumes that these ops are 32 bit and looks like never try to deal with 64-bit (am I overlooking something?). So, sync ops are 32-bit de-facto, having separate version means there is support for both 32-bit and 64-bit ops, but (by now) there is no user for 64-bit ops. All this lead to obvious question why we need API conversion now? Is not it easier to turn assumptions to requirements?

Xen does use the sync bit ops on 64-bit values (for 2-level event channels).

Quoted 27 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

bool sync_test_bit32(uint32_t *v, unsigned bit)

Quoted 8 lines

{
     if (sizeof(unsigned long) == 8 && (unsigned long)v & 0x4)
         return sync_test_bit((unsigned long *)(v - 1), bit + 32);
     else
         return sync_test_bit((unsigned long *)v, bit);
}

David

Talking about separate 32-bit ops I mean arch specific bitops which are targeting for 32-bit size/alignment.
With separate API for Xen it looks awkward for me, because currently there are only a few users for sync_*_bit ops - Xen and HV. Xen assumes that these ops are 32 bit and looks like never try to deal with 64-bit (am I overlooking something?). So, sync ops are 32-bit de-facto, having separate version means there is support for both 32-bit and 64-bit ops, but (by now) there is no user for 64-bit ops. All this lead to obvious question why we need API conversion now? Is not it easier to turn assumptions to requirements?

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

bool sync_test_bit32(uint32_t *v, unsigned bit)

{
     if (sizeof(unsigned long) == 8 && (unsigned long)v & 0x4)
         return sync_test_bit((unsigned long *)(v - 1), bit + 32);
     else
         return sync_test_bit((unsigned long *)v, bit);
}

David

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

Quoted 21 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

Quoted 4 lines

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

It seems like there isn't currently much call for/interest in a generic 32-bit set of bitops. Since this is a regression on arm64 right now how about we simply fix it up in the Xen layer now and if in the future a common need is found we can rework to use it.

We already have evtchn_fifo_{clear,set,is}_pending (although evtchn_fifo_unmask and consume_one_event need fixing to use is_pending) and evtchn_fifo_{test_and_set_,}mask, plus we would need evtchn_fifo_set_mask for consume_one_event to use instead of open coding.

With that then those helpers can become something like:

        #define BM(w) (unsigned long *)(w & ~0x7)
        #define EVTCHN_FIFO_BIT(b, w) (BUG_ON(w&0x3), w & 0x4 ? EVTCHN_FIFO_#b + 32 : EVTCHN_FIFO_#b)
        static void evtchn_fifo_clear_pending(unsigned port)
        {
        	event_word_t *word = event_word_from_port(port);
        	sync_clear_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
        }

similarly for the others.

If that is undesirable in the common code then wrap each existing helper in #ifndef evtchn_fifo_clean_pending and put something like the above in arch/arm64/include/asm/xen/events.h with a #define (and/or make the helpers themselves macros, or #define HAVE_XEN_FIFO_EVTCHN_ACCESSORS etc etc)

We still need your patch from
http://article.gmane.org/gmane.comp.emulators.xen.devel/196067 for the other unaligned bitmap case. Note that this also removes the use of BM for non-PENDING/MASKED bits, which is why it was safe for me to change it above (also it would be safe to & ~0x7 after that change anyway).

Ian.

Might be dealing with it on Xen side is only way unless arm64 folks say what they think...

Quoted 2 lines · Vladimir Murzin

Might be dealing with it on Xen side is only way unless arm64 folks say what they think...

I don't expect the arm64 guys will have any interesting in changing the bitops in this way. So please can you fix this on the Xen side, perhaps in the way I suggested.

If at some point arm64 does gain 32-bit capable bitops we can always switch back to using them directly again.

In the meantime we need to get this Xen regression fixed before 3.15. If you don't have time to work on this please let us know.

Ian.

Quoted 2 lines · Vladimir Murzin

Might be dealing with it on Xen side is only way unless arm64 folks say what they think...

I don't expect the arm64 guys will have any interesting in changing the bitops in this way. So please can you fix this on the Xen side, perhaps in the way I suggested.

If at some point arm64 does gain 32-bit capable bitops we can always switch back to using them directly again.

In the meantime we need to get this Xen regression fixed before 3.15. If you don't have time to work on this please let us know.

Ian.

Might be dealing with it on Xen side is only way unless arm64 folks say what they think...

Quoted 54 lines · Ian Campbell

Quoted 21 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

Quoted 4 lines

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

It seems like there isn't currently much call for/interest in a generic 32-bit set of bitops. Since this is a regression on arm64 right now how about we simply fix it up in the Xen layer now and if in the future a common need is found we can rework to use it.

We already have evtchn_fifo_{clear,set,is}_pending (although evtchn_fifo_unmask and consume_one_event need fixing to use is_pending) and evtchn_fifo_{test_and_set_,}mask, plus we would need evtchn_fifo_set_mask for consume_one_event to use instead of open coding.

With that then those helpers can become something like:

        #define BM(w) (unsigned long *)(w & ~0x7)
        #define EVTCHN_FIFO_BIT(b, w) (BUG_ON(w&0x3), w & 0x4 ? EVTCHN_FIFO_#b + 32 : EVTCHN_FIFO_#b)
        static void evtchn_fifo_clear_pending(unsigned port)
        {
              event_word_t *word = event_word_from_port(port);
              sync_clear_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
        }

similarly for the others.

If that is undesirable in the common code then wrap each existing helper in #ifndef evtchn_fifo_clean_pending and put something like the above in arch/arm64/include/asm/xen/events.h with a #define (and/or make the helpers themselves macros, or #define HAVE_XEN_FIFO_EVTCHN_ACCESSORS etc etc)

Quoted 8 lines

We still need your patch from
http://article.gmane.org/gmane.comp.emulators.xen.devel/196067 for the other unaligned bitmap case. Note that this also removes the use of BM for non-PENDING/MASKED bits, which is why it was safe for me to change it above (also it would be safe to & ~0x7 after that change anyway).

Ian.

Might be dealing with it on Xen side is only way unless arm64 folks say what they think... because reports related to unaligned access still appear I'll put the minimally tested patch for making arm64 bitops 32-bits friendly ;)

---
 arch/arm64/lib/bitops.S | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

arch/arm64/lib/bitops.S +10 -11

index 7dac371..7c3b058 100644
@@ -26,14 +26,14 @@
  */
        .macro  bitop, name, instr
 ENTRY( \name   )
-       and     w3, w0, #63             // Get bit offset
+       and     w3, w0, #31             // Get bit offset
        eor     w0, w0, w3              // Clear low bits
        mov     x2, #1
-       add     x1, x1, x0, lsr #3      // Get word offset
+       add     x1, x1, x0, lsr #2      // Get word offset
        lsl     x3, x2, x3              // Create mask
-1:     ldxr    x2, [x1]
-       \instr  x2, x2, x3
-       stxr    w0, x2, [x1]
+1:     ldxr    w2, [x1]
+       \instr  w2, w2, w3
+       stxr    w0, w2, [x1]
        cbnz    w0, 1b
        ret
 ENDPROC(\name  )

@@ -41,15 +41,15 @@ ENDPROC(\name       )
 
        .macro  testop, name, instr
 ENTRY( \name   )
-       and     w3, w0, #63             // Get bit offset
+       and     w3, w0, #31             // Get bit offset
        eor     w0, w0, w3              // Clear low bits
        mov     x2, #1
-       add     x1, x1, x0, lsr #3      // Get word offset
+       add     x1, x1, x0, lsr #2      // Get word offset
        lsl     x4, x2, x3              // Create mask
-1:     ldxr    x2, [x1]
+1:     ldxr    w2, [x1]
        lsr     x0, x2, x3              // Save old value of bit
-       \instr  x2, x2, x4              // toggle bit
-       stlxr   w5, x2, [x1]
+       \instr  w2, w2, w4              // toggle bit
+       stlxr   w5, w2, [x1]
        cbnz    w5, 1b
        dmb     ish
        and     x0, x0, #1
--

1.8.3

Quoted 54 lines · Ian Campbell

Quoted 21 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

Quoted 4 lines

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

It seems like there isn't currently much call for/interest in a generic 32-bit set of bitops. Since this is a regression on arm64 right now how about we simply fix it up in the Xen layer now and if in the future a common need is found we can rework to use it.

We already have evtchn_fifo_{clear,set,is}_pending (although evtchn_fifo_unmask and consume_one_event need fixing to use is_pending) and evtchn_fifo_{test_and_set_,}mask, plus we would need evtchn_fifo_set_mask for consume_one_event to use instead of open coding.

With that then those helpers can become something like:

        #define BM(w) (unsigned long *)(w & ~0x7)
        #define EVTCHN_FIFO_BIT(b, w) (BUG_ON(w&0x3), w & 0x4 ? EVTCHN_FIFO_#b + 32 : EVTCHN_FIFO_#b)
        static void evtchn_fifo_clear_pending(unsigned port)
        {
              event_word_t *word = event_word_from_port(port);
              sync_clear_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
        }

similarly for the others.

If that is undesirable in the common code then wrap each existing helper in #ifndef evtchn_fifo_clean_pending and put something like the above in arch/arm64/include/asm/xen/events.h with a #define (and/or make the helpers themselves macros, or #define HAVE_XEN_FIFO_EVTCHN_ACCESSORS etc etc)

Quoted 8 lines

We still need your patch from
http://article.gmane.org/gmane.comp.emulators.xen.devel/196067 for the other unaligned bitmap case. Note that this also removes the use of BM for non-PENDING/MASKED bits, which is why it was safe for me to change it above (also it would be safe to & ~0x7 after that change anyway).

Ian.

Might be dealing with it on Xen side is only way unless arm64 folks say what they think... because reports related to unaligned access still appear I'll put the minimally tested patch for making arm64 bitops 32-bits friendly ;)

---
 arch/arm64/lib/bitops.S | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

arch/arm64/lib/bitops.S +10 -11

index 7dac371..7c3b058 100644
@@ -26,14 +26,14 @@
  */
        .macro  bitop, name, instr
 ENTRY( \name   )
-       and     w3, w0, #63             // Get bit offset
+       and     w3, w0, #31             // Get bit offset
        eor     w0, w0, w3              // Clear low bits
        mov     x2, #1
-       add     x1, x1, x0, lsr #3      // Get word offset
+       add     x1, x1, x0, lsr #2      // Get word offset
        lsl     x3, x2, x3              // Create mask
-1:     ldxr    x2, [x1]
-       \instr  x2, x2, x3
-       stxr    w0, x2, [x1]
+1:     ldxr    w2, [x1]
+       \instr  w2, w2, w3
+       stxr    w0, w2, [x1]
        cbnz    w0, 1b
        ret
 ENDPROC(\name  )

@@ -41,15 +41,15 @@ ENDPROC(\name       )
 
        .macro  testop, name, instr
 ENTRY( \name   )
-       and     w3, w0, #63             // Get bit offset
+       and     w3, w0, #31             // Get bit offset
        eor     w0, w0, w3              // Clear low bits
        mov     x2, #1
-       add     x1, x1, x0, lsr #3      // Get word offset
+       add     x1, x1, x0, lsr #2      // Get word offset
        lsl     x4, x2, x3              // Create mask
-1:     ldxr    x2, [x1]
+1:     ldxr    w2, [x1]
        lsr     x0, x2, x3              // Save old value of bit
-       \instr  x2, x2, x4              // toggle bit
-       stlxr   w5, x2, [x1]
+       \instr  w2, w2, w4              // toggle bit
+       stlxr   w5, w2, [x1]
        cbnz    w5, 1b
        dmb     ish
        and     x0, x0, #1
--

1.8.3

Quoted 21 lines · David Vrabel

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

Quoted 4 lines

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

It seems like there isn't currently much call for/interest in a generic 32-bit set of bitops. Since this is a regression on arm64 right now how about we simply fix it up in the Xen layer now and if in the future a common need is found we can rework to use it.

We already have evtchn_fifo_{clear,set,is}_pending (although evtchn_fifo_unmask and consume_one_event need fixing to use is_pending) and evtchn_fifo_{test_and_set_,}mask, plus we would need evtchn_fifo_set_mask for consume_one_event to use instead of open coding.

With that then those helpers can become something like:

        #define BM(w) (unsigned long *)(w & ~0x7)
        #define EVTCHN_FIFO_BIT(b, w) (BUG_ON(w&0x3), w & 0x4 ? EVTCHN_FIFO_#b + 32 : EVTCHN_FIFO_#b)
        static void evtchn_fifo_clear_pending(unsigned port)
        {
        	event_word_t *word = event_word_from_port(port);
        	sync_clear_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
        }

similarly for the others.

If that is undesirable in the common code then wrap each existing helper in #ifndef evtchn_fifo_clean_pending and put something like the above in arch/arm64/include/asm/xen/events.h with a #define (and/or make the helpers themselves macros, or #define HAVE_XEN_FIFO_EVTCHN_ACCESSORS etc etc)

We still need your patch from
http://article.gmane.org/gmane.comp.emulators.xen.devel/196067 for the other unaligned bitmap case. Note that this also removes the use of BM for non-PENDING/MASKED bits, which is why it was safe for me to change it above (also it would be safe to & ~0x7 after that change anyway).

Ian.

Quoted 12 lines · Vladimir Murzin

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Quoted 7 lines

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good

I've never been particularly happy with the way the events_fifo.c uses casts for the sync_*_bit() calls and I think we should do option 2.

A generic implementation could be something like:

Xen assumes that bit operations are able to operate on 32-bit size and alignment [1]. For arm64 bitops are based on atomic exclusive load/store instructions to guarantee that changes are made atomically. However, these instructions require that address to be aligned to the data size. Because, by default, bitops operates on 64-bit size it implies that address should be aligned appropriately. All these lead to breakage of Xen assumption for bitops properties.

With this patch 32-bit sized/aligned bitops is implemented.

[1] http://www.gossamer-threads.com/lists/xen/devel/325613

Signed-off-by Vladimir Murzin <[email protected]>

---
Apart this patch other approaches were implemented:
1. turn bitops to be 32-bit size/align tolerant.
the changes are minimal, but I'm not sure how broad side effect might be
2. separate 32-bit size/aligned operations.
it exports new API, which might not be good