Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Subject: Re: [PATCH] arm64: head: Fix cache inconsistency of the
 identity-mapped region
To:     Will Deacon <will@kernel.org>
Cc:     Marc Zyngier <maz@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Mark Rutland <mark.rutland@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Ard Biesheuvel <ardb@kernel.org>,
        Vikram Sethi <vsethi@nvidia.com>,
        Thierry Reding <treding@nvidia.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>
References: <20220415170504.3781878-1-sdonthineni@nvidia.com>
 <87y202agz9.wl-maz@kernel.org>
 <6b08ece9-64c0-9de9-9876-ed2dceee9bb0@nvidia.com>
 <20220420100801.GA7235@willie-the-truck>
 <f1b32237-f178-0656-b3ee-814eec4edb71@nvidia.com>
 <20220421114719.GA8557@willie-the-truck>
From:   Shanker R Donthineni <sdonthineni@nvidia.com>
Message-ID: <873ccd61-34c1-36be-f77a-52c3db860bf9@nvidia.com>
Date:   Thu, 5 May 2022 06:42:45 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <20220421114719.GA8557@willie-the-truck>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ek9KRVlRVUtQL090eEFwV2I0NXViczk2dVJaTGZPWEl1NVFmVWZZUmVobnlO?=
 =?utf-8?B?RXRLMjZLNlJGc1liWGVUa29IcFBzQ3Q4V2UrSG05bEhKQXJZaVEzQWFaTXUw?=
 =?utf-8?B?MExxOU1WMXdUbE1sRktEZTc5VEt2WHl1R3JPNkdpM2owYTBLWkRtWEUxM09T?=
 =?utf-8?B?dzRad29RV0FkUklhK2pQRk03RjJESXNrVXNIQ3N0L1EvaklqVkZja0FudEVZ?=
 =?utf-8?B?a0Yrdnh4NU11V0o1dCt6bllBOGhiK3NUanJrcVorbWFEOHMwcVBxelpzWm9L?=
 =?utf-8?B?VjA4R0ZuWjRrN1cxb2pJZ3lnNXM1b2VVb1J0d3hkcmNsVWVubVovSjdRdjN5?=
 =?utf-8?B?Z1ZVdWtwd2ZjNHJVNnJ6ZjNCZ28zbS93TkV3QlA2Qm0zNVJDUmFQTXRDanYy?=
 =?utf-8?B?REswcGZJV2R2YTA3bCtXUzR5VlBlZ2pwMHpXM3QxOEFVd3VYVUdJWEpsbHpF?=
 =?utf-8?B?QzJ5Vk9TVnhjckFIWlNRMDhwSzJ2UndKSVlualUrdHNiWXBDMi9sc0F6aldK?=
 =?utf-8?B?WVI4YjRpSkFHRzB0N1BpYTNES1BrWkcwczlJcVhYK1ZhYVdydW1jQmp2VlRv?=
 =?utf-8?B?MlZwYSs2UXcyRngrYitibEhEVGFhWjcvR0FaYjFaYXFjUUJPQXJYVVlKdERv?=
 =?utf-8?B?ZWFuTTduQjJzTm1tZXZObmNlN1FYejZ6TEw0MCtoUnpsckhoWkdhWVQzK2xJ?=
 =?utf-8?B?QXlMRnV4SGgxWGgreG5tY2U5aUdRRlpXOGxtVkxobXNjcG8xRmRuUUJWbmhM?=
 =?utf-8?B?S1o5ZkZ3QW83R2Z5UU9DZ1lUQlVwNUJFUzlydVQ3VnpQUEJWMGxJeFl4aWhN?=
 =?utf-8?B?YmJUTDYzOFhROVZvY0F2aVNRd0ozMGlBK2txMmRvMkl6NWFBQVNpZzRrQ2Zs?=
 =?utf-8?B?eDl6eEhicTdyZ2RyaEZ1OWozR1cwa0Z0cGRsRDdYUlR5Qkw1Z0FuSkFvNWNa?=
 =?utf-8?B?SU0rSGFCSEN4ZU1QKytGZER5T3pVSE9zWi84aHdEWGJCcXRlbE5DMWw1QU4x?=
 =?utf-8?B?R0FvQit5azZzWWc2ZjYySTBaNGhCL1dKWGtrd2pkR3NHMjMwWUpzS1JrU1RO?=
 =?utf-8?B?T1ZOdTF1bzZpajRuWlhPMVZKaU13bmRFUUduY3NLT2dQbVdvWmJoSTBiTkNF?=
 =?utf-8?B?M1daYWkrK2pDdStHZDltOE9xMDQ2Nk5PUERrWldwWmc4OEdHOWdyTG84UUNx?=
 =?utf-8?B?RWN0Wk1SNHc5V3U1NHZnbU9nT2R1UURzc3NDajA3THE4TUt0ZlhLSjJacDlG?=
 =?utf-8?B?Q1lXMUFPQjJDT2w2RjI5L2tGSVpjb2lBMDVZQXF0MzZHSVFJd3htT0tuSTI0?=
 =?utf-8?B?VkdFbWZxWjc2cXlvMTJoWjMrNE80ZWM2M2JZSW9LVmE5c1VvNzVyTXM0aGdL?=
 =?utf-8?B?YjFIZnhGZWY3T21CNGd3Tm1kNEJlWnIrMW54TEV2dFNZWm1tTmJmN3d3Vmxs?=
 =?utf-8?B?dk1hazlleGg3eGx4NThkSkNTSmQ0eTRWL1R6SmNqTUFJQm5WSVFUOTI5UWFr?=
 =?utf-8?B?aGZzTGtva3JOZmRBb0N0WTJKdEV0dS9pUFF5VDBndHAxZmNwdFB6L0RlOXlJ?=
 =?utf-8?B?SERsSXBVb2JCRUthVWVzNmN6VWc2TXJZcHhody9FMUxpRGlMNE00M3IvTHJ6?=
 =?utf-8?B?dCtvN1oxeG11OFA1c1l1Wk1zZ0FCbUpXbVhxN01lM0VYYllLa1E2NnhzT3p2?=
 =?utf-8?B?MEllUkY5TDJJTlpCKzNwQ1VhaXQvWms5bFlqNGwrL3phNDVlc1dNYnVrVUhG?=
 =?utf-8?B?MENjSmpwR3F6RlM0Rk1RdEpGS0d4WFozcjVscTMxY3FXSVgvbFhUL3AyczVE?=
 =?utf-8?B?b05jQ09GNkFVdnpjTGpRTmxkZG9ZbWxJa3U3M0hVR1VEakROT0Qzd1BPaEtm?=
 =?utf-8?B?aHdZdUZlT01TRUZkK2F6L0JybG9uQk0weGZsVUJ0RTI0dUR0R2FnSytJTDYy?=
 =?utf-8?B?cFhVd2lqajhuTkpFOFZDejc0WDd2OFZ5QThFSURBaE1GSVAzVXV3N3VYS3Q1?=
 =?utf-8?B?bERrS0k1cy9OU3pvemVMVkZGSUxMR0JDc2JGZTZoVmdueE5NZlJsTCtiN0dH?=
 =?utf-8?B?MGpkdFpWYlBDekNSVGh4dFJCb25BRmlSMkI4VEl0ejl0ZngyTUJwaUxnV1Vo?=
 =?utf-8?B?dDZsV2JDRjJGWW1aVDhmcFdtYkVDcmFGNmU4VTdUZC9LMk5wVnJONkN2Q1ZN?=
 =?utf-8?B?b1ZQMXlJam5KTGZXaHB1MlRGZUdOMzBoOWFNZ25Lb3V3V1ZqUzRRMHhVWFdF?=
 =?utf-8?B?d0hvZU16Mis2Y2JsOU8wM0RETkVQbFNvdTd6cWs5enB0TUVBTllBdXkvaThC?=
 =?utf-8?B?Wmg1UDlvU0hXMC84d1hRSVBSWEluM09ObGQvTkJ4RU8zc3djWFpxQT09?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 72aaee0d-5895-4ae3-c1b6-08da2e8c55b3
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2751.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2022 11:42:29.6833
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Gvbqm98mulOMRmbmeHWnJbBqdPI8/W1RHP+1wp4Kax2MNF5vUGVG4BOK1HBa9rvFmHqOUItTN2TkU7Ot3C15KA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1400
X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Will,

