Re: [Xen-devel] [PATCH] xen: fix alignment for bitops

[Vladimir Murzin <murzin.v@xxxxxxxxx>]

On Tue, Apr 15, 2014 at 2:05 PM, Vladimir Murzin <murzin.v@xxxxxxxxx> wrote:
> On Tue, Apr 15, 2014 at 11:17 AM, David Vrabel <david.vrabel@xxxxxxxxxx> 
> 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.

Vladimir

> Vladimir
>
>>
>> David

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