[2/3] PCI: rcar: Allow 64bit MSI addresses
diff mbox series

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
Related show

Commit Message

Marek Vasut March 17, 2019, 12:06 a.m. UTC
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>
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
---
 drivers/pci/controller/pcie-rcar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Sergei Shtylyov March 17, 2019, 8:03 a.m. UTC | #1
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
Wolfram Sang March 17, 2019, 9:12 a.m. UTC | #2
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?
Marek Vasut March 17, 2019, 10:59 p.m. UTC | #3
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
Marek Vasut March 17, 2019, 11:37 p.m. UTC | #4
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.
Geert Uytterhoeven March 18, 2019, 8:35 a.m. UTC | #5
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
Geert Uytterhoeven March 18, 2019, 8:39 a.m. UTC | #6
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
Geert Uytterhoeven March 18, 2019, 9:30 a.m. UTC | #7
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
Marek Vasut March 19, 2019, 1:16 a.m. UTC | #8
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 ...
Marek Vasut March 22, 2019, 2:30 a.m. UTC | #9
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>

[...]

Patch
diff mbox series

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);