From patchwork Thu Aug 11 19:39:11 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: bdegraaf@codeaurora.org X-Patchwork-Id: 9275831 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 933E960780 for ; Thu, 11 Aug 2016 19:41:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 822C0286D4 for ; Thu, 11 Aug 2016 19:41:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 76AC028786; Thu, 11 Aug 2016 19:41:21 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E5F38286D4 for ; Thu, 11 Aug 2016 19:41:20 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bXvpK-0000rd-1J; Thu, 11 Aug 2016 19:39:38 +0000 Received: from smtp.codeaurora.org ([198.145.29.96]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bXvpE-0000qJ-Pr for linux-arm-kernel@lists.infradead.org; Thu, 11 Aug 2016 19:39:33 +0000 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id F23EB6141A; Thu, 11 Aug 2016 19:39:11 +0000 (UTC) Received: from mail.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id A142861405; Thu, 11 Aug 2016 19:39:11 +0000 (UTC) MIME-Version: 1.0 Date: Thu, 11 Aug 2016 15:39:11 -0400 From: bdegraaf@codeaurora.org To: Will Deacon Subject: Re: [RFC] arm64: Enforce gettimeofday vdso structure read ordering In-Reply-To: <20160811155409.GE8232@arm.com> References: <20160811153744.3212-1-cov@codeaurora.org> <20160811155409.GE8232@arm.com> Message-ID: <8375bf253e2f9034e5c5943e68876935@codeaurora.org> X-Sender: bdegraaf@codeaurora.org User-Agent: Roundcube Webmail/1.1.4 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160811_123932_960284_9CBFF7CA X-CRM114-Status: GOOD ( 11.76 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Catalin Marinas , Timur Tabi , Nathan Lynch , linux-kernel@vger.kernel.org, Christopher Covington , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Sorry, this is my first go at proper email etiquette. I think I've got it now. ------------------------------------------------------------------------------ "Can you explain the problem that you're fixing here, please?" ------------------------------------------------------------------------------ Since your question included only the vdso_data structure itself and the vdso.c file changes, I'm assuming that's what you meant by "here." I moved tb_seq_count to the start of the vdso_data structure because of the nature of the ldar/strl instructions, which do not support offsets. This avoided the extra math to locate the structure offset, while at the same time using the same base pointer for the structure. ia64 does something similar and also puts this as the first field. I moved the use_syscall field up next to it just to keep most elements aligned according to their width. That "use_syscall" is the first element used in the logic was an additional reason I decided to move that one in particular. the update of tb_seq_count. /confused Will" -------------------------------------------------------------------------- Hopefully, this more detailed explanation helps make things clearer. Thank you, Brent --- a/arch/arm64/include/asm/vdso_datapage.h +++ b/arch/arm64/include/asm/vdso_datapage.h @@ -21,6 +21,8 @@ #ifndef __ASSEMBLY__ struct vdso_data { + __u32 tb_seq_count; /* Timebase sequence counter */ + __u32 use_syscall; __u64 cs_cycle_last; /* Timebase at clocksource init */ __u64 raw_time_sec; /* Raw time */ __u64 raw_time_nsec; @@ -30,14 +32,12 @@ struct vdso_data { __u64 xtime_coarse_nsec; __u64 wtm_clock_sec; /* Wall to monotonic time */ __u64 wtm_clock_nsec; - __u32 tb_seq_count; /* Timebase sequence counter */ /* cs_* members must be adjacent and in this order (ldp accesses) */ __u32 cs_mono_mult; /* NTP-adjusted clocksource multiplier */ __u32 cs_shift; /* Clocksource shift (mono = raw) */ __u32 cs_raw_mult; /* Raw clocksource multiplier */ __u32 tz_minuteswest; /* Whacky timezone stuff */ __u32 tz_dsttime; - __u32 use_syscall; }; ---------------------------------------------------------------------------- As to vdso.c, I changed the instructions used to access tb_seq_count (both in vdso.c and the gettimeofday.S file) to the load-acquire/store-release instructions because those instructions seem to be designed for this particular scenario: their "one-way" barriers enforce the order we need, on top of eliminating the explicit barriers. ---------------------------------------------------------------------------- --- a/arch/arm64/kernel/vdso.c +++ b/arch/arm64/kernel/vdso.c @@ -201,10 +201,12 @@ up_fail: */ void update_vsyscall(struct timekeeper *tk) { + register u32 tmp; u32 use_syscall = strcmp(tk->tkr_mono.clock->name, "arch_sys_counter"); - ++vdso_data->tb_seq_count; - smp_wmb(); + tmp = smp_load_acquire(&vdso_data->tb_seq_count); + ++tmp; + smp_store_release(&vdso_data->tb_seq_count, tmp); "This looks busted -- the writes to vdso_data can be reordered before