From owner-svn-src-all@freebsd.org Mon Jan 25 18:47:11 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F0863A45455; Mon, 25 Jan 2016 18:47:11 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: from mail-ig0-x22c.google.com (mail-ig0-x22c.google.com [IPv6:2607:f8b0:4001:c05::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id BC806F5F; Mon, 25 Jan 2016 18:47:11 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: by mail-ig0-x22c.google.com with SMTP id ik10so40775830igb.1; Mon, 25 Jan 2016 10:47:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=wcRDTreqRtVIVyFGRRD5USfrUaVbh9p99/J9Hq6zxqM=; b=euPwLnJRy6xl8cCzApbwRtUS9pxKSPQhF197X+n1ttv7Xz8gICrFws35wfmwc5sdfI jmjGtWDLjcKNX+HyiDN6qI3fMUdyxpab9Y2R6Pb4j+sPE4+U12TzlL/gsi7PdLKPoZcD giHpEMmyI0WGgmbCGRMlZX4kzXCUE7q/qQkN4vc5xT1vg0gKJP8R1faERO1t/gu46M1A ro3ih0tly5CPPHfXeXMePRzVgdYYEXCDlB32iKRePu/rciB0Kbr4nlxyHJWHwuxNZ4gR kMlHdRB6q82HgtGnV+BKet0ZAbP2c3YhGErzPikLBPxQOhXghxOLrpTpwfeohNdaloEe hwOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=wcRDTreqRtVIVyFGRRD5USfrUaVbh9p99/J9Hq6zxqM=; b=cRT6DnLRRYaVccCgwpUPdrOusMout4wRZ2PCic78vEw0W2bMWZ+68hiSgVBtETrtmL 96gtbbmX1WNbNayaXV1pNNuAef0Ft5pUMcZTfmjh2BYYZ6jaTUA0Xs0jMzn8fR5+12MQ JeWV+Pua8X+ouCAJJWyM0qoYomGfMRAtPl0Dj/qA2uSUJnoTKTiBbXKTy91pkG/AfXWW z00P5EKt5DNzMQNbHmGYa2b3p+QmRQCPY6tih7R2sRHJoWt3m8EcQs5jYwCdDOeBqHB0 V/zTxXV8LlXwxxSd2yq+xzAhlsx3iTCWxxs3KrT8KHfM3fX8wYUgZ3DHioNFmKl9FZyP zEDA== X-Gm-Message-State: AG10YORgaKYr5zIEGpffoq8r7gzLmnkJCnRGvji1PB5hRVmbYriAy5vKdlnqkXKVK4Hnu0DA9h68sujsBhoKVg== MIME-Version: 1.0 X-Received: by 10.50.40.8 with SMTP id t8mr19436418igk.26.1453747631217; Mon, 25 Jan 2016 10:47:11 -0800 (PST) Received: by 10.64.55.97 with HTTP; Mon, 25 Jan 2016 10:47:11 -0800 (PST) In-Reply-To: <20160125174338.29103e6b@zapp.Home> References: <201601251409.u0PE9abE013306@repo.freebsd.org> <20160125154453.GA3942@kib.kiev.ua> <20160125170053.7ae20536@zapp.Home> <1453742926.42081.8.camel@freebsd.org> <20160125174338.29103e6b@zapp.Home> Date: Mon, 25 Jan 2016 19:47:11 +0100 Message-ID: Subject: Re: svn commit: r294727 - head/sys/arm/arm From: Svatopluk Kraus To: Andrew Turner Cc: Ian Lepore , Konstantin Belousov , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Jan 2016 18:47:12 -0000 On Mon, Jan 25, 2016 at 6:43 PM, Andrew Turner wrote: > On Mon, 25 Jan 2016 10:28:46 -0700 > Ian Lepore wrote: > >> On Mon, 2016-01-25 at 17:00 +0000, Andrew Turner wrote: >> > On Mon, 25 Jan 2016 17:44:53 +0200 >> > Konstantin Belousov wrote: >> > >> > > On Mon, Jan 25, 2016 at 02:09:36PM +0000, Svatopluk Kraus wrote: >> > > > Author: skra >> > > > Date: Mon Jan 25 14:09:35 2016 >> > > > New Revision: 294727 >> > > > URL: https://svnweb.freebsd.org/changeset/base/294727 >> > > > >> > > > Log: >> > > > Fix an occasional undefined instruction abort during module >> > > > loading. >> > > > Even if data cache maintenance was done by IO code, the >> > > > relocation >> > > > fixup process creates dirty cache entries that we must write >> > > > back >> > > > before doing icache sync. >> > > Does arm64 need the same fix ? >> > > >> > >> > I don't think so. On arm64 we call cpu_icache_sync_range to sync the >> > I >> > and D cache. This will clean the D-Cache to the Point of Coherency, >> > then invalidate the I-Cache. I think this is slightly wrong as we >> > only >> > need to clean to the Point of Unification. >> > >> > Looking at the ARMv7 implementation of cpu_icache_sync_all is wrong, >> > it >> > only invalidates the I-Cache. cpu_icache_sync_range is almost >> > correct, >> > it will clean the D-Cache, with the same issue as arm64, but it is >> > missing a barrier after this operation, and is missing a branch >> > predictor invalidation. >> > >> > This change seems to be working around the brokenness of the >> > existing cache sync operations rather than fixing them, however I >> > don't know the >> > details on why this approach to fixing the issue was taken. >> > >> > Andrew >> > >> >> I disagree that the fact that icache_sync only cleans the icache means >> it's broken. It means that the callers have to do the right thing >> with the data cache before calling the icache ops depending on the >> situation, and that's what arm code has always done. > > If it's not broken then we are needlessly issuing extra cache handling > operations in other places. > > If we expect these function to sync the icache with an already clean > dcache then armv7_icache_sync_all looks correct (other than a missing > branch predictor invalidation). However in this case why are we > cleaning the dcache in armv7_icache_sync_range? > > In arm9_icache_sync_all and arm9_icache_sync_range we flush the icache > before cleaning the dcache. This also seems wrong given the above, > however I'm unsure on the details of cache handling on these older CPUs. > > Andrew Well, looking at patch, there is #if __ARM_ARCH >= 6 in the patch. For armv6/v7, the patch is perfectly correct. It does dcache_wb_pou() and then icache_inv_all(). According to arm arm manual, icache-invalidate-all does invalidate branch predictor cache too. I believe that cpu specific function (like armv7_icache_sync_all) are not used anymore for ARM_NEW_PMAP, which means armv6/v7. WRT armv4, I'm not familiar with their implementation, but the function calling sequence should be okay too. Svata