From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] xen: fix alignment for bitops Date: Tue, 15 Apr 2014 14:18:22 +0100 Message-ID: <534D319E.9030804@citrix.com> References: <1397508202-16249-1-git-send-email-murzin.v@gmail.com> <534D0737.1060703@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Wa3Fr-0007T8-MV for xen-devel@lists.xenproject.org; Tue, 15 Apr 2014 13:18:27 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Vladimir Murzin Cc: Ian Campbell , Stefano Stabellini , Catalin Marinas , Will Deacon , xen-devel@lists.xenproject.org, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org On 15/04/14 14:13, Vladimir Murzin wrote: > On Tue, Apr 15, 2014 at 2:05 PM, Vladimir Murzin wrote: >> On Tue, Apr 15, 2014 at 11:17 AM, David Vrabel 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