From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mian Yousaf Kaukab Subject: Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag Date: Tue, 26 May 2020 17:28:47 +0200 Message-ID: <20200526152847.GA16107@suse.de> References: <20200512144803.24344-1-ykaukab@suse.de> <20200512144803.24344-2-ykaukab@suse.de> <52f099e4-5c03-2141-f049-cd3adeb04c5b@wwwdotorg.org> <20200513104127.GA2309@suse.de> <20200519230326.GA827289@bogus> <20200520085558.GB2136208@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20200520085558.GB2136208@ulmo> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Thierry Reding Cc: Rob Herring , Stephen Warren , robin.murphy-5wv7dgnIgG8@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, talho-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org, jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, afaerber-l3A5Bk7waGM@public.gmane.org, arnd-r2nGTMty4D4@public.gmane.org, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org List-Id: linux-tegra@vger.kernel.org On Wed, May 20, 2020 at 10:55:58AM +0200, Thierry Reding wrote: > On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote: > > On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote: > > > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote: > > > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote: > > > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote: > > > >>> Add documentation for the new optional flag added for SRAM driver. > > > >> > > > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml > > > >> > > > >>> + reserved-only: > > > >>> + description: > > > >>> + The flag indicating, that only SRAM reserved regions have to be remapped. > > > >>> + remapping type is selected depending upon no-memory-wc as usual. > > > >>> + type: boolean > > > >> > > > >> This feels a bit like a SW flag rather than a HW description, so I'm not > > > >> sure it's appropriate to put it into DT. > > > > > > > > Reserved regions themselves are software descriptions, no? Then we have 'pool' > > > > flag which is again a software flag and so on. This flag falls into same > > > > category and nothing out of ordinary. > > > > > > I suppose that's true to some extent. This is indeed a description of > > > the system environment presented to the SW that consumes the DT, which > > > is a bit more than pure HW description but still a description of > > > something imposed externally rather than describing something that's up > > > to the discretion of the consuming SW. So, go ahead. > > > > > > >> Are there any cases where the SW should map all of the SRAM, i.e. where > > > >> we wouldn't expect to set reserved-only? [...] > > > > > > > > Yes, here are a few examples: > > > > arch/arm/boot/dts/aspeed-g*.dtsi > > > > arch/arm/boot/dts/at91*.dtsi > > > > arch/arm/boot/dts/bcm7445.dtsi > > > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything > > > > except the reserved region. > > > > > > > >> [...] I'd expect reserved-only to be > > > >> the default, and perhaps only, mode of operation for the SRAM driver. > > > > > > > > It will break compatibility with existing dtbs. > > > > > > > >> If we can't do that because some SW currently expects to be able to map > > > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the > > > >> SRAM driver which parts it's using, hence still allowing the driver to > > > >> only map in-use portions? > > > > > > > > User doesn’t need sram driver in that case. It can use genalloc api directly. > > > > > > This sounds a bit odd. Without a driver for the reserved region, nothing > > > should be touching it, since otherwise there's no code that owns an > > > manages the region. If any code needs to consume the region, it should > > > obtain info about the region from some form of provider code that can > > > handle both the allocation and mapping. Anything else sounds like some > > > consumer code directly making use of DT nodes it doesn't own. But since > > > I'm not familiar enough with the SRAM driver and genalloc code that you > > > mention to fully understand the allocation paths I guess I won't object > > > for now, although it does still sound fishy. > > > > I'm fine with the concept, but I don't think a single flag is adequate. > > If there are reserved regions within the SRAM, then define child nodes > > to mark those regions reserved. I don't think you need a new flag. Just > > a 'reg' property and nothing else. > > It sounds to me like there are two different interpretations of SRAM and > reserved regions. On one hand, as you suggest, we have one SRAM that's > made available as genalloc pool and then individual regions can be > marked as reserved so that they aren't added to that pool. > > At the same time, each reserved region is also exposed as a separate > pool and that's in fact used by many consumers as a way of getting a > specific chunk of the SRAM for their own use (via phandle to the region > from the consumer's device tree node). > > In addition to that, the reserved region code doesn't actually fully do > its job because while the reserved region isn't actually added to the > "top-level" SRAM pool, the memory is still mapped. At the same time this > is something that we actually want because, like I mentioned, some of > the consumers do want to get at their SRAM chunks via references to the > partitions. > > The problem that this patch series is really trying to solve is another > still: the complete SRAM is always mapped to kernel memory, irrespective > of whether any regions are marked reserved or not and that can cause > speculative accesses to memory outside of the defined regions. > > Stephen's suggestion is to default to only mapping memory for which a > partition has been defined in the SRAM and assuming that all SRAM > outside of those partitions is off limits. I think that's a sensible > default and it's unambiguous. > > But as Yousaf points out that would break compatibility with existing > device trees. Depending on how you interpret the bindings one could > argue that those device trees are buggy and should have partitions > defined (in the cases I've looked at they end up using a fixed region > anyway, so that could've just been made explicit in the device tree). > > However, it also looks like all of the users that rely on the original > behaviour where they can just access the full pool are those that don't > define any reserved regions, whereas all users that do reserve regions > will actually use those reserved regions. > > So I think we can make use of this by differentiating in the driver > between SRAM nodes with or without children and change the behaviour > accordingly. I think that has the big advantage that it makes things > work as (I think) most people would expect and doesn't further > complicate the binding with extra flags. I tend to agree on mapping partitions only if they exist. So far I could only find one exception. It is arch/arm/boot/dts/armada-370.dtsi which is using the top level pool as well as a partition to reserve 32 bytes at the bottom of sram. This can be fixed along with the sram driver change, by adding another partition for the rest of the sram and using its handle in the crypto@90000 instead of top-level sram node handle. Do you see anymore exceptions where both top level pool and the partitions both are being used? Then on the backward compatibility topic, another issue is that boot code could add sram nodes dynamically. For example arch/arm/mach-k3/common.c in u-boot does it. This particular case will not break after the suggested change because it is not adding any partitions. However, there could be other boot-loaders which are not this lucky. > > Thierry /Yousaf 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=-3.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS 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 483BCC433DF for ; Tue, 26 May 2020 15:29:15 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 152FE20787 for ; Tue, 26 May 2020 15:29:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="FdPT+5y2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 152FE20787 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=3OpCqFBHpdhgezrONaoio09vNHtyMY4RC4BP/TJ5+jQ=; b=FdPT+5y2g8V0u5 Jv964FraN4p5AulqUnX/iToU+hgwPE/3YDBuBi8RFlfeveatZtgftzEmVdnpOZvnZGim9Y/Qy+3// nn4yeTQbpPZfeHAdVI2HjrVbNUdN74V/ApMc5fXWKGqIw+BJP2+6TMWj0HXRKH6l8BnvVXlEtsvOh RlUV7EyVv07X5fV9kMfHrfvHRlAKakXmlZ3yKI8Zx6Ybwmjmz3q0c+SfnG0IipJyj7kaHyhKrXtJZ JKnne0Q+ZSRPvH5rNNCzr7pooXz2p9dQBa7KTp2mouW9auEnJqOVTH7QIGd7jkL2HxuYkCuNpyYyl fYXs7orxvEbFS7Mh5c1w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jdbVr-0007KX-B8; Tue, 26 May 2020 15:29:07 +0000 Received: from mx2.suse.de ([195.135.220.15]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jdbVo-0007JS-05 for linux-arm-kernel@lists.infradead.org; Tue, 26 May 2020 15:29:05 +0000 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 3E94BB260; Tue, 26 May 2020 15:28:58 +0000 (UTC) Date: Tue, 26 May 2020 17:28:47 +0200 From: Mian Yousaf Kaukab To: Thierry Reding Subject: Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag Message-ID: <20200526152847.GA16107@suse.de> References: <20200512144803.24344-1-ykaukab@suse.de> <20200512144803.24344-2-ykaukab@suse.de> <52f099e4-5c03-2141-f049-cd3adeb04c5b@wwwdotorg.org> <20200513104127.GA2309@suse.de> <20200519230326.GA827289@bogus> <20200520085558.GB2136208@ulmo> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200520085558.GB2136208@ulmo> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200526_082904_335939_5B7E8294 X-CRM114-Status: GOOD ( 42.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Herring , arnd@arndb.de, devicetree@vger.kernel.org, gregkh@linuxfoundation.org, Stephen Warren , linux-kernel@vger.kernel.org, jonathanh@nvidia.com, talho@nvidia.com, linux-tegra@vger.kernel.org, robin.murphy@arm.com, afaerber@suse.de, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org T24gV2VkLCBNYXkgMjAsIDIwMjAgYXQgMTA6NTU6NThBTSArMDIwMCwgVGhpZXJyeSBSZWRpbmcg d3JvdGU6Cj4gT24gVHVlLCBNYXkgMTksIDIwMjAgYXQgMDU6MDM6MjZQTSAtMDYwMCwgUm9iIEhl cnJpbmcgd3JvdGU6Cj4gPiBPbiBUdWUsIE1heSAxOSwgMjAyMCBhdCAxMDoxNjo0M0FNIC0wNjAw LCBTdGVwaGVuIFdhcnJlbiB3cm90ZToKPiA+ID4gT24gNS8xMy8yMCA0OjQxIEFNLCBNaWFuIFlv dXNhZiBLYXVrYWIgd3JvdGU6Cj4gPiA+ID4gT24gVHVlLCBNYXkgMTIsIDIwMjAgYXQgMDE6NDU6 MjhQTSAtMDYwMCwgU3RlcGhlbiBXYXJyZW4gd3JvdGU6Cj4gPiA+ID4+IE9uIDUvMTIvMjAgODo0 OCBBTSwgTWlhbiBZb3VzYWYgS2F1a2FiIHdyb3RlOgo+ID4gPiA+Pj4gQWRkIGRvY3VtZW50YXRp b24gZm9yIHRoZSBuZXcgb3B0aW9uYWwgZmxhZyBhZGRlZCBmb3IgU1JBTSBkcml2ZXIuCj4gPiA+ ID4+Cj4gPiA+ID4+PiBkaWZmIC0tZ2l0IGEvRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRp bmdzL3NyYW0vc3JhbS55YW1sIGIvRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL3Ny YW0vc3JhbS55YW1sCj4gPiA+ID4+Cj4gPiA+ID4+PiArICByZXNlcnZlZC1vbmx5Ogo+ID4gPiA+ Pj4gKyAgICBkZXNjcmlwdGlvbjoKPiA+ID4gPj4+ICsgICAgICBUaGUgZmxhZyBpbmRpY2F0aW5n LCB0aGF0IG9ubHkgU1JBTSByZXNlcnZlZCByZWdpb25zIGhhdmUgdG8gYmUgcmVtYXBwZWQuCj4g PiA+ID4+PiArICAgICAgcmVtYXBwaW5nIHR5cGUgaXMgc2VsZWN0ZWQgZGVwZW5kaW5nIHVwb24g bm8tbWVtb3J5LXdjIGFzIHVzdWFsLgo+ID4gPiA+Pj4gKyAgICB0eXBlOiBib29sZWFuCj4gPiA+ ID4+Cj4gPiA+ID4+IFRoaXMgZmVlbHMgYSBiaXQgbGlrZSBhIFNXIGZsYWcgcmF0aGVyIHRoYW4g YSBIVyBkZXNjcmlwdGlvbiwgc28gSSdtIG5vdAo+ID4gPiA+PiBzdXJlIGl0J3MgYXBwcm9wcmlh dGUgdG8gcHV0IGl0IGludG8gRFQuCj4gPiA+ID4gCj4gPiA+ID4gUmVzZXJ2ZWQgcmVnaW9ucyB0 aGVtc2VsdmVzIGFyZSBzb2Z0d2FyZSBkZXNjcmlwdGlvbnMsIG5vPyBUaGVuIHdlIGhhdmUgJ3Bv b2wnCj4gPiA+ID4gZmxhZyB3aGljaCBpcyBhZ2FpbiBhIHNvZnR3YXJlIGZsYWcgYW5kIHNvIG9u LiBUaGlzIGZsYWcgZmFsbHMgaW50byBzYW1lCj4gPiA+ID4gY2F0ZWdvcnkgYW5kIG5vdGhpbmcg b3V0IG9mIG9yZGluYXJ5Lgo+ID4gPiAKPiA+ID4gSSBzdXBwb3NlIHRoYXQncyB0cnVlIHRvIHNv bWUgZXh0ZW50LiBUaGlzIGlzIGluZGVlZCBhIGRlc2NyaXB0aW9uIG9mCj4gPiA+IHRoZSBzeXN0 ZW0gZW52aXJvbm1lbnQgcHJlc2VudGVkIHRvIHRoZSBTVyB0aGF0IGNvbnN1bWVzIHRoZSBEVCwg d2hpY2gKPiA+ID4gaXMgYSBiaXQgbW9yZSB0aGFuIHB1cmUgSFcgZGVzY3JpcHRpb24gYnV0IHN0 aWxsIGEgZGVzY3JpcHRpb24gb2YKPiA+ID4gc29tZXRoaW5nIGltcG9zZWQgZXh0ZXJuYWxseSBy YXRoZXIgdGhhbiBkZXNjcmliaW5nIHNvbWV0aGluZyB0aGF0J3MgdXAKPiA+ID4gdG8gdGhlIGRp c2NyZXRpb24gb2YgdGhlIGNvbnN1bWluZyBTVy4gU28sIGdvIGFoZWFkLgo+ID4gPiAKPiA+ID4g Pj4gQXJlIHRoZXJlIGFueSBjYXNlcyB3aGVyZSB0aGUgU1cgc2hvdWxkIG1hcCBhbGwgb2YgdGhl IFNSQU0sIGkuZS4gd2hlcmUKPiA+ID4gPj4gd2Ugd291bGRuJ3QgZXhwZWN0IHRvIHNldCByZXNl cnZlZC1vbmx5PyBbLi4uXQo+ID4gPiA+IAo+ID4gPiA+IFllcywgaGVyZSBhcmUgYSBmZXcgZXhh bXBsZXM6Cj4gPiA+ID4gYXJjaC9hcm0vYm9vdC9kdHMvYXNwZWVkLWcqLmR0c2kKPiA+ID4gPiBh cmNoL2FybS9ib290L2R0cy9hdDkxKi5kdHNpCj4gPiA+ID4gYXJjaC9hcm0vYm9vdC9kdHMvYmNt NzQ0NS5kdHNpCj4gPiA+ID4gVGhlbiBhcmNoL2FybS9ib290L2R0cy9kcmE3LmR0c2kgaXMgYW4g ZXhhbXBsZSB3aGVyZSB3ZSBzaG91bGQgbWFwIGV2ZXJ5dGhpbmcKPiA+ID4gPiBleGNlcHQgdGhl IHJlc2VydmVkIHJlZ2lvbi4KPiA+ID4gPiAKPiA+ID4gPj4gWy4uLl0gSSdkIGV4cGVjdCByZXNl cnZlZC1vbmx5IHRvIGJlCj4gPiA+ID4+IHRoZSBkZWZhdWx0LCBhbmQgcGVyaGFwcyBvbmx5LCBt b2RlIG9mIG9wZXJhdGlvbiBmb3IgdGhlIFNSQU0gZHJpdmVyLgo+ID4gPiA+IAo+ID4gPiA+IEl0 IHdpbGwgYnJlYWsgY29tcGF0aWJpbGl0eSB3aXRoIGV4aXN0aW5nIGR0YnMuCj4gPiA+ID4gCj4g PiA+ID4+IElmIHdlIGNhbid0IGRvIHRoYXQgYmVjYXVzZSBzb21lIFNXIGN1cnJlbnRseSBleHBl Y3RzIHRvIGJlIGFibGUgdG8gbWFwCj4gPiA+ID4+IGFyYml0cmFyeSBwb3J0aW9ucyBvZiB0aGUg U1JBTSwgc2hvdWxkbid0IHRoYXQgU1cgYmUgZml4ZWQgdG8gdGVsbCB0aGUKPiA+ID4gPj4gU1JB TSBkcml2ZXIgd2hpY2ggcGFydHMgaXQncyB1c2luZywgaGVuY2Ugc3RpbGwgYWxsb3dpbmcgdGhl IGRyaXZlciB0bwo+ID4gPiA+PiBvbmx5IG1hcCBpbi11c2UgcG9ydGlvbnM/Cj4gPiA+ID4gCj4g PiA+ID4gVXNlciBkb2VzbuKAmXQgbmVlZCBzcmFtIGRyaXZlciBpbiB0aGF0IGNhc2UuIEl0IGNh biB1c2UgZ2VuYWxsb2MgYXBpIGRpcmVjdGx5Lgo+ID4gPiAKPiA+ID4gVGhpcyBzb3VuZHMgYSBi aXQgb2RkLiBXaXRob3V0IGEgZHJpdmVyIGZvciB0aGUgcmVzZXJ2ZWQgcmVnaW9uLCBub3RoaW5n Cj4gPiA+IHNob3VsZCBiZSB0b3VjaGluZyBpdCwgc2luY2Ugb3RoZXJ3aXNlIHRoZXJlJ3Mgbm8g Y29kZSB0aGF0IG93bnMgYW4KPiA+ID4gbWFuYWdlcyB0aGUgcmVnaW9uLiBJZiBhbnkgY29kZSBu ZWVkcyB0byBjb25zdW1lIHRoZSByZWdpb24sIGl0IHNob3VsZAo+ID4gPiBvYnRhaW4gaW5mbyBh Ym91dCB0aGUgcmVnaW9uIGZyb20gc29tZSBmb3JtIG9mIHByb3ZpZGVyIGNvZGUgdGhhdCBjYW4K PiA+ID4gaGFuZGxlIGJvdGggdGhlIGFsbG9jYXRpb24gYW5kIG1hcHBpbmcuIEFueXRoaW5nIGVs c2Ugc291bmRzIGxpa2Ugc29tZQo+ID4gPiBjb25zdW1lciBjb2RlIGRpcmVjdGx5IG1ha2luZyB1 c2Ugb2YgRFQgbm9kZXMgaXQgZG9lc24ndCBvd24uIEJ1dCBzaW5jZQo+ID4gPiBJJ20gbm90IGZh bWlsaWFyIGVub3VnaCB3aXRoIHRoZSBTUkFNIGRyaXZlciBhbmQgZ2VuYWxsb2MgY29kZSB0aGF0 IHlvdQo+ID4gPiBtZW50aW9uIHRvIGZ1bGx5IHVuZGVyc3RhbmQgdGhlIGFsbG9jYXRpb24gcGF0 aHMgSSBndWVzcyBJIHdvbid0IG9iamVjdAo+ID4gPiBmb3Igbm93LCBhbHRob3VnaCBpdCBkb2Vz IHN0aWxsIHNvdW5kIGZpc2h5Lgo+ID4gCj4gPiBJJ20gZmluZSB3aXRoIHRoZSBjb25jZXB0LCBi dXQgSSBkb24ndCB0aGluayBhIHNpbmdsZSBmbGFnIGlzIGFkZXF1YXRlLiAKPiA+IElmIHRoZXJl IGFyZSByZXNlcnZlZCByZWdpb25zIHdpdGhpbiB0aGUgU1JBTSwgdGhlbiBkZWZpbmUgY2hpbGQg bm9kZXMgCj4gPiB0byBtYXJrIHRob3NlIHJlZ2lvbnMgcmVzZXJ2ZWQuIEkgZG9uJ3QgdGhpbmsg eW91IG5lZWQgYSBuZXcgZmxhZy4gSnVzdCAKPiA+IGEgJ3JlZycgcHJvcGVydHkgYW5kIG5vdGhp bmcgZWxzZS4KPiAKPiBJdCBzb3VuZHMgdG8gbWUgbGlrZSB0aGVyZSBhcmUgdHdvIGRpZmZlcmVu dCBpbnRlcnByZXRhdGlvbnMgb2YgU1JBTSBhbmQKPiByZXNlcnZlZCByZWdpb25zLiBPbiBvbmUg aGFuZCwgYXMgeW91IHN1Z2dlc3QsIHdlIGhhdmUgb25lIFNSQU0gdGhhdCdzCj4gbWFkZSBhdmFp bGFibGUgYXMgZ2VuYWxsb2MgcG9vbCBhbmQgdGhlbiBpbmRpdmlkdWFsIHJlZ2lvbnMgY2FuIGJl Cj4gbWFya2VkIGFzIHJlc2VydmVkIHNvIHRoYXQgdGhleSBhcmVuJ3QgYWRkZWQgdG8gdGhhdCBw b29sLgo+IAo+IEF0IHRoZSBzYW1lIHRpbWUsIGVhY2ggcmVzZXJ2ZWQgcmVnaW9uIGlzIGFsc28g ZXhwb3NlZCBhcyBhIHNlcGFyYXRlCj4gcG9vbCBhbmQgdGhhdCdzIGluIGZhY3QgdXNlZCBieSBt YW55IGNvbnN1bWVycyBhcyBhIHdheSBvZiBnZXR0aW5nIGEKPiBzcGVjaWZpYyBjaHVuayBvZiB0 aGUgU1JBTSBmb3IgdGhlaXIgb3duIHVzZSAodmlhIHBoYW5kbGUgdG8gdGhlIHJlZ2lvbgo+IGZy b20gdGhlIGNvbnN1bWVyJ3MgZGV2aWNlIHRyZWUgbm9kZSkuCj4gCj4gSW4gYWRkaXRpb24gdG8g dGhhdCwgdGhlIHJlc2VydmVkIHJlZ2lvbiBjb2RlIGRvZXNuJ3QgYWN0dWFsbHkgZnVsbHkgZG8K PiBpdHMgam9iIGJlY2F1c2Ugd2hpbGUgdGhlIHJlc2VydmVkIHJlZ2lvbiBpc24ndCBhY3R1YWxs eSBhZGRlZCB0byB0aGUKPiAidG9wLWxldmVsIiBTUkFNIHBvb2wsIHRoZSBtZW1vcnkgaXMgc3Rp bGwgbWFwcGVkLiBBdCB0aGUgc2FtZSB0aW1lIHRoaXMKPiBpcyBzb21ldGhpbmcgdGhhdCB3ZSBh Y3R1YWxseSB3YW50IGJlY2F1c2UsIGxpa2UgSSBtZW50aW9uZWQsIHNvbWUgb2YKPiB0aGUgY29u c3VtZXJzIGRvIHdhbnQgdG8gZ2V0IGF0IHRoZWlyIFNSQU0gY2h1bmtzIHZpYSByZWZlcmVuY2Vz IHRvIHRoZQo+IHBhcnRpdGlvbnMuCj4gCj4gVGhlIHByb2JsZW0gdGhhdCB0aGlzIHBhdGNoIHNl cmllcyBpcyByZWFsbHkgdHJ5aW5nIHRvIHNvbHZlIGlzIGFub3RoZXIKPiBzdGlsbDogdGhlIGNv bXBsZXRlIFNSQU0gaXMgYWx3YXlzIG1hcHBlZCB0byBrZXJuZWwgbWVtb3J5LCBpcnJlc3BlY3Rp dmUKPiBvZiB3aGV0aGVyIGFueSByZWdpb25zIGFyZSBtYXJrZWQgcmVzZXJ2ZWQgb3Igbm90IGFu ZCB0aGF0IGNhbiBjYXVzZQo+IHNwZWN1bGF0aXZlIGFjY2Vzc2VzIHRvIG1lbW9yeSBvdXRzaWRl IG9mIHRoZSBkZWZpbmVkIHJlZ2lvbnMuCj4gCj4gU3RlcGhlbidzIHN1Z2dlc3Rpb24gaXMgdG8g ZGVmYXVsdCB0byBvbmx5IG1hcHBpbmcgbWVtb3J5IGZvciB3aGljaCBhCj4gcGFydGl0aW9uIGhh cyBiZWVuIGRlZmluZWQgaW4gdGhlIFNSQU0gYW5kIGFzc3VtaW5nIHRoYXQgYWxsIFNSQU0KPiBv dXRzaWRlIG9mIHRob3NlIHBhcnRpdGlvbnMgaXMgb2ZmIGxpbWl0cy4gSSB0aGluayB0aGF0J3Mg YSBzZW5zaWJsZQo+IGRlZmF1bHQgYW5kIGl0J3MgdW5hbWJpZ3VvdXMuCj4gCj4gQnV0IGFzIFlv dXNhZiBwb2ludHMgb3V0IHRoYXQgd291bGQgYnJlYWsgY29tcGF0aWJpbGl0eSB3aXRoIGV4aXN0 aW5nCj4gZGV2aWNlIHRyZWVzLiBEZXBlbmRpbmcgb24gaG93IHlvdSBpbnRlcnByZXQgdGhlIGJp bmRpbmdzIG9uZSBjb3VsZAo+IGFyZ3VlIHRoYXQgdGhvc2UgZGV2aWNlIHRyZWVzIGFyZSBidWdn eSBhbmQgc2hvdWxkIGhhdmUgcGFydGl0aW9ucwo+IGRlZmluZWQgKGluIHRoZSBjYXNlcyBJJ3Zl IGxvb2tlZCBhdCB0aGV5IGVuZCB1cCB1c2luZyBhIGZpeGVkIHJlZ2lvbgo+IGFueXdheSwgc28g dGhhdCBjb3VsZCd2ZSBqdXN0IGJlZW4gbWFkZSBleHBsaWNpdCBpbiB0aGUgZGV2aWNlIHRyZWUp Lgo+IAo+IEhvd2V2ZXIsIGl0IGFsc28gbG9va3MgbGlrZSBhbGwgb2YgdGhlIHVzZXJzIHRoYXQg cmVseSBvbiB0aGUgb3JpZ2luYWwKPiBiZWhhdmlvdXIgd2hlcmUgdGhleSBjYW4ganVzdCBhY2Nl c3MgdGhlIGZ1bGwgcG9vbCBhcmUgdGhvc2UgdGhhdCBkb24ndAo+IGRlZmluZSBhbnkgcmVzZXJ2 ZWQgcmVnaW9ucywgd2hlcmVhcyBhbGwgdXNlcnMgdGhhdCBkbyByZXNlcnZlIHJlZ2lvbnMKPiB3 aWxsIGFjdHVhbGx5IHVzZSB0aG9zZSByZXNlcnZlZCByZWdpb25zLgo+IAo+IFNvIEkgdGhpbmsg d2UgY2FuIG1ha2UgdXNlIG9mIHRoaXMgYnkgZGlmZmVyZW50aWF0aW5nIGluIHRoZSBkcml2ZXIK PiBiZXR3ZWVuIFNSQU0gbm9kZXMgd2l0aCBvciB3aXRob3V0IGNoaWxkcmVuIGFuZCBjaGFuZ2Ug dGhlIGJlaGF2aW91cgo+IGFjY29yZGluZ2x5LiBJIHRoaW5rIHRoYXQgaGFzIHRoZSBiaWcgYWR2 YW50YWdlIHRoYXQgaXQgbWFrZXMgdGhpbmdzCj4gd29yayBhcyAoSSB0aGluaykgbW9zdCBwZW9w bGUgd291bGQgZXhwZWN0IGFuZCBkb2Vzbid0IGZ1cnRoZXIKPiBjb21wbGljYXRlIHRoZSBiaW5k aW5nIHdpdGggZXh0cmEgZmxhZ3MuCgpJIHRlbmQgdG8gYWdyZWUgb24gbWFwcGluZyBwYXJ0aXRp b25zIG9ubHkgaWYgdGhleSBleGlzdC4gU28gZmFyIEkgY291bGQKb25seSBmaW5kIG9uZSBleGNl cHRpb24uIEl0IGlzIGFyY2gvYXJtL2Jvb3QvZHRzL2FybWFkYS0zNzAuZHRzaSB3aGljaCBpcwp1 c2luZyB0aGUgdG9wIGxldmVsIHBvb2wgYXMgd2VsbCBhcyBhIHBhcnRpdGlvbiB0byByZXNlcnZl IDMyIGJ5dGVzIGF0IHRoZQpib3R0b20gb2Ygc3JhbS4gVGhpcyBjYW4gYmUgZml4ZWQgYWxvbmcg d2l0aCB0aGUgc3JhbSBkcml2ZXIgY2hhbmdlLCBieQphZGRpbmcgYW5vdGhlciBwYXJ0aXRpb24g Zm9yIHRoZSByZXN0IG9mIHRoZSBzcmFtIGFuZCB1c2luZyBpdHMgaGFuZGxlIGluCnRoZSBjcnlw dG9AOTAwMDAgaW5zdGVhZCBvZiB0b3AtbGV2ZWwgc3JhbSBub2RlIGhhbmRsZS4gRG8geW91IHNl ZSBhbnltb3JlCmV4Y2VwdGlvbnMgd2hlcmUgYm90aCB0b3AgbGV2ZWwgcG9vbCBhbmQgdGhlIHBh cnRpdGlvbnMgYm90aCBhcmUgYmVpbmcgdXNlZD8KClRoZW4gb24gdGhlIGJhY2t3YXJkIGNvbXBh dGliaWxpdHkgdG9waWMsIGFub3RoZXIgaXNzdWUgaXMgdGhhdCBib290IGNvZGUKY291bGQgYWRk IHNyYW0gbm9kZXMgZHluYW1pY2FsbHkuIEZvciBleGFtcGxlIGFyY2gvYXJtL21hY2gtazMvY29t bW9uLmMgaW4KdS1ib290IGRvZXMgaXQuIFRoaXMgcGFydGljdWxhciBjYXNlIHdpbGwgbm90IGJy ZWFrIGFmdGVyIHRoZSBzdWdnZXN0ZWQgY2hhbmdlCmJlY2F1c2UgaXQgaXMgbm90IGFkZGluZyBh bnkgcGFydGl0aW9ucy4gSG93ZXZlciwgdGhlcmUgY291bGQgYmUgb3RoZXIKYm9vdC1sb2FkZXJz IHdoaWNoIGFyZSBub3QgdGhpcyBsdWNreS4KCj4gCj4gVGhpZXJyeQoKL1lvdXNhZgoKCl9fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJu ZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRw Oi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK 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=-3.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 783DFC433E1 for ; Tue, 26 May 2020 15:29:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5DDD120723 for ; Tue, 26 May 2020 15:29:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729330AbgEZP26 (ORCPT ); Tue, 26 May 2020 11:28:58 -0400 Received: from mx2.suse.de ([195.135.220.15]:44664 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728815AbgEZP26 (ORCPT ); Tue, 26 May 2020 11:28:58 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 3E94BB260; Tue, 26 May 2020 15:28:58 +0000 (UTC) Date: Tue, 26 May 2020 17:28:47 +0200 From: Mian Yousaf Kaukab To: Thierry Reding Cc: Rob Herring , Stephen Warren , robin.murphy@arm.com, devicetree@vger.kernel.org, talho@nvidia.com, jonathanh@nvidia.com, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, afaerber@suse.de, arnd@arndb.de, gregkh@linuxfoundation.org Subject: Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag Message-ID: <20200526152847.GA16107@suse.de> References: <20200512144803.24344-1-ykaukab@suse.de> <20200512144803.24344-2-ykaukab@suse.de> <52f099e4-5c03-2141-f049-cd3adeb04c5b@wwwdotorg.org> <20200513104127.GA2309@suse.de> <20200519230326.GA827289@bogus> <20200520085558.GB2136208@ulmo> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200520085558.GB2136208@ulmo> Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Wed, May 20, 2020 at 10:55:58AM +0200, Thierry Reding wrote: > On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote: > > On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote: > > > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote: > > > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote: > > > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote: > > > >>> Add documentation for the new optional flag added for SRAM driver. > > > >> > > > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml > > > >> > > > >>> + reserved-only: > > > >>> + description: > > > >>> + The flag indicating, that only SRAM reserved regions have to be remapped. > > > >>> + remapping type is selected depending upon no-memory-wc as usual. > > > >>> + type: boolean > > > >> > > > >> This feels a bit like a SW flag rather than a HW description, so I'm not > > > >> sure it's appropriate to put it into DT. > > > > > > > > Reserved regions themselves are software descriptions, no? Then we have 'pool' > > > > flag which is again a software flag and so on. This flag falls into same > > > > category and nothing out of ordinary. > > > > > > I suppose that's true to some extent. This is indeed a description of > > > the system environment presented to the SW that consumes the DT, which > > > is a bit more than pure HW description but still a description of > > > something imposed externally rather than describing something that's up > > > to the discretion of the consuming SW. So, go ahead. > > > > > > >> Are there any cases where the SW should map all of the SRAM, i.e. where > > > >> we wouldn't expect to set reserved-only? [...] > > > > > > > > Yes, here are a few examples: > > > > arch/arm/boot/dts/aspeed-g*.dtsi > > > > arch/arm/boot/dts/at91*.dtsi > > > > arch/arm/boot/dts/bcm7445.dtsi > > > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything > > > > except the reserved region. > > > > > > > >> [...] I'd expect reserved-only to be > > > >> the default, and perhaps only, mode of operation for the SRAM driver. > > > > > > > > It will break compatibility with existing dtbs. > > > > > > > >> If we can't do that because some SW currently expects to be able to map > > > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the > > > >> SRAM driver which parts it's using, hence still allowing the driver to > > > >> only map in-use portions? > > > > > > > > User doesn’t need sram driver in that case. It can use genalloc api directly. > > > > > > This sounds a bit odd. Without a driver for the reserved region, nothing > > > should be touching it, since otherwise there's no code that owns an > > > manages the region. If any code needs to consume the region, it should > > > obtain info about the region from some form of provider code that can > > > handle both the allocation and mapping. Anything else sounds like some > > > consumer code directly making use of DT nodes it doesn't own. But since > > > I'm not familiar enough with the SRAM driver and genalloc code that you > > > mention to fully understand the allocation paths I guess I won't object > > > for now, although it does still sound fishy. > > > > I'm fine with the concept, but I don't think a single flag is adequate. > > If there are reserved regions within the SRAM, then define child nodes > > to mark those regions reserved. I don't think you need a new flag. Just > > a 'reg' property and nothing else. > > It sounds to me like there are two different interpretations of SRAM and > reserved regions. On one hand, as you suggest, we have one SRAM that's > made available as genalloc pool and then individual regions can be > marked as reserved so that they aren't added to that pool. > > At the same time, each reserved region is also exposed as a separate > pool and that's in fact used by many consumers as a way of getting a > specific chunk of the SRAM for their own use (via phandle to the region > from the consumer's device tree node). > > In addition to that, the reserved region code doesn't actually fully do > its job because while the reserved region isn't actually added to the > "top-level" SRAM pool, the memory is still mapped. At the same time this > is something that we actually want because, like I mentioned, some of > the consumers do want to get at their SRAM chunks via references to the > partitions. > > The problem that this patch series is really trying to solve is another > still: the complete SRAM is always mapped to kernel memory, irrespective > of whether any regions are marked reserved or not and that can cause > speculative accesses to memory outside of the defined regions. > > Stephen's suggestion is to default to only mapping memory for which a > partition has been defined in the SRAM and assuming that all SRAM > outside of those partitions is off limits. I think that's a sensible > default and it's unambiguous. > > But as Yousaf points out that would break compatibility with existing > device trees. Depending on how you interpret the bindings one could > argue that those device trees are buggy and should have partitions > defined (in the cases I've looked at they end up using a fixed region > anyway, so that could've just been made explicit in the device tree). > > However, it also looks like all of the users that rely on the original > behaviour where they can just access the full pool are those that don't > define any reserved regions, whereas all users that do reserve regions > will actually use those reserved regions. > > So I think we can make use of this by differentiating in the driver > between SRAM nodes with or without children and change the behaviour > accordingly. I think that has the big advantage that it makes things > work as (I think) most people would expect and doesn't further > complicate the binding with extra flags. I tend to agree on mapping partitions only if they exist. So far I could only find one exception. It is arch/arm/boot/dts/armada-370.dtsi which is using the top level pool as well as a partition to reserve 32 bytes at the bottom of sram. This can be fixed along with the sram driver change, by adding another partition for the rest of the sram and using its handle in the crypto@90000 instead of top-level sram node handle. Do you see anymore exceptions where both top level pool and the partitions both are being used? Then on the backward compatibility topic, another issue is that boot code could add sram nodes dynamically. For example arch/arm/mach-k3/common.c in u-boot does it. This particular case will not break after the suggested change because it is not adding any partitions. However, there could be other boot-loaders which are not this lucky. > > Thierry /Yousaf