diff mbox series

[v2,2/2] PCI: rcar: Return all Fs from read which triggered an exception

Message ID 20220117220355.92575-2-marek.vasut@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/2] PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access() | expand

Commit Message

Marek Vasut Jan. 17, 2022, 10:03 p.m. UTC
From: Marek Vasut <marek.vasut+renesas@gmail.com>

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.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Krzysztof Wilczyński <kw@linux.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-renesas-soc@vger.kernel.org
---
V2: Rebase on 1/2
---
 drivers/pci/controller/pcie-rcar-host.c | 67 ++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann Jan. 17, 2022, 11:38 p.m. UTC | #1
On Mon, Jan 17, 2022 at 11:03 PM <marek.vasut@gmail.com> wrote:
> 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.

Hi Marek,

As mentioned on IRC, I think this can be done a lot simpler, using a .text.fixup
section hack:
> +void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
> +{
> +#ifdef CONFIG_ARM
> +       asm volatile(
> +               "       str %0, [%1]\n"
> +               "       isb\n"
> +       ::"r"(val), "r"(pcie->base + reg):"memory");


I think this would looks something like

   int error = 0;
   asm volatile(
        "       str %1, [%2]\n"
        "1:       isb\n"
        "2:\n"
        "         pushsection .text.fixup,\"ax\"\n"
        "       .align  2\n"                                    \
        "3:     mov     %0, %3\n"                               \
        "       b       2b\n"                                   \
        "       .popsection\n"                                  \
        "       .pushsection __ex_table,\"a\"\n"                \
        "       .align  3\n"                                    \
        "       .long   1b, 3b\n"                               \
        "       .popsection"                                    \
       : "+r" (error) :"r"(val), "r"(pcie->base + reg), "i" (-ENXIO):"memory");

This saves you from hand-parsing the instruction sequence, which tends
to be even more fragile. After this, you just need to check the
'error' variable,
which remains at 0 normally but contains -ENXIO if an exception hits.

I'm not entirely sure this works for the particular exception you are getting,
and it probably requires not registering the rcar_pcie_aarch32_abort_handler
function, but it seems likely to work.

        Arnd
Bjorn Helgaas Jan. 18, 2022, 4:13 p.m. UTC | #2
On Mon, Jan 17, 2022 at 11:03:55PM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> 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.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Krzysztof Wilczyński <kw@linux.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: Rebase on 1/2
> ---
>  drivers/pci/controller/pcie-rcar-host.c | 67 ++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> index f0a0d560fefc..875dd5d417ee 100644
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -107,6 +107,35 @@ static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
>  	return val >> shift;
>  }
>  
> +void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
> +{
> +#ifdef CONFIG_ARM
> +	asm volatile(
> +		"	str %0, [%1]\n"
> +		"	isb\n"
> +	::"r"(val), "r"(pcie->base + reg):"memory");
> +#else
> +	rcar_pci_write_reg(pcie, val, reg);
> +#endif
> +}
> +
> +u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
> +{
> +#ifdef CONFIG_ARM
> +	u32 val;
> +
> +	asm volatile(
> +		"rcar_pci_read_reg_workaround_start:\n"
> +		"1:	ldr %0, [%1]\n"
> +		"	isb\n"
> +	: "=r"(val):"r"(pcie->base + reg):"memory");
> +
> +	return val;
> +#else
> +	return rcar_pci_read_reg(pcie, reg);
> +#endif
> +}
> +
>  /* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
>  static int rcar_pcie_config_access(struct rcar_pcie_host *host,
>  		unsigned char access_type, struct pci_bus *bus,
> @@ -179,9 +208,9 @@ static int rcar_pcie_config_access(struct rcar_pcie_host *host,
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	if (access_type == RCAR_PCI_ACCESS_READ)
> -		*data = rcar_pci_read_reg(pcie, PCIECDR);
> +		*data = rcar_pci_read_reg_workaround(pcie, PCIECDR);
>  	else
> -		rcar_pci_write_reg(pcie, *data, PCIECDR);
> +		rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
>  
>  	/* Disable the configuration access */
>  	rcar_pci_write_reg(pcie, 0, PCIECCTLR);
> @@ -1091,7 +1120,11 @@ static struct platform_driver rcar_pcie_driver = {
>  static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
>  		unsigned int fsr, struct pt_regs *regs)
>  {
> +	extern u32 *rcar_pci_read_reg_workaround_start;
> +	unsigned long pc = instruction_pointer(regs);
> +	unsigned long instr = *(unsigned long *)pc;
>  	unsigned long flags;
> +	u32 reg, val;
>  	int ret = 0;
>  
>  	spin_lock_irqsave(&pmsr_lock, flags);
> @@ -1101,6 +1134,36 @@ static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
>  	if (ret)
>  		goto unlock_exit;
>  
> +	/*
> +	 * Test whether the faulting instruction is 'isb' and if
> +	 * so, test whether it is the 'isb' instruction within
> +	 * rcar_pci_read_reg_workaround() asm volatile()
> +	 * implementation of read access. If it is, fix it up.
> +	 */
> +	instr &= ~0xf;
> +	if ((instr == 0xf57ff060 || instr == 0xf3bf8f60) &&
> +	    (pc == (u32)&rcar_pci_read_reg_workaround_start + 4)) {
> +		/*
> +		 * If the instruction being executed was a read,
> +		 * make it look like it read all-ones.
> +		 */
> +		instr = *(unsigned long *)(pc - 4);
> +		reg = (instr >> 12) & 15;
> +
> +		if ((instr & 0x0c100000) == 0x04100000) {
> +			if (instr & 0x00400000)
> +				val = 255;
> +			else
> +				val = -1;

Can you please use PCI_SET_ERROR_RESPONSE() or something similar here
to make this greppable?

> +			regs->uregs[reg] = val;
> +			regs->ARM_pc += 4;
> +		} else if ((instr & 0x0e100090) == 0x00100090) {
> +			regs->uregs[reg] = -1;

Also here, I guess?

> +			regs->ARM_pc += 4;
> +		}
> +	}
> +
>  unlock_exit:
>  	spin_unlock_irqrestore(&pmsr_lock, flags);
>  	return ret;
> -- 
> 2.34.1
>
Bjorn Helgaas Jan. 19, 2022, 11:27 p.m. UTC | #3
On Tue, Jan 18, 2022 at 10:13:14AM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 17, 2022 at 11:03:55PM +0100, marek.vasut@gmail.com wrote:

> > +	instr &= ~0xf;
> > +	if ((instr == 0xf57ff060 || instr == 0xf3bf8f60) &&
> > +	    (pc == (u32)&rcar_pci_read_reg_workaround_start + 4)) {
> > +		/*
> > +		 * If the instruction being executed was a read,
> > +		 * make it look like it read all-ones.
> > +		 */
> > +		instr = *(unsigned long *)(pc - 4);
> > +		reg = (instr >> 12) & 15;
> > +
> > +		if ((instr & 0x0c100000) == 0x04100000) {
> > +			if (instr & 0x00400000)
> > +				val = 255;
> > +			else
> > +				val = -1;
> 
> Can you please use PCI_SET_ERROR_RESPONSE() or something similar here
> to make this greppable?

I should have mentioned that PCI_SET_ERROR_RESPONSE() was added in the
current merge window, so it will appear in v5.17-rc1.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index f0a0d560fefc..875dd5d417ee 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -107,6 +107,35 @@  static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 	return val >> shift;
 }
 
+void rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
+{
+#ifdef CONFIG_ARM
+	asm volatile(
+		"	str %0, [%1]\n"
+		"	isb\n"
+	::"r"(val), "r"(pcie->base + reg):"memory");
+#else
+	rcar_pci_write_reg(pcie, val, reg);
+#endif
+}
+
+u32 rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, unsigned int reg)
+{
+#ifdef CONFIG_ARM
+	u32 val;
+
+	asm volatile(
+		"rcar_pci_read_reg_workaround_start:\n"
+		"1:	ldr %0, [%1]\n"
+		"	isb\n"
+	: "=r"(val):"r"(pcie->base + reg):"memory");
+
+	return val;
+#else
+	return rcar_pci_read_reg(pcie, reg);
+#endif
+}
+
 /* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
 static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		unsigned char access_type, struct pci_bus *bus,
@@ -179,9 +208,9 @@  static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (access_type == RCAR_PCI_ACCESS_READ)
-		*data = rcar_pci_read_reg(pcie, PCIECDR);
+		*data = rcar_pci_read_reg_workaround(pcie, PCIECDR);
 	else
-		rcar_pci_write_reg(pcie, *data, PCIECDR);
+		rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
 
 	/* Disable the configuration access */
 	rcar_pci_write_reg(pcie, 0, PCIECCTLR);
@@ -1091,7 +1120,11 @@  static struct platform_driver rcar_pcie_driver = {
 static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
+	extern u32 *rcar_pci_read_reg_workaround_start;
+	unsigned long pc = instruction_pointer(regs);
+	unsigned long instr = *(unsigned long *)pc;
 	unsigned long flags;
+	u32 reg, val;
 	int ret = 0;
 
 	spin_lock_irqsave(&pmsr_lock, flags);
@@ -1101,6 +1134,36 @@  static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 	if (ret)
 		goto unlock_exit;
 
+	/*
+	 * Test whether the faulting instruction is 'isb' and if
+	 * so, test whether it is the 'isb' instruction within
+	 * rcar_pci_read_reg_workaround() asm volatile()
+	 * implementation of read access. If it is, fix it up.
+	 */
+	instr &= ~0xf;
+	if ((instr == 0xf57ff060 || instr == 0xf3bf8f60) &&
+	    (pc == (u32)&rcar_pci_read_reg_workaround_start + 4)) {
+		/*
+		 * If the instruction being executed was a read,
+		 * make it look like it read all-ones.
+		 */
+		instr = *(unsigned long *)(pc - 4);
+		reg = (instr >> 12) & 15;
+
+		if ((instr & 0x0c100000) == 0x04100000) {
+			if (instr & 0x00400000)
+				val = 255;
+			else
+				val = -1;
+
+			regs->uregs[reg] = val;
+			regs->ARM_pc += 4;
+		} else if ((instr & 0x0e100090) == 0x00100090) {
+			regs->uregs[reg] = -1;
+			regs->ARM_pc += 4;
+		}
+	}
+
 unlock_exit:
 	spin_unlock_irqrestore(&pmsr_lock, flags);
 	return ret;