On 4/21/22 6:47 AM, Will Deacon wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Shanker,
>
> First of all, thanks for continuing to share so many useful details here.
> I'm pretty confident we'll eventually get to the bottom of this.
>
> On Wed, Apr 20, 2022 at 10:02:13AM -0500, Shanker R Donthineni wrote:
>> On 4/20/22 5:08 AM, Will Deacon wrote:
>>> Non-coherent? You mean non-cacheable, right? At this stage, we only
>>>>> have a single CPU, so I'm not sure coherency is the problem here. When
>>>>> you say 'drop', is that an eviction? Replaced by what? By the previous
>>>>> version of the cache line, containing the stale value?
>>>> Yes,non-cacheable. The cache line corresponding to function enter_vhe() was
>>>> marked with shareable & WB-cache as a result of write to RELA, the same cache
>>>> line is being fetched with non-shareable & non-cacheable. The eviction is
>>>> not pushed to PoC and got dropped because of cache-line attributes mismatch.
>>> I'm really struggling to understand this. Why is the instruction fetch
>>> non-shareable? I'm trying to align your observations with the rules about
>>> mismatched aliases in the architecture and I'm yet to satisfy myself that
>>> the CPU is allowed to drop a dirty line on the floor in response to an
>>> unexpected hit.
>>>
>>> My mental model (which seems to align with Marc) is something like:
>>>
>>>
>>> 1. The boot CPU fetches the line via a cacheable mapping and dirties it
>>>    in its L1 as part of applying the relocations.
>> ARM-CPU core is sending ReadNotSharedDirty CHI command to LLC (last-level-cache).
>> This cache-line is marked as checked-out in LLC, would be used to keep track
>> of coherency.
> Oh man, that CHI stuff always sends me into a spin so I'm probably not the
> right person to debug it, but let's see...
>
>>> 2. The boot CPU then enters EL2 with the MMU off and fetches the same
>>>    line on the I-side. AFAICT, the architecture says this is done with
>>>    outer-shareable, non-cacheable attributes.
>> ARM-CPU core is sending ReadNoSnoop CHI command when MMU disabled. The
>> marked cache-line from the step 1) become invalid in LLC.
> When you say "invalid" here, do you mean in the same sense as "cache
> invalidation"? Why is that the right thing to do in response to a _read_?
>
> Would the same invalidation happen if the ReadNoSnoop originated from a
> different CPU?
>
There is no issue if other cores are issuing ReadNoSnoop commands.

