From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3592C433EB for ; Thu, 16 Jul 2020 10:53:17 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C6372206C1 for ; Thu, 16 Jul 2020 10:53:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C6372206C1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jw1VY-0000Nv-HJ; Thu, 16 Jul 2020 10:52:56 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jw1VW-0000NC-OF for xen-devel@lists.xenproject.org; Thu, 16 Jul 2020 10:52:54 +0000 X-Inumbo-ID: 7fbf0128-c752-11ea-b7bb-bc764e2007e4 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 7fbf0128-c752-11ea-b7bb-bc764e2007e4; Thu, 16 Jul 2020 10:52:53 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 9C88FAD1F; Thu, 16 Jul 2020 10:52:56 +0000 (UTC) Subject: Re: [PATCH v3 1/2] x86: restore pv_rtc_handler() invocation To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= References: <5426dd6f-50cd-dc23-5c6b-0ab631d98d38@suse.com> <7dd4b668-06ca-807a-9cc1-77430b2376a8@suse.com> <20200715121347.GY7191@Air-de-Roger> <2b9de0fd-5973-ed66-868c-ffadca83edf3@suse.com> <20200715133217.GZ7191@Air-de-Roger> <20200715145144.GA7191@Air-de-Roger> <20200716103110.GM7191@Air-de-Roger> From: Jan Beulich Message-ID: <1ab50fc4-fbf4-7a5d-e98b-48442df5d2ca@suse.com> Date: Thu, 16 Jul 2020 12:52:53 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200716103110.GM7191@Air-de-Roger> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: "xen-devel@lists.xenproject.org" , Paul Durrant , Wei Liu , Andrew Cooper Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On 16.07.2020 12:31, Roger Pau Monné wrote: > On Thu, Jul 16, 2020 at 12:06:14PM +0200, Jan Beulich wrote: >> On 15.07.2020 16:51, Roger Pau Monné wrote: >>> On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote: >>>> On 15.07.2020 15:32, Roger Pau Monné wrote: >>>>> Feel free to change to ACCESS_ONCE or barrier if you think it's >>>>> clearer. >>>> >>>> I did so (also on the writer side), not the least based on guessing >>>> what Andrew would presumably have preferred. >>> >>> Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write >>> side actually required? I'm not sure I see what ACCESS_ONCE protects >>> against in handle_rtc_once. >> >> Well, this is all sort of a mess, I think. We have this mixture of >> ACCESS_ONCE() and read_atomic() / write_atomic(), but I don't think >> we use them consistently, and I'm not sure either is suitable to >> deal with all (theoretical) corner cases. >> >> read_atomic() / write_atomic() guarantee a single insn to be used >> to access a piece of data. I'm uncertain whether they also guarantee >> single access (i.e. that the compiler won't replicate the asm()-s). > > Yes, that would be my expectation from my reading of the manual, as > it prevents gcc from: "move it out of loops or omit it on the > assumption that the result from a previous call is still valid". > >> The wording in gcc doc is pretty precise, but not quite enough imo >> to be entirely certain. > > I agree it's not that precise. > >> ACCESS_ONCE() guarantees single access, but doesn't guarantee that >> the compiler wouldn't split this single access into multiple insns. >> (It's just, like elsewhere, that it would be pretty silly of it if >> it did.) >> >> Yesterday, as said, I tried to in particular do what I expect/guess >> Andrew would have wanted done. This is despite me not being entirely >> convinced this is the right thing to do here, i.e. personally I >> would have preferred read_atomic() / write_atomic(), as I think the >> intention of what the gcc doc is saying is what we want (taking >> into consideration both uses of "volatile" in these helpers). > > Well, gcc states: > > "Note that the compiler can move even volatile asm instructions > relative to other code, including across jump instructions." > > So I think we likely want to use {read/write}_atomic plus a compiler > barrier? I'm not sure anyway how the read of pv_rtc_handler could be > moved, but I guess I'm not that creative :). > > AFAICT we require a write_atomic in handle_rtc_once in order to assure > a single instruction is used (no barrier required), and then we > require a read_atomic + a compiler barrier in rtc_guest_write in order > to prevent the compiler from optimizing the accesses to 'hook' in any > way? (that barrier might not be strictly required, as you say it's not > fully clear whether 'asm volatile' doesn't provide the necessary > protection here). Yes, but I'd really like to wait for Andrew's input here before we make any further adjustments. In any even what has gone in yesterday is already better than what the original code was that needed restoring. Jan