[PATCH 0/3] i.MX8 malloc pool position and 32-bit only DMA

[PATCH 0/3] i.MX8 malloc pool position and 32-bit only DMA

[Andrey Smirnov]

Everyone:

This series is a result of debugging FEC and uSDHC failures on
i.MX8MQ. Patches 1 and 2 are pretty straightforward and shouldn't be
controversial. Patch 3, OTOH, may or may not be a good way to solve
this problem, but it's a good way to start a discussion on the subject
which is my main goal here.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (3):
  mci: imx-esdhc: Bail out if DMA address is larger than 32-bits
  net: fec: Bail out if DMA address is larger than 32-bits
  ARM: start: Place malloc pool within 32-bit address space

 arch/arm/cpu/Kconfig    |  3 +++
 arch/arm/cpu/start.c    | 23 +++++++++++++++++++++++
 drivers/mci/Kconfig     |  1 +
 drivers/mci/imx-esdhc.c |  3 +++
 drivers/net/Kconfig     |  1 +
 drivers/net/fec_imx.c   | 10 +++++++++-
 6 files changed, 40 insertions(+), 1 deletion(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] mci: imx-esdhc: Bail out if DMA address is larger than 32-bits

[Andrey Smirnov]

On 64-bit SoCs it becomes possible to end up with a DMA buffer
allocated in the region of memory inaccessible to ESDHC
controller. Change the code to bail out if that happens to avoid
silent failures.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/mci/imx-esdhc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mci/imx-esdhc.c b/drivers/mci/imx-esdhc.c
index db96a8139..f6451e204 100644
--- a/drivers/mci/imx-esdhc.c
+++ b/drivers/mci/imx-esdhc.c
@@ -304,6 +304,9 @@ esdhc_send_cmd(struct mci_host *mci, struct mci_cmd *cmd, struct mci_data *data)
 			dma = dma_map_single(host->dev, ptr, num_bytes, dir);
 			if (dma_mapping_error(host->dev, dma))
 				return -EIO;
+
+			if (dma > U32_MAX)
+				return -EFAULT;
 		}
 
 		err = esdhc_setup_data(mci, data, dma);
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] mci: imx-esdhc: Bail out if DMA address is larger than 32-bits

[Sascha Hauer]

On Thu, Aug 23, 2018 at 08:05:09PM -0700, Andrey Smirnov wrote:
> On 64-bit SoCs it becomes possible to end up with a DMA buffer
> allocated in the region of memory inaccessible to ESDHC
> controller. Change the code to bail out if that happens to avoid
> silent failures.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/mci/imx-esdhc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mci/imx-esdhc.c b/drivers/mci/imx-esdhc.c
> index db96a8139..f6451e204 100644
> --- a/drivers/mci/imx-esdhc.c
> +++ b/drivers/mci/imx-esdhc.c
> @@ -304,6 +304,9 @@ esdhc_send_cmd(struct mci_host *mci, struct mci_cmd *cmd, struct mci_data *data)
>  			dma = dma_map_single(host->dev, ptr, num_bytes, dir);
>  			if (dma_mapping_error(host->dev, dma))
>  				return -EIO;
> +
> +			if (dma > U32_MAX)
> +				return -EFAULT;
>  		}

If struct device_d had a dma_mask member, it could be initialized by the
esdhc driver probe function and dma_map_single() would already fail on
addresses > 32bit without additional checks.

That's what Linux does and I think we should do the same.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] mci: imx-esdhc: Bail out if DMA address is larger than 32-bits

[Andrey Smirnov]

On Sun, Aug 26, 2018 at 11:53 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Thu, Aug 23, 2018 at 08:05:09PM -0700, Andrey Smirnov wrote:
> > On 64-bit SoCs it becomes possible to end up with a DMA buffer
> > allocated in the region of memory inaccessible to ESDHC
> > controller. Change the code to bail out if that happens to avoid
> > silent failures.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  drivers/mci/imx-esdhc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mci/imx-esdhc.c b/drivers/mci/imx-esdhc.c
> > index db96a8139..f6451e204 100644
> > --- a/drivers/mci/imx-esdhc.c
> > +++ b/drivers/mci/imx-esdhc.c
> > @@ -304,6 +304,9 @@ esdhc_send_cmd(struct mci_host *mci, struct mci_cmd *cmd, struct mci_data *data)
> >                       dma = dma_map_single(host->dev, ptr, num_bytes, dir);
> >                       if (dma_mapping_error(host->dev, dma))
> >                               return -EIO;
> > +
> > +                     if (dma > U32_MAX)
> > +                             return -EFAULT;
> >               }
>
> If struct device_d had a dma_mask member, it could be initialized by the
> esdhc driver probe function and dma_map_single() would already fail on
> addresses > 32bit without additional checks.
>
> That's what Linux does and I think we should do the same.
>