>> As per the ARM-ARM specification, CMO is recommended whenever memory
>> attributes are changed for a given memory region.
> The mismatched attribute section (B2.8) is quite a lot more nuanced than
> that, and in particular I don't see it allowing for writes to be lost
> in the presence of a mismatched read. Then again, the whole section is
> hard to follow and doesn't really call out instruction fetch other than
> in a note at the end.
>
>> With my understating the CPU core must generate coherent accesses for
>> Shared+Cacheable memory but not clear for OSH+non-cacheable regions
>> in the spec.
> Ok, but we don't need the reloc to be visible to the MMU-off fetch in
> the case you're describing so coherency between the two aliases is not
> required. The problem is that the cacheable data is being lost forever,
> so even cacheable accesses can't retrieve it.
>
>> Are you expecting "OSH+non-cacheable" must generate coherent accesses?
>>
>>> 3. !!! Somehow the instruction fetch results in the _dirty_ line from (1)
>>>        being discarded !!!
>> The ARM-CPU is sending WritebackFull CHI command to LLC for the cache line
>> which was marked as invalid from step 2). The write CMD is ignored/dropped.
> Is this because the LLC is inclusive and doesn't know what to do with the
> writeback after invalidating the line earlier on? When else would a
> writeback get silently dropped?
>
> Stepping back a bit, my concern with all of this is that it seems as though
> non-cacheable instruction fetches that alias with dirty lines on the data
> side can lead to data loss. This feels like a significantly broader issue
> than the relocation case you've run into, particularly as instruction fetch
> with the MMU off is still permitted to fetch speculatively from within a
> couple of pages of the PC. Cache maintenance doesn't feel like a viable
> solution in general, as we'd need to ensure that no fetch occurs between
> the write and the CMO, and this could occur on another CPU, off the back
> of an IRQ or even a FIQ (with the fetch being made at a different exception
> level).
>
> Given that you have all the low-level details about the problem, it would
> probably be worth pulling Arm into this so we can figure out what the right
> solution is. I'm happy to be involved with that, if you like.
>
>
Thanks Will for your valuable comments on dropping valid cache lines. I've
forwarded your responses to hardware team, reviewed CHI and ARM-ARM
specification one more time. Our CPU team was agreed to fix in hardware
convert ReadNoSnoop to ReadSnoop command (step-2).

Thanks,
Shanker Donthineni