linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: stefan@agner.ch (Stefan Agner)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] pci: imx6: support kernels built in Thumb-2 mode
Date: Wed, 28 Nov 2018 18:53:27 +0100	[thread overview]
Message-ID: <074f80db0eceb76672bca283c5de37b9@agner.ch> (raw)
In-Reply-To: <ff476784-b79e-18bf-5870-4365c8377deb@arm.com>

On 28.11.2018 17:16, Robin Murphy wrote:
> Hi Stefan,
> 
> On 28/11/2018 13:25, Stefan Agner wrote:
>> Add a fault handler which handles reads in Thumb-2 mode. Install
>> the appropriate handler depending on which mode the kernel has
>> been built. This avoids an "Unhandled fault: external abort on
>> non-linefetch (0x1008) at 0xf0a80000" during boot on a device
>> with a PCIe switch connected.
>>
>> Link: https://lore.kernel.org/linux-pci/20181126161645.8177-1-stefan at agner.ch/
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> FWIW, I found this manual helpful to write the code below:
>> http://hermes.wings.cs.wisc.edu/files/Thumb-2SupplementReferenceManual.pdf#page=43&zoom=100,0,66
> 
> This one's rather less ancient and even more authoritative ;)
> 
> https://static.docs.arm.com/ddi0406/cd/DDI0406C_d_armv7ar_arm.pdf
> 
> (ARMv7 had a few new encodings over and above ARMv6T2, although in
> fairness I don't think any should be relevant to this specific case)
> 

Thanks, I tried to find the right document on arm.com, but I timed out
after 5 minutes or so :-)

>> --
>> Stefan
>>
>>   drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++++++++++++++++++-
>>   1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
>> index 69f86234f7c0..683deb74d69f 100644
>> --- a/drivers/pci/controller/dwc/pci-imx6.c
>> +++ b/drivers/pci/controller/dwc/pci-imx6.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/reset.h>
>>   #include <linux/pm_domain.h>
>>   #include <linux/pm_runtime.h>
>> +#include <asm/opcodes.h>
>>     #include "pcie-designware.h"
>>   @@ -299,6 +300,37 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
>>   	return 1;
>>   }
>>   +static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
>> +		unsigned int fsr, struct pt_regs *regs)
>> +{
>> +	unsigned long pc = instruction_pointer(regs);
>> +	unsigned long instr = *(unsigned long *)pc;
>> +	unsigned long thumb2_instr = __mem_to_opcode_thumb16(instr);
>> +	int reg = thumb2_instr & 7;
>> +
>> +	if (!__opcode_is_thumb16(instr & 0x0000ffffUL))
>> +		return 1;
> 
> There are plenty of 32-bit Thumb encodings of various LDR/STR
> variants, and I doubt we can guarantee that the offset, target
> register, and/or addressing mode for a config space access will
> *always* suit the (relatively limited) 16-bit ones.

Hm, I guess they should be handled too?

I looked at the code where I had the abort at hand triggered
(dw_pcie_read).
 
>> +
>> +	/* Load word/byte and halfword immediate offset */
>> +	if (((thumb2_instr & 0xe800) == 0x6800) ||
>> +	    ((thumb2_instr & 0xf800) == 0x8800)) {
>> +		unsigned long val;
>> +
>> +		if (thumb2_instr & 0x1000)
>> +			val = 0xff;
>> +		else if (thumb2_instr & 0x8000)
>> +			val = 0xffff;
>> +		else
>> +			val = 0xffffffffUL;
>> +
>> +		regs->uregs[reg] = val;
>> +		regs->ARM_pc += 2;
>> +		return 0;
>> +	}
> 
> What about stores? The existing implementation handles them, so either
> that's dead code which could perhaps be cleaned up, or they need to be
> handled here too.
> 

I think the current handler (imx6q_pcie_abort_handler) checks bit 20,
which means it must be a load not?

>> +
>> +	return 1;
>> +}
>> +
>>   static int imx6_pcie_attach_pd(struct device *dev)
>>   {
>>   	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
>> @@ -1069,6 +1101,8 @@ static struct platform_driver imx6_pcie_driver = {
>>     static int __init imx6_pcie_init(void)
>>   {
>> +	bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
> 
> Can these aborts definitely *only* be triggered by kernel accesses,
> and never, say, via an mmap() of anything exposed to userspace?

Honestly, I am not very familiar with PCIe, I don't know...

--
Stefan

> 
> Robin.
> 
>> +
>>   	/*
>>   	 * Since probe() can be deferred we need to make sure that
>>   	 * hook_fault_code is not called after __init memory is freed
>> @@ -1076,7 +1110,8 @@ static int __init imx6_pcie_init(void)
>>   	 * we can install the handler here without risking it
>>   	 * accessing some uninitialized driver state.
>>   	 */
>> -	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
>> +	hook_fault_code(8, thumb2 ? imx6q_pcie_abort_handler_thumb2 :
>> +			imx6q_pcie_abort_handler, SIGBUS, 0,
>>   			"external abort on non-linefetch");
>>     	return platform_driver_register(&imx6_pcie_driver);
>>

  reply	other threads:[~2018-11-28 17:53 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-28 13:25 [PATCH] pci: imx6: support kernels built in Thumb-2 mode Stefan Agner
2018-11-28 16:16 ` Robin Murphy
2018-11-28 17:53   ` Stefan Agner [this message]
2018-11-28 19:35     ` Robin Murphy
2018-11-29  9:54       ` Stefan Agner
2018-11-28 18:56   ` Trent Piepho
2018-11-28 19:52     ` Robin Murphy
2018-11-28 20:00       ` Trent Piepho
2018-11-28 18:01 ` Russell King - ARM Linux
2018-11-29  9:47   ` Stefan Agner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=074f80db0eceb76672bca283c5de37b9@agner.ch \
    --to=stefan@agner.ch \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).