Good idea, will do in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] net: fec: Bail out if DMA address is larger than 32-bits

[Andrey Smirnov]

On 64-bit SoCs it becomes possible to end up with a DMA buffer
allocated in the region of memory inaccessible to FEC
controller. Change the code to bail out if that happens to avoid
silent failures that'd result otherwise.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 98711baa7..a61fd8294 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -484,6 +484,9 @@ static int fec_send(struct eth_device *dev, void *eth_data, int data_length)
 	if (dma_mapping_error(fec->dev, dma))
 		return -EIO;
 
+	if (dma > U32_MAX)
+		return -EFAULT;
+
 	writel((uint32_t)(dma), &fec->tbd_base[fec->tbd_index].data_pointer);
 
 	/*
@@ -612,7 +615,12 @@ static int fec_alloc_receive_packets(struct fec_priv *fec, int count, int size)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		writel(virt_to_phys(p), &fec->rbd_base[i].data_pointer);
+		unsigned long addr = virt_to_phys(p);
+
+		if (addr > U32_MAX)
+			return -EFAULT;
+
+		writel(addr, &fec->rbd_base[i].data_pointer);
 		p += size;
 	}
 
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] ARM: start: Place malloc pool within 32-bit address space

[Andrey Smirnov]

Some 64-bit SoC have IP cores whose DMA capability is limited to
32-bits only. To handles such cases introduced DMA_32_BIT_ONLY as well
as add the code to make sure that malloc pool (which will be used for
DMA buffers) is accessible using 32-bit address.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/cpu/Kconfig |  3 +++
 arch/arm/cpu/start.c | 23 +++++++++++++++++++++++
 drivers/mci/Kconfig  |  1 +
 drivers/net/Kconfig  |  1 +
 4 files changed, 28 insertions(+)

diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
index 2359c56b3..d9f68d9d0 100644
--- a/arch/arm/cpu/Kconfig
+++ b/arch/arm/cpu/Kconfig
@@ -3,6 +3,9 @@ comment "Processor Type"
 config PHYS_ADDR_T_64BIT
 	bool
 
+config DMA_32_BIT_ONLY
+	bool
+
 config CPU_32
 	bool
 	select HAS_MODULES
diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index 898f7ae19..3fc322d52 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -203,6 +203,29 @@ __noreturn void barebox_non_pbl_start(unsigned long membase,
 		}
 	}
 
+	if (IS_ENABLED(CONFIG_DMA_32_BIT_ONLY) &&
+	    /* If membase is past 4GiB there's nothing we can do */
+	    !WARN_ON(membase > U32_MAX)) {
+		/*
+		 * Using SZ_4G against unsigned long will produce
+		 * warning when compiling for 32-bit machines, so we
+		 * defined the constant below so we can compare
+		 * against U32_MAX instead.
+		 */
+		const unsigned long __malloc_end = malloc_end - 1;
+
+		if (__malloc_end > U32_MAX) {
+			/*
+			 * Some ARMv8 SoCs use IP blocks that are
+			 * only do DMA transfers in first 4GiB of
+			 * address space. To avoid allocating bad
+			 * DMA buffers we move our malloc pool to
+			 * reside within that region.
+			 */
+			malloc_end = U32_MAX + 1;
+		}
+	}
+
 	/*
 	 * Maximum malloc space is the Kconfig value if given
 	 * or 1GB.
diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
index 954f957bc..0687841a7 100644
--- a/drivers/mci/Kconfig
+++ b/drivers/mci/Kconfig
@@ -83,6 +83,7 @@ config MCI_IMX
 config MCI_IMX_ESDHC
 	bool "i.MX esdhc"
 	depends on ARCH_IMX
+	select DMA_32_BIT_ONLY if PHYS_ADDR_T_64BIT
 	help
 	  Enable this entry to add support to read and write SD cards on a
 	  Freescale i.MX25/35/51 based system.
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index b633a3ac4..a1679d8f6 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -135,6 +135,7 @@ config DRIVER_NET_FEC_IMX
 	bool "i.MX FEC Ethernet driver"
 	depends on ARCH_HAS_FEC_IMX
 	select PHYLIB
+	select DMA_32_BIT_ONLY if PHYS_ADDR_T_64BIT
 
 config DRIVER_NET_GIANFAR
 	bool "Gianfar Ethernet"
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] ARM: start: Place malloc pool within 32-bit address space

[Sascha Hauer]

On Thu, Aug 23, 2018 at 08:05:11PM -0700, Andrey Smirnov wrote:
> Some 64-bit SoC have IP cores whose DMA capability is limited to
> 32-bits only. To handles such cases introduced DMA_32_BIT_ONLY as well
> as add the code to make sure that malloc pool (which will be used for
> DMA buffers) is accessible using 32-bit address.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/cpu/Kconfig |  3 +++
>  arch/arm/cpu/start.c | 23 +++++++++++++++++++++++
>  drivers/mci/Kconfig  |  1 +
>  drivers/net/Kconfig  |  1 +
>  4 files changed, 28 insertions(+)
> 
> diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
> index 2359c56b3..d9f68d9d0 100644
> --- a/arch/arm/cpu/Kconfig
> +++ b/arch/arm/cpu/Kconfig
> @@ -3,6 +3,9 @@ comment "Processor Type"
>  config PHYS_ADDR_T_64BIT
>  	bool
>  
> +config DMA_32_BIT_ONLY
> +	bool
> +
>  config CPU_32
>  	bool
>  	select HAS_MODULES
> diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
> index 898f7ae19..3fc322d52 100644
> --- a/arch/arm/cpu/start.c
> +++ b/arch/arm/cpu/start.c
> @@ -203,6 +203,29 @@ __noreturn void barebox_non_pbl_start(unsigned long membase,
>  		}
>  	}
>  
> +	if (IS_ENABLED(CONFIG_DMA_32_BIT_ONLY) &&
> +	    /* If membase is past 4GiB there's nothing we can do */
> +	    !WARN_ON(membase > U32_MAX)) {
> +		/*
> +		 * Using SZ_4G against unsigned long will produce
> +		 * warning when compiling for 32-bit machines, so we
> +		 * defined the constant below so we can compare
> +		 * against U32_MAX instead.
> +		 */
> +		const unsigned long __malloc_end = malloc_end - 1;
> +
> +		if (__malloc_end > U32_MAX) {
> +			/*
> +			 * Some ARMv8 SoCs use IP blocks that are
> +			 * only do DMA transfers in first 4GiB of
> +			 * address space. To avoid allocating bad
> +			 * DMA buffers we move our malloc pool to
> +			 * reside within that region.
> +			 */
> +			malloc_end = U32_MAX + 1;
> +		}
> +	}

I suggest to call barebox_arm_entry() with 32bit memory only instead of
putting that into common code. This is no proper solution anyway, so I
think we can equally well push that issue back to the SoCs. I may change
my mind when more SoCs come up with this issue...


> diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
> index 954f957bc..0687841a7 100644
> --- a/drivers/mci/Kconfig
> +++ b/drivers/mci/Kconfig
> @@ -83,6 +83,7 @@ config MCI_IMX
>  config MCI_IMX_ESDHC
>  	bool "i.MX esdhc"
>  	depends on ARCH_IMX
> +	select DMA_32_BIT_ONLY if PHYS_ADDR_T_64BIT

Enforcing the 32bit memory limitation at compile time is not very nice
since it means it is active even when the driver ends up being unused.
This is not a practical issue currently but consider a newer SoC with a
newer SD core without this limitation, but which shall be compiled
together with i.MX8M support.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] ARM: start: Place malloc pool within 32-bit address space

[Andrey Smirnov]

On Mon, Aug 27, 2018 at 2:06 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Thu, Aug 23, 2018 at 08:05:11PM -0700, Andrey Smirnov wrote:
> > Some 64-bit SoC have IP cores whose DMA capability is limited to
> > 32-bits only. To handles such cases introduced DMA_32_BIT_ONLY as well
> > as add the code to make sure that malloc pool (which will be used for
> > DMA buffers) is accessible using 32-bit address.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  arch/arm/cpu/Kconfig |  3 +++
> >  arch/arm/cpu/start.c | 23 +++++++++++++++++++++++
> >  drivers/mci/Kconfig  |  1 +
> >  drivers/net/Kconfig  |  1 +
> >  4 files changed, 28 insertions(+)
> >
> > diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
> > index 2359c56b3..d9f68d9d0 100644
> > --- a/arch/arm/cpu/Kconfig
> > +++ b/arch/arm/cpu/Kconfig
> > @@ -3,6 +3,9 @@ comment "Processor Type"
> >  config PHYS_ADDR_T_64BIT
> >       bool
> >
> > +config DMA_32_BIT_ONLY
> > +     bool
> > +
> >  config CPU_32
> >       bool
> >       select HAS_MODULES
> > diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
> > index 898f7ae19..3fc322d52 100644
> > --- a/arch/arm/cpu/start.c
> > +++ b/arch/arm/cpu/start.c
> > @@ -203,6 +203,29 @@ __noreturn void barebox_non_pbl_start(unsigned long membase,
> >               }
> >       }
> >
> > +     if (IS_ENABLED(CONFIG_DMA_32_BIT_ONLY) &&
> > +         /* If membase is past 4GiB there's nothing we can do */
> > +         !WARN_ON(membase > U32_MAX)) {
> > +             /*
> > +              * Using SZ_4G against unsigned long will produce
> > +              * warning when compiling for 32-bit machines, so we
> > +              * defined the constant below so we can compare
> > +              * against U32_MAX instead.
> > +              */
> > +             const unsigned long __malloc_end = malloc_end - 1;
> > +
> > +             if (__malloc_end > U32_MAX) {
> > +                     /*
> > +                      * Some ARMv8 SoCs use IP blocks that are
> > +                      * only do DMA transfers in first 4GiB of
> > +                      * address space. To avoid allocating bad
> > +                      * DMA buffers we move our malloc pool to
> > +                      * reside within that region.
> > +                      */
> > +                     malloc_end = U32_MAX + 1;
> > +             }
> > +     }
>
> I suggest to call barebox_arm_entry() with 32bit memory only instead of
> putting that into common code. This is no proper solution anyway, so I
> think we can equally well push that issue back to the SoCs. I may change
> my mind when more SoCs come up with this issue...
>

