Re: [PATCH] pci: imx6: support kernels built in Thumb-2 mode

[Stefan Agner]

On 28.11.2018 19:01, Russell King - ARM Linux wrote:
> On Wed, Nov 28, 2018 at 02:25:54PM +0100, 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@xxxxxxxx/
>> Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
>> ---
>> 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
>>
>> --
>> 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;
> 
> So what happens if userspace mmap()s the PCIe space (eg, via
> /dev/mem), and then accesses it, triggering this fault?  You'll
> be attempting to read from userspace here, which will oops the
> kernel.  The kernel is not allowed to access userspace by
> simply dereferencing a pointer.
> 

Note that imx6q_pcie_abort_handler above does the same for ARM code. I
guess a quick fix for now would be just using user_mode before
dereferencing the pc:

	if (user_mode(regs))
		return 1;

Is userspace mmap'ing PCIe space common?

If we want to support user space, I guess we anyway should merge the ARM
and Thumb2 handler, and detect & act accordingly.

--
Stefan

>> +	unsigned long thumb2_instr = __mem_to_opcode_thumb16(instr);
>> +	int reg = thumb2_instr & 7;
>> +
>> +	if (!__opcode_is_thumb16(instr & 0x0000ffffUL))
>> +		return 1;
>> +
>> +	/* 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;
>> +	}
>> +
>> +	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);
>> +
>>  	/*
>>  	 * 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);
>> --
>> 2.19.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel