| Message ID | 20220117220355.92575-2-marek.vasut@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access() | expand |
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
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 >
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 --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;