OK, fair enough. I'll convert v2 to do that.

>
> > diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
> > index 954f957bc..0687841a7 100644
> > --- a/drivers/mci/Kconfig
> > +++ b/drivers/mci/Kconfig
> > @@ -83,6 +83,7 @@ config MCI_IMX
> >  config MCI_IMX_ESDHC
> >       bool "i.MX esdhc"
> >       depends on ARCH_IMX
> > +     select DMA_32_BIT_ONLY if PHYS_ADDR_T_64BIT
>
> Enforcing the 32bit memory limitation at compile time is not very nice
> since it means it is active even when the driver ends up being unused.
> This is not a practical issue currently but consider a newer SoC with a
> newer SD core without this limitation, but which shall be compiled
> together with i.MX8M support.
>

Yeah, that's true. Luckily we won't need that option if I limit memory
size via barebox_arm_entry() as you suggest.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] i.MX8 malloc pool position and 32-bit only DMA

[Sam Ravnborg]

Hi Andrey.

> This series is a result of debugging FEC and uSDHC failures on
> i.MX8MQ. Patches 1 and 2 are pretty straightforward and shouldn't be
> controversial. Patch 3, OTOH, may or may not be a good way to solve
> this problem, but it's a good way to start a discussion on the subject
> which is my main goal here.
> 
> Feedback is welcome!
> 
> Thanks,
> Andrey Smirnov
> 
> Andrey Smirnov (3):
>   mci: imx-esdhc: Bail out if DMA address is larger than 32-bits
>   net: fec: Bail out if DMA address is larger than 32-bits

In the above patches the checks are distributed to the
users. Are there any reason why we could not centralize this
check in the dma code?
As I assume it is everyone that is constrained to the
32BIT address space.

And do we really need these checks if we teach
malloc to only provide memory that is DMA'able?

For the malloc bits I like that this change
is limited to ARM code and not in generic code.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] i.MX8 malloc pool position and 32-bit only DMA

[Andrey Smirnov]

On Fri, Aug 24, 2018 at 10:15 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Andrey.
>
> > This series is a result of debugging FEC and uSDHC failures on
> > i.MX8MQ. Patches 1 and 2 are pretty straightforward and shouldn't be
> > controversial. Patch 3, OTOH, may or may not be a good way to solve
> > this problem, but it's a good way to start a discussion on the subject
> > which is my main goal here.
> >
> > Feedback is welcome!
> >
> > Thanks,
> > Andrey Smirnov
> >
> > Andrey Smirnov (3):
> >   mci: imx-esdhc: Bail out if DMA address is larger than 32-bits
> >   net: fec: Bail out if DMA address is larger than 32-bits
>
> In the above patches the checks are distributed to the
> users. Are there any reason why we could not centralize this
> check in the dma code?

We definitely can. As Sascha already pointed out this can be folded
into dma_mapping_error().

> As I assume it is everyone that is constrained to the
> 32BIT address space.
>
> And do we really need these checks if we teach
> malloc to only provide memory that is DMA'able?
>

I'd prefer to keep them since I'd rather not have a situation where
there's a chance of silent, hard to detect, failure, since it
manifests in really bizarre ways (in my case Ethernet would stop
working after I would probe eMMC). Also, once all of the error
checking is folded into dma_mapping_error() it wouldn't be that much
of code anyway.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox