Re: [PATCH] xen: fix alignment for bitops

[David Vrabel <david.vrabel@citrix.com>]

On 15/04/14 14:13, Vladimir Murzin wrote:
> On Tue, Apr 15, 2014 at 2:05 PM, Vladimir Murzin <murzin.v@gmail.com> wrote:
>> On Tue, Apr 15, 2014 at 11:17 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>>> On 14/04/14 21:43, Vladimir Murzin wrote:
>>>> Bitops operations like set/clear/change mandate world aligned pointer, mainly
>>>> because architectures specific implementation.
>>>>
>>>> Looks that DEFINE_PER_CPU does required alignment for cpu_control_block;
>>>> however, local copy used for bitops might not be world aligned.
>>>>
>>>> Instruct compiler to keep local copy of "ready" world aligned.
>>> [...]
>>>> --- a/drivers/xen/events/events_fifo.c
>>>> +++ b/drivers/xen/events/events_fifo.c
>>>> @@ -285,7 +285,7 @@ static void consume_one_event(unsigned cpu,
>>>>  static void evtchn_fifo_handle_events(unsigned cpu)
>>>>  {
>>>>       struct evtchn_fifo_control_block *control_block;
>>>> -     uint32_t ready;
>>>> +     uint32_t __aligned(sizeof(long)) ready;
>>>
>>> This doesn't look sufficient to me.  The event words within the event
>>> array are 32-bit sized/aligned and sync_set_bit() and sync_clear_bit()
>>> etc. are done on these words.
>>>
>>> I think arm64 is going to have to provide sync_clear_bit(),
>>> sync_set_bit(), sync_test_bit() and sync_test_and_set_bit() that work
>>> with 32-bit sized and aligned words.
>>
>> Looks reasonable. If I understand correctly these sync_* functions are
>> used by Xen only (or at least was introduced because of Xen).
>> I might take rework them for arm64 to operate on 32-bit
>> size/alignment. Does this sounds reasonable?
>> ... and what to do with this patch?
>>
> 
> I think it could be dropped if clear_bit(priority, BM(ready)) is
> replaced with sync_ equivalent.

ready can be unsigned long as Ian suggested.

David