| Message ID | 20190317000608.24881-2-marek.vasut@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/3] PCI: rcar: Replace unsigned long with u32 for register values | expand |
Hello! On 17.03.2019 3:06, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The MSI address can be 64bit. Switch the data type used to hold the > result of virt_to_phys() to phys_addr_t to reflect it's properties Its. > correctly and program the top 32bits of PA into PCIEMSIAUR. > > 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 [...] MBR, Sergei
On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The MSI address can be 64bit. Switch the data type used to hold the > result of virt_to_phys() to phys_addr_t to reflect it's properties > correctly and program the top 32bits of PA into PCIEMSIAUR. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not hit a 64-bit MSI address before? Is this tied to our 32-bit limitation?
On 3/17/19 9:03 AM, Sergei Shtylyov wrote: > Hello! > > On 17.03.2019 3:06, marek.vasut@gmail.com wrote: > >> From: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> The MSI address can be 64bit. Switch the data type used to hold the >> result of virt_to_phys() to phys_addr_t to reflect it's properties > > Its. Fixed >> correctly and program the top 32bits of PA into PCIEMSIAUR. >> >> 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 > [...] > > MBR, Sergei
On 3/17/19 10:12 AM, Wolfram Sang wrote: > On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote: >> From: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> The MSI address can be 64bit. Switch the data type used to hold the >> result of virt_to_phys() to phys_addr_t to reflect it's properties >> correctly and program the top 32bits of PA into PCIEMSIAUR. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not > hit a 64-bit MSI address before? I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would happily return 64bit address, but with the cards I tested (a few intel NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs either way. > Is this tied to our 32-bit limitation? This might be a question for the HW team. I would be tempted to cautiously say yes.
On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The MSI address can be 64bit. Switch the data type used to hold the > result of virt_to_phys() to phys_addr_t to reflect it's properties Side note: probably this should use a proper DMA API instead of get_free_pages()/virt_to_phys(). > correctly and program the top 32bits of PA into PCIEMSIAUR. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -930,7 +930,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); Note that his register is now non-zero. According to the documentation, clearing PCIEMSIALR.MSIFE should be sufficient to disable MSI, and thus there's no need to zero PCIEMSIAUR in rcar_pcie_teardown_msi(). So nothing to change there, good. Gr{oetje,eeting}s, Geert
Hi Marek, On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: > On 3/17/19 10:12 AM, Wolfram Sang wrote: > > On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote: > >> From: Marek Vasut <marek.vasut+renesas@gmail.com> > >> > >> The MSI address can be 64bit. Switch the data type used to hold the > >> result of virt_to_phys() to phys_addr_t to reflect it's properties > >> correctly and program the top 32bits of PA into PCIEMSIAUR. > >> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > > > Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not > > hit a 64-bit MSI address before? > > I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would > happily return 64bit address, but with the cards I tested (a few intel > NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs > either way. No doubt you would be receiving the MSIs, if you have RAM at the truncated address, but wouldn't that cause memory corruption? Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") When MSI support was added, only R-Car H1 and Gen2 were supported. H1 doesn't have LPAE. Gen2 has, but it might have been disabled. Gr{oetje,eeting}s, Geert
Hi Marek, On Mon, Mar 18, 2019 at 9:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 3/17/19 10:12 AM, Wolfram Sang wrote: > > > On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote: > > >> From: Marek Vasut <marek.vasut+renesas@gmail.com> > > >> > > >> The MSI address can be 64bit. Switch the data type used to hold the > > >> result of virt_to_phys() to phys_addr_t to reflect it's properties > > >> correctly and program the top 32bits of PA into PCIEMSIAUR. > > >> > > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > > > > > Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not > > > hit a 64-bit MSI address before? > > > > I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would > > happily return 64bit address, but with the cards I tested (a few intel > > NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs > > either way. > > No doubt you would be receiving the MSIs, if you have RAM at the truncated > address, but wouldn't that cause memory corruption? > > Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") > > When MSI support was added, only R-Car H1 and Gen2 were supported. > H1 doesn't have LPAE. Gen2 has, but it might have been disabled. Correction: as this is always mapped kernel memory, LPAE doesn't matter. So the bug matters for arm64 only. Gr{oetje,eeting}s, Geert
On 3/18/19 10:30 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Mon, Mar 18, 2019 at 9:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>> On 3/17/19 10:12 AM, Wolfram Sang wrote: >>>> On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote: >>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com> >>>>> >>>>> The MSI address can be 64bit. Switch the data type used to hold the >>>>> result of virt_to_phys() to phys_addr_t to reflect it's properties >>>>> correctly and program the top 32bits of PA into PCIEMSIAUR. >>>>> >>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>> >>>> Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not >>>> hit a 64-bit MSI address before? >>> >>> I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would >>> happily return 64bit address, but with the cards I tested (a few intel >>> NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs >>> either way. >> >> No doubt you would be receiving the MSIs, if you have RAM at the truncated >> address, but wouldn't that cause memory corruption? >> >> Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe") >> >> When MSI support was added, only R-Car H1 and Gen2 were supported. >> H1 doesn't have LPAE. Gen2 has, but it might have been disabled. > > Correction: as this is always mapped kernel memory, LPAE doesn't matter. > So the bug matters for arm64 only. And since the address is in the 0x7_3xxx_xxxx range on H3 S-XS, there is no visible memory corruption. Joy ...
On 3/18/19 9:35 AM, Geert Uytterhoeven wrote: > On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote: >> From: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> The MSI address can be 64bit. Switch the data type used to hold the >> result of virt_to_phys() to phys_addr_t to reflect it's properties > > Side note: probably this should use a proper DMA API instead of > get_free_pages()/virt_to_phys(). In fact, I think it doesn't matter. The MSI message is never written into this page as an actual memory write, it is interpreted by the hardware as an interrupt when it arrives. So I believe this page is just a safety net, which is never actually written. >> correctly and program the top 32bits of PA into PCIEMSIAUR. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> [...]
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index 857d88fd03d5..801925a136ae 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -888,7 +888,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); @@ -930,7 +930,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);