Re: [PATCH v3 2/2] PCI: rcar: Return all Fs from read which triggered an exception

From: Marek Vasut <marek.vasut@gmail.com>
To: "Pali Rohár" <pali@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>
Cc: linux-pci@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Wolfram Sang <wsa@the-dreams.de>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v3 2/2] PCI: rcar: Return all Fs from read which triggered an exception
Date: Thu, 17 Feb 2022 04:24:24 +0100	[thread overview]
Message-ID: <912a2a2a-b8f6-6819-8974-ab2db2cf0f77@gmail.com> (raw)
In-Reply-To: <20220216115044.kslb47pup4dsukgh@pali>

On 2/16/22 12:50, Pali Rohár wrote:

Hi,

>> In case the controller is transitioning to L1 in rcar_pcie_config_access(),
>> any read/write access to PCIECDR triggers asynchronous external abort. This
>> is because the transition to L1 link state must be manually finished by the
>> driver. The PCIe IP can transition back from L1 state to L0 on its own.
>>
>> The current asynchronous external abort hook implementation restarts
>> the instruction which finally triggered the fault, which can be a
>> different instruction than the read/write instruction which started
>> the faulting access. Usually the instruction which finally triggers
>> the fault is one which has some data dependency on the result of the
>> read/write. In case of read, the read value after fixup is undefined,
>> while a read value of faulting read should be all Fs.
>>
>> It is possible to enforce the fault using 'isb' instruction placed
>> right after the read/write instruction which started the faulting
>> access. Add custom register accessors which perform the read/write
>> followed immediately by 'isb'.
>>
>> This way, the fault always happens on the 'isb' and in case of read,
>> which is located one instruction before the 'isb', it is now possible
>> to fix up the return value of the read in the asynchronous external
>> abort hook and make that read return all Fs.
> 
> I'm looking at this again and I do not think that this is reliable.

Are you still running into any problems on your hardware with these 
patches applied ?

> Asynchronous aborts are by definition asynchronous. Placing isb looks
> like a hack to decrease probability that asynchronous abort would be
> triggered at wrong time.

That is exactly what this patch fixes, the ISB enforces the async 
exception at the right moment so it can be fixed up in the fixup handler 
(thanks Arnd).

> Marek: Cannot you change the code to trigger proper synchronous abort
> for this operation? If this is ARMv7 system, what about trying to change
> memory mapping where is the accessing address to strongly-ordered?
> Writing to strongly-ordered ARMv7 mapping could not report asynchronous
> aborts anymore, but I'm not sure.

No, last time I tried tweaking the mapping, that didn't lead to sync aborts.

> Marek: Are you sure that also ldr instruction is causing asynchronous
> abort? This is strange as normally load from memory mapped config space
> could not finish earlier than data from config space are fetched.
> Normally these load instructions should cause synchronous abort on data
> errors.

I am positive LDR triggers async abort on this hardware, since that's 
how this problem was triggered by Geert.

[...]