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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>