net: rtl8169: Fix cache maintenance issues

Quoted 8 lines · Thierry Reding

From: Thierry Reding <[email protected]>

size is always non-negative, so it should be unsigned, whereas the address and size can be larger than 32 bit on 64-bit architectures. Change the mmu_set_region_dcache_behaviour() to use these types in anticipation of making the API available on other architectures.

arch/arm/include/asm/system.h +0 -0

arch/arm/include/asm/system.h enum { · line 204

Quoted 3 lines

arch/arm/include/asm/system.h +1 -1

-void mmu_set_region_dcache_behaviour(u32 start, int size,
+void mmu_set_region_dcache_behaviour(unsigned long start, unsigned long size,
 				     enum dcache_option option);

If we were to use LPAE on a 32-bit system, physical addresses could be more than 32-bit. That would imply we should create a physaddr_t type rather than relying on unsigned long. Still, I suppose since U-Boot just maps RAM (and everything else) 1:1, we'd never use RAM beyond 4GiB, so LPAE actually isn't that interesting...

From: Thierry Reding <[email protected]>

Implement an API that can be used by drivers to allocate memory from a poll that is mapped uncached. This is useful if drivers would otherwise

need to do extensive cache maintenance (or explicitly maintaining the cache isn't safe).

The API is protected using the new CONFIG_SYS_NONCACHED_MEMORY setting. Boards can set this to the size to be used for the non-cached area. The area will typically be right below the malloc() area, but architectures should take care of aligning the beginning and end of the area to honor any mapping restrictions. Architectures must also ensure that mappings established for this area do not overlap with the malloc() area (which should remain cached for improved performance).

While the API is currently only implemented for ARM v7, it should be generic enough to allow other architectures to implement it as well.

then a flush of

README +7 -0

+		one region may result in overwriting data that hardware has
+		written to another region in the same cache-line. This can
+		happen for example in network drivers where descriptors for
+		buffers are typically smaller than the CPU cache-line (e.g.
+		16 bytes vs. 32 or 64 bytes).
+
+		Non-cached memory is only supported on 32-bit ARM at present.

Quoted 4 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Implement an API that can be used by drivers to allocate memory from a poll that is mapped uncached. This is useful if drivers would otherwise

s/poll/pool/

Quoted 15 lines

need to do extensive cache maintenance (or explicitly maintaining the cache isn't safe).

The API is protected using the new CONFIG_SYS_NONCACHED_MEMORY setting. Boards can set this to the size to be used for the non-cached area. The area will typically be right below the malloc() area, but architectures should take care of aligning the beginning and end of the area to honor any mapping restrictions. Architectures must also ensure that mappings established for this area do not overlap with the malloc() area (which should remain cached for improved performance).

While the API is currently only implemented for ARM v7, it should be generic enough to allow other architectures to implement it as well.

README Configuration Settings:

Quoted 9 lines

README +8 -0

 
+- CONFIG_SYS_NONCACHED_MEMORY:
+		Size of non-cached memory area. This area of memory will be
+		typically located right below the malloc() area and mapped
+		uncached in the MMU. This is useful for drivers that would
+		otherwise require a lot of explicit cache maintenance. For
+		some drivers it's also impossible to properly maintain the
+		cache. For example if the regions that need to be flushed
+		are not a multiple of the cache-line size,

I would add:

*and* padding cannot be allocated between the regions to align them (i.e. if the HW requires a contiguous array of regions, and the size of each region is not cache-aligned),

arch/arm/include/asm/system.h void mmu_set_region_dcache_behaviour(unsigned long start, unsigned long size, · line 216

Quoted 13 lines

README +7 -0

+		one region may result in overwriting data that hardware has
+		written to another region in the same cache-line. This can
+		happen for example in network drivers where descriptors for
+		buffers are typically smaller than the CPU cache-line (e.g.
+		16 bytes vs. 32 or 64 bytes).
+
+		Non-cached memory is only supported on 32-bit ARM at present.

arch/arm/include/asm/system.h +3 -0

 
+#ifdef CONFIG_SYS_NONCACHED_MEMORY
+void noncached_init(void);
+unsigned long noncached_alloc(size_t size, size_t align);

s/size_t/unsigned long/ would match the arguments in the earlier patch to mmu_set_region_dcache_behaviour()'s prototype.

Quoted 1 line

arch/arm/lib/cache.c +0 -0

arch/arm/lib/cache.c __weak void enable_caches(void) · line 67

Quoted 5 lines

arch/arm/lib/cache.c +5 -0

+void noncached_init(void)
+{
+	unsigned long start, end, size;
+
+	end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) - MMU_SECTION_SIZE;

Not really "end" (which implies it's inside the region) but "first address outside the region". That said, I'm not sure how to express that succinctly, so perhaps "end" is fine!

arch/arm/lib/cache.c __weak void enable_caches(void) · line 85

Quoted 6 lines

arch/arm/lib/cache.c +6 -0

+unsigned long noncached_alloc(size_t size, size_t align)
+{
+	unsigned long next = ALIGN(noncached_next, align);
+
+	if (next >= noncached_end || (next + size) >= noncached_end)
+		return 0;

If size is quite large, and next is near to U32_MAX, there's a chance of wrap-around there. Perhaps calculate the size left in the region (noncached_end - next), and validate that's >= size.

On 08/18/2014 02:00 AM, Thierry Reding wrote:

arch/arm/include/asm/system.h void mmu_set_region_dcache_behaviour(unsigned long start, unsigned long size, · line 216

Quoted 5 lines

arch/arm/include/asm/system.h +3 -0

 
+#ifdef CONFIG_SYS_NONCACHED_MEMORY
+void noncached_init(void);
+unsigned long noncached_alloc(size_t size, size_t align);

s/size_t/unsigned long/ would match the arguments in the earlier patch to mmu_set_region_dcache_behaviour()'s prototype.

From: Thierry Reding <[email protected]>

Some boards, most notably those with a PCIe ethernet NIC, require this to avoid cache coherency problems. Since the option adds very little code and overhead enable it across all Tegra generations. Other drivers may also start supporting this functionality at some point, so enabling it now will automatically reap the benefits later on.

From: Thierry Reding <[email protected]>

RX and TX descriptor rings should be aligned to 256 byte boundaries. Use the DEFINE_ALIGN_BUFFER() macro to define the buffers so that they don't have to be manually aligned later on. Also make sure that the buffers do align to cache-line boundaries in case the cache-line is higher than the 256 byte alignment requirements of the NIC.

Also add a warning if the cache-line size is larger than the descriptor size, because the driver may discard changes to descriptors made by the hardware when requeuing RX buffers.

Quoted 13 lines · Thierry Reding

From: Thierry Reding <[email protected]>

RX and TX descriptor rings should be aligned to 256 byte boundaries. Use the DEFINE_ALIGN_BUFFER() macro to define the buffers so that they don't have to be manually aligned later on. Also make sure that the buffers do align to cache-line boundaries in case the cache-line is higher than the 256 byte alignment requirements of the NIC.

Also add a warning if the cache-line size is larger than the descriptor size, because the driver may discard changes to descriptors made by the hardware when requeuing RX buffers.

(quoted diff) +0 -0

@@ -909,6 +899,18 @@ static int rtl_init(struct eth_device *dev, bd_t *bis)

drivers/net/rtl8169.c static int rtl_init(struct eth_device *dev, bd_t *bis) · line 909

Quoted 8 lines

drivers/net/rtl8169.c +8 -0

+	/*
+	 * Warn if the cache-line size is larger than the descriptor size. In
+	 * such cases the driver will likely fail because the CPU needs to
+	 * flush the cache when requeuing RX buffers, therefore descriptors
+	 * written by the hardware may be discarded.
+	 */
+	if (ARCH_DMA_MINALIGN > sizeof(struct RxDesc))
+		printf("WARNING: cache-line size is larger than descriptor size\n");

I'd be tempted to make that a compile-time #error (or perhaps just a #warning) Perhaps #error would break compilation of existing boards though?

Quoted 24 lines · Stephen Warren

Quoted 22 lines · Thierry Reding

From: Thierry Reding <[email protected]>

RX and TX descriptor rings should be aligned to 256 byte boundaries. Use the DEFINE_ALIGN_BUFFER() macro to define the buffers so that they don't have to be manually aligned later on. Also make sure that the buffers do align to cache-line boundaries in case the cache-line is higher than the 256 byte alignment requirements of the NIC.

Also add a warning if the cache-line size is larger than the descriptor size, because the driver may discard changes to descriptors made by the hardware when requeuing RX buffers.

drivers/net/rtl8169.c +8 -0

@@ -909,6 +899,18 @@ static int rtl_init(struct eth_device *dev, bd_t *bis)
 
+    /*
+     * Warn if the cache-line size is larger than the descriptor size. In
+     * such cases the driver will likely fail because the CPU needs to
+     * flush the cache when requeuing RX buffers, therefore descriptors
+     * written by the hardware may be discarded.
+     */
+    if (ARCH_DMA_MINALIGN > sizeof(struct RxDesc))
+            printf("WARNING: cache-line size is larger than descriptor size\n");

Quoted 2 lines

I'd be tempted to make that a compile-time #error (or perhaps just a #warning) Perhaps #error would break compilation of existing boards though?

There are two SH4 boards that use the rtl8169 (r7780mp and sh7785lcr) for which this condition is true, so #error would break them (well, technically not r7780mp since it comments out CONFIG_RTL8169 in the configuration). I'll make it a #warning instead.

Quoted 25 lines · Stephen Warren

Quoted 24 lines · Thierry Reding

Quoted 22 lines

From: Thierry Reding <[email protected]>

RX and TX descriptor rings should be aligned to 256 byte boundaries. Use the DEFINE_ALIGN_BUFFER() macro to define the buffers so that they don't have to be manually aligned later on. Also make sure that the buffers do align to cache-line boundaries in case the cache-line is higher than the 256 byte alignment requirements of the NIC.

Also add a warning if the cache-line size is larger than the descriptor size, because the driver may discard changes to descriptors made by the hardware when requeuing RX buffers.

drivers/net/rtl8169.c +8 -0

@@ -909,6 +899,18 @@ static int rtl_init(struct eth_device *dev, bd_t *bis)
 
+    /*
+     * Warn if the cache-line size is larger than the descriptor size. In
+     * such cases the driver will likely fail because the CPU needs to
+     * flush the cache when requeuing RX buffers, therefore descriptors
+     * written by the hardware may be discarded.
+     */
+    if (ARCH_DMA_MINALIGN > sizeof(struct RxDesc))
+            printf("WARNING: cache-line size is larger than descriptor size\n");

Quoted 7 lines

Quoted 2 lines

I'd be tempted to make that a compile-time #error (or perhaps just a #warning) Perhaps #error would break compilation of existing boards though?

There are two SH4 boards that use the rtl8169 (r7780mp and sh7785lcr) for which this condition is true, so #error would break them (well, technically not r7780mp since it comments out CONFIG_RTL8169 in the configuration). I'll make it a #warning instead.

Adding the maintainer for comment.

Regards,
Simon

Hi!

I comment to latest patch.

Best regards,
Nobuhiro

2014-11-13 1:23 GMT+09:00 Simon Glass <[email protected]>:

Quoted 34 lines · Thierry Reding

Quoted 25 lines · Stephen Warren

Quoted 24 lines · Thierry Reding

Quoted 22 lines

From: Thierry Reding <[email protected]>

RX and TX descriptor rings should be aligned to 256 byte boundaries. Use the DEFINE_ALIGN_BUFFER() macro to define the buffers so that they don't have to be manually aligned later on. Also make sure that the buffers do align to cache-line boundaries in case the cache-line is higher than the 256 byte alignment requirements of the NIC.

Also add a warning if the cache-line size is larger than the descriptor size, because the driver may discard changes to descriptors made by the hardware when requeuing RX buffers.

drivers/net/rtl8169.c +8 -0

@@ -909,6 +899,18 @@ static int rtl_init(struct eth_device *dev, bd_t *bis)
 
+    /*
+     * Warn if the cache-line size is larger than the descriptor size. In
+     * such cases the driver will likely fail because the CPU needs to
+     * flush the cache when requeuing RX buffers, therefore descriptors
+     * written by the hardware may be discarded.
+     */
+    if (ARCH_DMA_MINALIGN > sizeof(struct RxDesc))
+            printf("WARNING: cache-line size is larger than descriptor size\n");

Quoted 7 lines

Quoted 2 lines

I'd be tempted to make that a compile-time #error (or perhaps just a #warning) Perhaps #error would break compilation of existing boards though?

There are two SH4 boards that use the rtl8169 (r7780mp and sh7785lcr) for which this condition is true, so #error would break them (well, technically not r7780mp since it comments out CONFIG_RTL8169 in the configuration). I'll make it a #warning instead.

Quoted 4 lines

Adding the maintainer for comment.

Regards,
Simon

--
Nobuhiro Iwamatsu
iwamatsu at {nigauri.org / debian.org}
GPG ID: 40AD1FA6

From: Thierry Reding <[email protected]>

To work around potential issues with explicit cache maintenance of the RX and TX descriptor rings, allocate them from a pool of uncached memory if the architecture supports it.

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>

To work around potential issues with explicit cache maintenance of the RX and TX descriptor rings, allocate them from a pool of uncached memory if the architecture supports it.

drivers/net/rtl8169.c struct RxDesc { · line 310

Quoted 8 lines

drivers/net/rtl8169.c +2 -0

 
+#ifndef CONFIG_SYS_NONCACHED_MEMORY
 /* Define the TX Descriptor */
 DEFINE_ALIGN_BUFFER(struct TxDesc, tx_ring, NUM_TX_DESC, RTL8169_ALIGN);
 
 /* Define the RX Descriptor */
 DEFINE_ALIGN_BUFFER(struct RxDesc, rx_ring, NUM_RX_DESC, RTL8169_ALIGN);
+#endif

It feels slightly inconsistent to have one case allocate this memory in BSS at compile-time, and in the other to allocate it dynamically.

Would it be better to remove this global, and simply call different APIs (regular aligned malloc, v.s. non-cached allocate) during rtl_init()?

From: Thierry Reding <[email protected]>

This series attempts to fix a long-standing problem in the rtl8169 driver (though the same problem may exist in other drivers as well). Let me first explain what exactly the issue is:

The rtl8169 driver provides a set of RX and TX descriptors for the device to use. Once they're set up, the device is told about their location so that it can fetch the descriptors using DMA. The device will also write packet state back into these descriptors using DMA. For this to work properly, whenever a driver needs to access these descriptors it needs to invalidate the D-cache line(s) associated with them. Similarly when changes to the descriptor have been made by the driver, the cache lines need to be flushed to make sure the changes are visible to the device.

The descriptors are 16 bytes in size. This causes problems when used on CPUs that have a cache-line size that is larger than 16 bytes. One example is the NVIDIA Tegra124 which has 64-byte cache-lines. That means that 4 descriptors fit into a single cache-line. So whenever the driver flushes a cache-line it has the potential to discard changes made to another descriptor by the DMA device. One typical symptom is that large transfers over TFTP will often not complete and hang somewhere midway because a device marked a packet received but the driver flushing the cache and causing the packet to be lost.

Since the descriptors need to be consecutive in memory, I don't see a way to fix this other than to use uncached memory. Therefore the solution proposed in this patch series is to introduce a mechanism in U-Boot to allow a driver to allocate from a pool of uncached memory. Currently an implementation is provided only for ARM v7. The idea is that a region (of user-definable size) immediately below (taking into account architecture-specific alignment restrictions) the malloc() area is mapped uncacheable in the MMU. A driver can use the new noncached_alloc() function to allocate a chunk of memory from this pool dynamically for buffers that it can't or doesn't want to do any explicit cache-maintainance on, yet needs to be shared with DMA devices.

Patches 1-3 are minor preparatory work. Patch 1 cleans up some coding style issues in the ARM v7 cache code and patch 2 uses more future-proof types for the mmu_set_region_dcache_behaviour() function arguments. Patch 3 is purely for debugging purposes. It will print out the region used by malloc() when DEBUG is enabled. This can be useful to see where the malloc() region is in the memory map (compared to the noncached region introduced in a later patch for example).

Patch 4 implements the noncached API for ARM v7. It obtains the start of the malloc() area and places the noncached region immediately below it so that noncached_alloc() can allocate from it. During boot, the noncached area will be set up immediately after malloc().

Patch 5 enables noncached memory for all Tegra boards. It uses a 1 MiB chunk which should be plenty (it's also the minimum on ARM v7 because it matches the MMU section size and therefore the granularity at which U-Boot can set the cacheable attributes).

Quoted 48 lines · Thierry Reding

From: Thierry Reding <[email protected]>

This series attempts to fix a long-standing problem in the rtl8169 driver (though the same problem may exist in other drivers as well). Let me first explain what exactly the issue is:

The rtl8169 driver provides a set of RX and TX descriptors for the device to use. Once they're set up, the device is told about their location so that it can fetch the descriptors using DMA. The device will also write packet state back into these descriptors using DMA. For this to work properly, whenever a driver needs to access these descriptors it needs to invalidate the D-cache line(s) associated with them. Similarly when changes to the descriptor have been made by the driver, the cache lines need to be flushed to make sure the changes are visible to the device.

The descriptors are 16 bytes in size. This causes problems when used on CPUs that have a cache-line size that is larger than 16 bytes. One example is the NVIDIA Tegra124 which has 64-byte cache-lines. That means that 4 descriptors fit into a single cache-line. So whenever the driver flushes a cache-line it has the potential to discard changes made to another descriptor by the DMA device. One typical symptom is that large transfers over TFTP will often not complete and hang somewhere midway because a device marked a packet received but the driver flushing the cache and causing the packet to be lost.

Since the descriptors need to be consecutive in memory, I don't see a way to fix this other than to use uncached memory. Therefore the solution proposed in this patch series is to introduce a mechanism in U-Boot to allow a driver to allocate from a pool of uncached memory. Currently an implementation is provided only for ARM v7. The idea is that a region (of user-definable size) immediately below (taking into account architecture-specific alignment restrictions) the malloc() area is mapped uncacheable in the MMU. A driver can use the new noncached_alloc() function to allocate a chunk of memory from this pool dynamically for buffers that it can't or doesn't want to do any explicit cache-maintainance on, yet needs to be shared with DMA devices.

Patches 1-3 are minor preparatory work. Patch 1 cleans up some coding style issues in the ARM v7 cache code and patch 2 uses more future-proof types for the mmu_set_region_dcache_behaviour() function arguments. Patch 3 is purely for debugging purposes. It will print out the region used by malloc() when DEBUG is enabled. This can be useful to see where the malloc() region is in the memory map (compared to the noncached region introduced in a later patch for example).

Patch 4 implements the noncached API for ARM v7. It obtains the start of the malloc() area and places the noncached region immediately below it so that noncached_alloc() can allocate from it. During boot, the noncached area will be set up immediately after malloc().

Quoted 4 lines

Patch 5 enables noncached memory for all Tegra boards. It uses a 1 MiB chunk which should be plenty (it's also the minimum on ARM v7 because it matches the MMU section size and therefore the granularity at which U-Boot can set the cacheable attributes).

If LPAE were to be enabled, the minimum would be 2MiB, but I suppose we can deal with that if/when the time comes.