| Message ID | 20190325114101.10198-6-marek.vasut@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [V4,1/6] PCI: rcar: Clean up remaining macros defining bits | expand |
On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> Does this warrant a Fixes tag? That notwithstanding, Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > 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 > --- > drivers/pci/controller/pcie-rcar.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index c6013f95bdb2..62d2de9fbf1c 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -890,7 +890,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); > @@ -932,7 +932,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > 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, base >> 32, PCIEMSIAUR); > > /* enable all MSI interrupts */ > rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); > -- > 2.20.1 >
On Wed, Mar 27, 2019 at 12:30 PM Simon Horman <horms@verge.net.au> wrote: > On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> > > Does this warrant a Fixes tag? (digging in old sent email) Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") Gr{oetje,eeting}s, Geert
On 3/27/19 1:22 PM, Geert Uytterhoeven wrote: > On Wed, Mar 27, 2019 at 12:30 PM Simon Horman <horms@verge.net.au> wrote: >> On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> >> >> Does this warrant a Fixes tag? > > (digging in old sent email) > Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") But does it really fix that commit, given that on Gen2 and earlier, it was not broken as those were 32bit platforms ?
Hi Marek, On Thu, Mar 28, 2019 at 4:19 AM Marek Vasut <marek.vasut@gmail.com> wrote: > On 3/27/19 1:22 PM, Geert Uytterhoeven wrote: > > On Wed, Mar 27, 2019 at 12:30 PM Simon Horman <horms@verge.net.au> wrote: > >> On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> > >> > >> Does this warrant a Fixes tag? > > > > (digging in old sent email) > > Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") > > But does it really fix that commit, given that on Gen2 and earlier, it > was not broken as those were 32bit platforms ? It does not fix the bug on that commit, as the bug cannot happen on arm32. It does fix that commit, in that that commit used "unsigned long" for a physical address, which is wrong, even on arm32 (esp. with LPAE). If you insist on having a Fixes tag for a commit where the bug could be seen: Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") Apart from that, drivers should use the DMA API instead of virt_to_phys(). However, now we have a better understanding of how MSI interrupts work, we don't even need to allocate that page. All we need is the physical address of a page that is guaranteed not to be backed by RAM (i.e. not to be a valid target for a legitimate PCI bus mastering transaction). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Mar 28, 2019 at 09:02:00AM +0100, Geert Uytterhoeven wrote: > Hi Marek, > > On Thu, Mar 28, 2019 at 4:19 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 3/27/19 1:22 PM, Geert Uytterhoeven wrote: > > > On Wed, Mar 27, 2019 at 12:30 PM Simon Horman <horms@verge.net.au> wrote: > > >> On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> > > >> > > >> Does this warrant a Fixes tag? > > > > > > (digging in old sent email) > > > Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") > > > > But does it really fix that commit, given that on Gen2 and earlier, it > > was not broken as those were 32bit platforms ? > > It does not fix the bug on that commit, as the bug cannot happen on arm32. > It does fix that commit, in that that commit used "unsigned long" for a > physical address, which is wrong, even on arm32 (esp. with LPAE). > If you insist on having a Fixes tag for a commit where the bug could be > seen: > Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") > > Apart from that, drivers should use the DMA API instead of virt_to_phys(). > However, now we have a better understanding of how MSI interrupts > work, we don't even need to allocate that page. All we need is the > physical address of a page that is guaranteed not to be backed by RAM > (i.e. not to be a valid target for a legitimate PCI bus mastering > transaction). Agreed but I would merge this patch first since it is a fix and update it later. Shall I go with the Fixes: tag above ? Thanks, Lorenzo > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Lorenzo, On Thu, Mar 28, 2019 at 5:28 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, Mar 28, 2019 at 09:02:00AM +0100, Geert Uytterhoeven wrote: > > On Thu, Mar 28, 2019 at 4:19 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > > On 3/27/19 1:22 PM, Geert Uytterhoeven wrote: > > > > On Wed, Mar 27, 2019 at 12:30 PM Simon Horman <horms@verge.net.au> wrote: > > > >> On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> > > > >> > > > >> Does this warrant a Fixes tag? > > > > > > > > (digging in old sent email) > > > > Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") > > > > > > But does it really fix that commit, given that on Gen2 and earlier, it > > > was not broken as those were 32bit platforms ? > > > > It does not fix the bug on that commit, as the bug cannot happen on arm32. > > It does fix that commit, in that that commit used "unsigned long" for a > > physical address, which is wrong, even on arm32 (esp. with LPAE). > > If you insist on having a Fixes tag for a commit where the bug could be > > seen: > > Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") > > > > Apart from that, drivers should use the DMA API instead of virt_to_phys(). > > However, now we have a better understanding of how MSI interrupts > > work, we don't even need to allocate that page. All we need is the > > physical address of a page that is guaranteed not to be backed by RAM > > (i.e. not to be a valid target for a legitimate PCI bus mastering > > transaction). > > Agreed but I would merge this patch first since it is a fix > and update it later. Sure, definitely. > Shall I go with the Fixes: tag above ? Fine for me, thanks! Gr{oetje,eeting}s, Geert
On 3/28/19 5:31 PM, Geert Uytterhoeven wrote: > Hi Lorenzo, > > On Thu, Mar 28, 2019 at 5:28 PM Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> On Thu, Mar 28, 2019 at 09:02:00AM +0100, Geert Uytterhoeven wrote: >>> On Thu, Mar 28, 2019 at 4:19 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 3/27/19 1:22 PM, Geert Uytterhoeven wrote: >>>>> On Wed, Mar 27, 2019 at 12:30 PM Simon Horman <horms@verge.net.au> wrote: >>>>>> On Mon, Mar 25, 2019 at 12:41:01PM +0100, 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> >>>>>> >>>>>> Does this warrant a Fixes tag? >>>>> >>>>> (digging in old sent email) >>>>> Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") >>>> >>>> But does it really fix that commit, given that on Gen2 and earlier, it >>>> was not broken as those were 32bit platforms ? >>> >>> It does not fix the bug on that commit, as the bug cannot happen on arm32. >>> It does fix that commit, in that that commit used "unsigned long" for a >>> physical address, which is wrong, even on arm32 (esp. with LPAE). >>> If you insist on having a Fixes tag for a commit where the bug could be >>> seen: >>> Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") >>> >>> Apart from that, drivers should use the DMA API instead of virt_to_phys(). >>> However, now we have a better understanding of how MSI interrupts >>> work, we don't even need to allocate that page. All we need is the >>> physical address of a page that is guaranteed not to be backed by RAM >>> (i.e. not to be a valid target for a legitimate PCI bus mastering >>> transaction). >> >> Agreed but I would merge this patch first since it is a fix >> and update it later. > > Sure, definitely. > >> Shall I go with the Fixes: tag above ? > > Fine for me, thanks! I don't feel strongly either way.
Hi Marek, On Mon, Mar 25, 2019 at 12:41 PM <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 > --- > drivers/pci/controller/pcie-rcar.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index c6013f95bdb2..62d2de9fbf1c 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -890,7 +890,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); > @@ -932,7 +932,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > 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, base >> 32, PCIEMSIAUR); As reported by 0day, this causes a warning on arm32 without LPAE: drivers/pci/controller/pcie-rcar.c:935:32: warning: right shift count >= width of type Using upper_32_bits() instead of an explicit shift should fix that. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 3/29/19 8:32 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Mon, Mar 25, 2019 at 12:41 PM <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 >> --- >> drivers/pci/controller/pcie-rcar.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c >> index c6013f95bdb2..62d2de9fbf1c 100644 >> --- a/drivers/pci/controller/pcie-rcar.c >> +++ b/drivers/pci/controller/pcie-rcar.c >> @@ -890,7 +890,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); >> @@ -932,7 +932,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) >> 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, base >> 32, PCIEMSIAUR); > > As reported by 0day, this causes a warning on arm32 without LPAE: > > drivers/pci/controller/pcie-rcar.c:935:32: warning: right shift > count >= width of type > > Using upper_32_bits() instead of an explicit shift should fix that. I saw the report too. Lorenzo, do you want a separate patch to squash with this or V5 ?
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index c6013f95bdb2..62d2de9fbf1c 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -890,7 +890,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); @@ -932,7 +932,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) 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, base >> 32, PCIEMSIAUR); /* enable all MSI interrupts */ rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);