| Message ID | 20190402013307.20912-6-marek.vasut@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [V5,1/6] PCI: rcar: Clean up remaining macros defining bits | expand |
On Tue, Apr 02, 2019 at 03:33:07AM +0200, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The MSI message address in the RC address space can be 64 bit. The > R-Car PCIe RC supports such a 64bit MSI message address as well. > The code currently uses virt_to_phys(__get_free_pages()) to obtain > a reserved page for the MSI message address, and the return value > of which can be a 64 bit physical address on 64 bit system. > > However, the driver only programs PCIEMSIALR register with the bottom > 32 bits of the virt_to_phys(__get_free_pages()) return value and does > not program the top 32 bits into PCIEMSIAUR, but rather programs the > PCIEMSIAUR register with 0x0. This worked fine on older 32 bit R-Car > SoCs, however may fail on new 64 bit R-Car SoCs. > > Since from a PCIe controller perspective, an inbound MSI is a memory > write to a special address (in case of this controller, defined by > the value in PCIEMSIAUR:PCIEMSIALR), which triggers an interrupt, but > never hits the DRAM _and_ because allocation of an MSI by a PCIe card > driver obtains the MSI message address by reading PCIEMSIAUR:PCIEMSIALR > in rcar_msi_setup_irqs(), incorrectly programmed PCIEMSIAUR cannot > cause memory corruption or other issues. > > There is however the possibility that if virt_to_phys(__get_free_pages()) > returned address above the 32bit boundary _and_ PCIEMSIAUR was programmed > to 0x0 _and_ if the system had physical RAM at the address matching the > value of PCIEMSIALR, a PCIe card driver could allocate a buffer with a > physical address matching the value of PCIEMSIALR and a remote write to > such a buffer by a PCIe card would trigger a spurious MSI. Very good descripion! > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> I double-checked with the datasheets previously. Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Tue, Apr 02, 2019 at 03:33:07AM +0200, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The MSI message address in the RC address space can be 64 bit. The > R-Car PCIe RC supports such a 64bit MSI message address as well. > The code currently uses virt_to_phys(__get_free_pages()) to obtain > a reserved page for the MSI message address, and the return value > of which can be a 64 bit physical address on 64 bit system. > > However, the driver only programs PCIEMSIALR register with the bottom > 32 bits of the virt_to_phys(__get_free_pages()) return value and does > not program the top 32 bits into PCIEMSIAUR, but rather programs the > PCIEMSIAUR register with 0x0. This worked fine on older 32 bit R-Car > SoCs, however may fail on new 64 bit R-Car SoCs. > > Since from a PCIe controller perspective, an inbound MSI is a memory > write to a special address (in case of this controller, defined by > the value in PCIEMSIAUR:PCIEMSIALR), which triggers an interrupt, but > never hits the DRAM _and_ because allocation of an MSI by a PCIe card > driver obtains the MSI message address by reading PCIEMSIAUR:PCIEMSIALR > in rcar_msi_setup_irqs(), incorrectly programmed PCIEMSIAUR cannot > cause memory corruption or other issues. > > There is however the possibility that if virt_to_phys(__get_free_pages()) > returned address above the 32bit boundary _and_ PCIEMSIAUR was programmed > to 0x0 _and_ if the system had physical RAM at the address matching the > value of PCIEMSIALR, a PCIe card driver could allocate a buffer with a > physical address matching the value of PCIEMSIALR and a remote write to > such a buffer by a PCIe card would trigger a spurious MSI. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
On Tue, Apr 02, 2019 at 03:33:07AM +0200, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The MSI message address in the RC address space can be 64 bit. The > R-Car PCIe RC supports such a 64bit MSI message address as well. > The code currently uses virt_to_phys(__get_free_pages()) to obtain > a reserved page for the MSI message address, and the return value > of which can be a 64 bit physical address on 64 bit system. > > However, the driver only programs PCIEMSIALR register with the bottom > 32 bits of the virt_to_phys(__get_free_pages()) return value and does > not program the top 32 bits into PCIEMSIAUR, but rather programs the > PCIEMSIAUR register with 0x0. This worked fine on older 32 bit R-Car > SoCs, however may fail on new 64 bit R-Car SoCs. > > Since from a PCIe controller perspective, an inbound MSI is a memory > write to a special address (in case of this controller, defined by > the value in PCIEMSIAUR:PCIEMSIALR), which triggers an interrupt, but > never hits the DRAM _and_ because allocation of an MSI by a PCIe card > driver obtains the MSI message address by reading PCIEMSIAUR:PCIEMSIALR > in rcar_msi_setup_irqs(), incorrectly programmed PCIEMSIAUR cannot > cause memory corruption or other issues. > > There is however the possibility that if virt_to_phys(__get_free_pages()) > returned address above the 32bit boundary _and_ PCIEMSIAUR was programmed > to 0x0 _and_ if the system had physical RAM at the address matching the > value of PCIEMSIALR, a PCIe card driver could allocate a buffer with a > physical address matching the value of PCIEMSIALR and a remote write to > such a buffer by a PCIe card would trigger a spurious MSI. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > V2: - s/it's/its/ in commit message > - Add R-B from Geert > V3: - Reworded commit message and thus dropped Geerts R-B > V4: - Add Geert's R-B again > V5: - Rebase on next/master 20190401 > - Use {lower,upper}_32_bits() instead of >> 32 If that's the only reason you resent this series I will add the lower_32_bits() code myself. Please do not rebase on top of next, apply code on top of a fixed -rc1 (we are currently using v5.1-rc1) and if there are dependencies on code already queued do let us know, we will handle conflicts in next ourselves. Lorenzo > --- > drivers/pci/controller/pcie-rcar.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index 168bc6b9bb93..5e0102796345 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -892,7 +892,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > { > struct device *dev = pcie->dev; > struct rcar_msi *msi = &pcie->msi; > - unsigned long base; > + phys_addr_t base; > int err, i; > > mutex_init(&msi->lock); > @@ -933,8 +933,8 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > msi->pages = __get_free_pages(GFP_KERNEL, 0); > base = virt_to_phys((void *)msi->pages); > > - rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR); > - rcar_pci_write_reg(pcie, 0, PCIEMSIAUR); > + rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); > + rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); > > /* enable all MSI interrupts */ > rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > -- > 2.20.1 >
On 4/4/19 11:28 AM, Lorenzo Pieralisi wrote: > On Tue, Apr 02, 2019 at 03:33:07AM +0200, marek.vasut@gmail.com wrote: >> From: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> The MSI message address in the RC address space can be 64 bit. The >> R-Car PCIe RC supports such a 64bit MSI message address as well. >> The code currently uses virt_to_phys(__get_free_pages()) to obtain >> a reserved page for the MSI message address, and the return value >> of which can be a 64 bit physical address on 64 bit system. >> >> However, the driver only programs PCIEMSIALR register with the bottom >> 32 bits of the virt_to_phys(__get_free_pages()) return value and does >> not program the top 32 bits into PCIEMSIAUR, but rather programs the >> PCIEMSIAUR register with 0x0. This worked fine on older 32 bit R-Car >> SoCs, however may fail on new 64 bit R-Car SoCs. >> >> Since from a PCIe controller perspective, an inbound MSI is a memory >> write to a special address (in case of this controller, defined by >> the value in PCIEMSIAUR:PCIEMSIALR), which triggers an interrupt, but >> never hits the DRAM _and_ because allocation of an MSI by a PCIe card >> driver obtains the MSI message address by reading PCIEMSIAUR:PCIEMSIALR >> in rcar_msi_setup_irqs(), incorrectly programmed PCIEMSIAUR cannot >> cause memory corruption or other issues. >> >> There is however the possibility that if virt_to_phys(__get_free_pages()) >> returned address above the 32bit boundary _and_ PCIEMSIAUR was programmed >> to 0x0 _and_ if the system had physical RAM at the address matching the >> value of PCIEMSIALR, a PCIe card driver could allocate a buffer with a >> physical address matching the value of PCIEMSIALR and a remote write to >> such a buffer by a PCIe card would trigger a spurious MSI. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: linux-renesas-soc@vger.kernel.org >> To: linux-pci@vger.kernel.org >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> V2: - s/it's/its/ in commit message >> - Add R-B from Geert >> V3: - Reworded commit message and thus dropped Geerts R-B >> V4: - Add Geert's R-B again >> V5: - Rebase on next/master 20190401 >> - Use {lower,upper}_32_bits() instead of >> 32 > > If that's the only reason you resent this series I will add the > lower_32_bits() code myself. Yes, you asked me to resend the whole series after the bot complained. > Please do not rebase on top of next, apply code on top of a fixed -rc1 > (we are currently using v5.1-rc1) and if there are dependencies on code > already queued do let us know, we will handle conflicts in next > ourselves. So do you want me to resend this one more time ?
On Thu, Apr 04, 2019 at 05:48:36PM +0200, Marek Vasut wrote: > On 4/4/19 11:28 AM, Lorenzo Pieralisi wrote: > > On Tue, Apr 02, 2019 at 03:33:07AM +0200, marek.vasut@gmail.com wrote: > >> From: Marek Vasut <marek.vasut+renesas@gmail.com> > >> > >> The MSI message address in the RC address space can be 64 bit. The > >> R-Car PCIe RC supports such a 64bit MSI message address as well. > >> The code currently uses virt_to_phys(__get_free_pages()) to obtain > >> a reserved page for the MSI message address, and the return value > >> of which can be a 64 bit physical address on 64 bit system. > >> > >> However, the driver only programs PCIEMSIALR register with the bottom > >> 32 bits of the virt_to_phys(__get_free_pages()) return value and does > >> not program the top 32 bits into PCIEMSIAUR, but rather programs the > >> PCIEMSIAUR register with 0x0. This worked fine on older 32 bit R-Car > >> SoCs, however may fail on new 64 bit R-Car SoCs. > >> > >> Since from a PCIe controller perspective, an inbound MSI is a memory > >> write to a special address (in case of this controller, defined by > >> the value in PCIEMSIAUR:PCIEMSIALR), which triggers an interrupt, but > >> never hits the DRAM _and_ because allocation of an MSI by a PCIe card > >> driver obtains the MSI message address by reading PCIEMSIAUR:PCIEMSIALR > >> in rcar_msi_setup_irqs(), incorrectly programmed PCIEMSIAUR cannot > >> cause memory corruption or other issues. > >> > >> There is however the possibility that if virt_to_phys(__get_free_pages()) > >> returned address above the 32bit boundary _and_ PCIEMSIAUR was programmed > >> to 0x0 _and_ if the system had physical RAM at the address matching the > >> value of PCIEMSIALR, a PCIe card driver could allocate a buffer with a > >> physical address matching the value of PCIEMSIALR and a remote write to > >> such a buffer by a PCIe card would trigger a spurious MSI. > >> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >> Cc: Phil Edworthy <phil.edworthy@renesas.com> > >> Cc: Simon Horman <horms+renesas@verge.net.au> > >> Cc: Wolfram Sang <wsa@the-dreams.de> > >> Cc: linux-renesas-soc@vger.kernel.org > >> To: linux-pci@vger.kernel.org > >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> --- > >> V2: - s/it's/its/ in commit message > >> - Add R-B from Geert > >> V3: - Reworded commit message and thus dropped Geerts R-B > >> V4: - Add Geert's R-B again > >> V5: - Rebase on next/master 20190401 > >> - Use {lower,upper}_32_bits() instead of >> 32 > > > > If that's the only reason you resent this series I will add the > > lower_32_bits() code myself. > > Yes, you asked me to resend the whole series after the bot complained. https://lists.01.org/pipermail/kbuild-all/2019-April/059428.html > > Please do not rebase on top of next, apply code on top of a fixed -rc1 > > (we are currently using v5.1-rc1) and if there are dependencies on code > > already queued do let us know, we will handle conflicts in next > > ourselves. > > So do you want me to resend this one more time ? No, in the message above I wanted to say I would make the update myself. Regardless, please never send patches aimed at the PCI tree on top on -next. Thanks, Lorenzo
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index 168bc6b9bb93..5e0102796345 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -892,7 +892,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) { struct device *dev = pcie->dev; struct rcar_msi *msi = &pcie->msi; - unsigned long base; + phys_addr_t base; int err, i; mutex_init(&msi->lock); @@ -933,8 +933,8 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) msi->pages = __get_free_pages(GFP_KERNEL, 0); base = virt_to_phys((void *)msi->pages); - rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR); - rcar_pci_write_reg(pcie, 0, PCIEMSIAUR); + rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR); + rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR); /* enable all MSI interrupts */ rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);