| Message ID | 20240223-fix_sparse_errors_checksum_tests-v10-1-b6a45914b7d8@rivosinc.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v10] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests | expand |
On 2/23/24 14:11, Charlie Jenkins wrote: > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > aligning the IP header, which were causing failures on architectures > that do not support misaligned accesses like some ARM platforms. To > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > standard alignment of an IP header and must be supported by the > architecture. > > Furthermore, all architectures except the m68k pad "struct > csum_ipv6_magic_data" to 44 bits. To make compatible with the m68k, > manually pad this structure to 44 bits. > > Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum") > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> Tested-by: Guenter Roeck <linux@roeck-us.net>
Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > aligning the IP header, which were causing failures on architectures > that do not support misaligned accesses like some ARM platforms. To > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > standard alignment of an IP header and must be supported by the > architecture. I'm still wondering what we are really trying to fix here. All other tests are explicitely testing that it works with any alignment. Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as well ? I would expect it, I see no comment in arm code which explicits that assumption around those functions. Isn't the problem only the following line, because csum_offset is unaligned ? csum = *(__wsum *)(random_buf + i + csum_offset); Otherwise, if there really is an alignment issue for the IPv6 source or destination address, isn't it enough to perform a 32 bits alignment ? I guess we should involve ARM people in this discussion. Christophe
On Mon, Feb 26, 2024 at 11:34:51AM +0000, Christophe Leroy wrote: > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > aligning the IP header, which were causing failures on architectures > > that do not support misaligned accesses like some ARM platforms. To > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > standard alignment of an IP header and must be supported by the > > architecture. > > I'm still wondering what we are really trying to fix here. > > All other tests are explicitely testing that it works with any alignment. > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > well ? I would expect it, I see no comment in arm code which explicits > that assumption around those functions. No, these functions are explicitly *not* designed to be used with any alignment. They are for 16-bit alignment only. I'm not sure where the idea that "any alignment" has come from, but it's never been the case AFAIK that we've supported that - or if we do now, that's something which has crept in under the radar.
Le 26/02/2024 à 12:47, Russell King (Oracle) a écrit : > On Mon, Feb 26, 2024 at 11:34:51AM +0000, Christophe Leroy wrote: >> Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : >>> The test cases for ip_fast_csum and csum_ipv6_magic were not properly >>> aligning the IP header, which were causing failures on architectures >>> that do not support misaligned accesses like some ARM platforms. To >>> solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the >>> standard alignment of an IP header and must be supported by the >>> architecture. >> >> I'm still wondering what we are really trying to fix here. >> >> All other tests are explicitely testing that it works with any alignment. >> >> Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as >> well ? I would expect it, I see no comment in arm code which explicits >> that assumption around those functions. > > No, these functions are explicitly *not* designed to be used with any > alignment. They are for 16-bit alignment only. > > I'm not sure where the idea that "any alignment" has come from, but it's > never been the case AFAIK that we've supported that - or if we do now, > that's something which has crept in under the radar. > Ok, 16-bit is fine for me, then there is no need to require a (14 + NET_IP_ALIGN) ie a 16-bytes (128-bit) alignment as this patch is doing. Christophe
On Mon, Feb 26, 2024 at 11:57:24AM +0000, Christophe Leroy wrote: > > > Le 26/02/2024 à 12:47, Russell King (Oracle) a écrit : > > On Mon, Feb 26, 2024 at 11:34:51AM +0000, Christophe Leroy wrote: > >> Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > >>> The test cases for ip_fast_csum and csum_ipv6_magic were not properly > >>> aligning the IP header, which were causing failures on architectures > >>> that do not support misaligned accesses like some ARM platforms. To > >>> solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > >>> standard alignment of an IP header and must be supported by the > >>> architecture. > >> > >> I'm still wondering what we are really trying to fix here. > >> > >> All other tests are explicitely testing that it works with any alignment. > >> > >> Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > >> well ? I would expect it, I see no comment in arm code which explicits > >> that assumption around those functions. > > > > No, these functions are explicitly *not* designed to be used with any > > alignment. They are for 16-bit alignment only. > > > > I'm not sure where the idea that "any alignment" has come from, but it's > > never been the case AFAIK that we've supported that - or if we do now, > > that's something which has crept in under the radar. > > > > Ok, 16-bit is fine for me, then there is no need to require a (14 + > NET_IP_ALIGN) ie a 16-bytes (128-bit) alignment as this patch is doing. Looking again at these two functions, I'm mistaken - this was written for optimal use with 32-bit alignment, not 16-bit. However, the entire IP layer is written with the assumption that for maximum performance, the IP header will be 32-bit aligned. However, that may not always be the case for incoming packets, and what saves 32-bit Arm is the ability to do unaligned loads in later revisions of the architecture, or the alignment fault handler (slow) on older revisions.
On 2/26/24 03:34, Christophe Leroy wrote: > > > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : >> The test cases for ip_fast_csum and csum_ipv6_magic were not properly >> aligning the IP header, which were causing failures on architectures >> that do not support misaligned accesses like some ARM platforms. To >> solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the >> standard alignment of an IP header and must be supported by the >> architecture. > > I'm still wondering what we are really trying to fix here. > > All other tests are explicitely testing that it works with any alignment. > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > well ? I would expect it, I see no comment in arm code which explicits > that assumption around those functions. > > Isn't the problem only the following line, because csum_offset is > unaligned ? > > csum = *(__wsum *)(random_buf + i + csum_offset); > > Otherwise, if there really is an alignment issue for the IPv6 source or > destination address, isn't it enough to perform a 32 bits alignment ? > It isn't just arm. Question should be what alignments the functions are supposed to be able to handle, not what they are optimized for. If byte and/or half word alignments are expected to be supported, there is still architecture code which would have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 and on sh4, for example. If unaligned accesses are expected to be handled, it would probably make sense to add a separate test case, though, to clarify that the test fails due to alignment issues, not due to input parameters. Thanks, Guenter
On Mon, Feb 26, 2024 at 08:44:29AM -0800, Guenter Roeck wrote: > On 2/26/24 03:34, Christophe Leroy wrote: > > > > > > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > > aligning the IP header, which were causing failures on architectures > > > that do not support misaligned accesses like some ARM platforms. To > > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > > standard alignment of an IP header and must be supported by the > > > architecture. > > > > I'm still wondering what we are really trying to fix here. > > > > All other tests are explicitely testing that it works with any alignment. > > > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > > well ? I would expect it, I see no comment in arm code which explicits > > that assumption around those functions. > > > > Isn't the problem only the following line, because csum_offset is > > unaligned ? > > > > csum = *(__wsum *)(random_buf + i + csum_offset); > > > > Otherwise, if there really is an alignment issue for the IPv6 source or > > destination address, isn't it enough to perform a 32 bits alignment ? > > > > It isn't just arm. > > Question should be what alignments the functions are supposed to be able > to handle, not what they are optimized for. If byte and/or half word alignments > are expected to be supported, there is still architecture code which would > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 > and on sh4, for example. If unaligned accesses are expected to be handled, > it would probably make sense to add a separate test case, though, to clarify > that the test fails due to alignment issues, not due to input parameters. It's network driver dependent. Most network drivers receive packets to the offset defined by NET_IP_ALIGN (which is normally 2) which has the effect of "mis-aligning" the ethernet header, but aligning the IP header. Whether drivers do that is up to drivers (and their capabilities). Some network drivers can not do this kind of alignment, so there are cases where the received packets aren't offset by two bytes, leading to the IP header being aligned to an odd 16-bit word rather than an even 16-bit word (and thus 32-bit aligned.) Then you have the possibility of other headers between the ethernet and IP header - not only things like VLANs, but also possibly DSA headers (for switches) and how big those are. There's a lot to be researched here!
On Mon, Feb 26, 2024 at 05:50:57PM +0000, Russell King (Oracle) wrote: > On Mon, Feb 26, 2024 at 08:44:29AM -0800, Guenter Roeck wrote: > > On 2/26/24 03:34, Christophe Leroy wrote: > > > > > > > > > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > > > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > > > aligning the IP header, which were causing failures on architectures > > > > that do not support misaligned accesses like some ARM platforms. To > > > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > > > standard alignment of an IP header and must be supported by the > > > > architecture. > > > > > > I'm still wondering what we are really trying to fix here. > > > > > > All other tests are explicitely testing that it works with any alignment. > > > > > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > > > well ? I would expect it, I see no comment in arm code which explicits > > > that assumption around those functions. > > > > > > Isn't the problem only the following line, because csum_offset is > > > unaligned ? > > > > > > csum = *(__wsum *)(random_buf + i + csum_offset); > > > > > > Otherwise, if there really is an alignment issue for the IPv6 source or > > > destination address, isn't it enough to perform a 32 bits alignment ? > > > > > > > It isn't just arm. > > > > Question should be what alignments the functions are supposed to be able > > to handle, not what they are optimized for. If byte and/or half word alignments > > are expected to be supported, there is still architecture code which would > > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 > > and on sh4, for example. If unaligned accesses are expected to be handled, > > it would probably make sense to add a separate test case, though, to clarify > > that the test fails due to alignment issues, not due to input parameters. > > It's network driver dependent. Most network drivers receive packets > to the offset defined by NET_IP_ALIGN (which is normally 2) which > has the effect of "mis-aligning" the ethernet header, but aligning > the IP header. > > Whether drivers do that is up to drivers (and their capabilities). > Some network drivers can not do this kind of alignment, so there are > cases where the received packets aren't offset by two bytes, leading > to the IP header being aligned to an odd 16-bit word rather than an > even 16-bit word (and thus 32-bit aligned.) > > Then you have the possibility of other headers between the ethernet > and IP header - not only things like VLANs, but also possibly DSA > headers (for switches) and how big those are. Those additional combinations can be supported by future test cases, but the goal of this patch was simply to have basic testing for these functions. The NET_IP_ALIGN offset is what the kernel defines to be supported, so that is the test case I went for. - Charlie > > There's a lot to be researched here! > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Feb 26, 2024 at 10:35:18AM -0800, Charlie Jenkins wrote: > On Mon, Feb 26, 2024 at 05:50:57PM +0000, Russell King (Oracle) wrote: > > On Mon, Feb 26, 2024 at 08:44:29AM -0800, Guenter Roeck wrote: > > > On 2/26/24 03:34, Christophe Leroy wrote: > > > > > > > > > > > > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > > > > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > > > > aligning the IP header, which were causing failures on architectures > > > > > that do not support misaligned accesses like some ARM platforms. To > > > > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > > > > standard alignment of an IP header and must be supported by the > > > > > architecture. > > > > > > > > I'm still wondering what we are really trying to fix here. > > > > > > > > All other tests are explicitely testing that it works with any alignment. > > > > > > > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > > > > well ? I would expect it, I see no comment in arm code which explicits > > > > that assumption around those functions. > > > > > > > > Isn't the problem only the following line, because csum_offset is > > > > unaligned ? > > > > > > > > csum = *(__wsum *)(random_buf + i + csum_offset); > > > > > > > > Otherwise, if there really is an alignment issue for the IPv6 source or > > > > destination address, isn't it enough to perform a 32 bits alignment ? > > > > > > > > > > It isn't just arm. > > > > > > Question should be what alignments the functions are supposed to be able > > > to handle, not what they are optimized for. If byte and/or half word alignments > > > are expected to be supported, there is still architecture code which would > > > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 > > > and on sh4, for example. If unaligned accesses are expected to be handled, > > > it would probably make sense to add a separate test case, though, to clarify > > > that the test fails due to alignment issues, not due to input parameters. > > > > It's network driver dependent. Most network drivers receive packets > > to the offset defined by NET_IP_ALIGN (which is normally 2) which > > has the effect of "mis-aligning" the ethernet header, but aligning > > the IP header. > > > > Whether drivers do that is up to drivers (and their capabilities). > > Some network drivers can not do this kind of alignment, so there are > > cases where the received packets aren't offset by two bytes, leading > > to the IP header being aligned to an odd 16-bit word rather than an > > even 16-bit word (and thus 32-bit aligned.) > > > > Then you have the possibility of other headers between the ethernet > > and IP header - not only things like VLANs, but also possibly DSA > > headers (for switches) and how big those are. > > Those additional combinations can be supported by future test cases, > but the goal of this patch was simply to have basic testing for these > functions. The NET_IP_ALIGN offset is what the kernel defines to be > supported, so that is the test case I went for. I think you misunderstand. "NET_IP_ALIGN offset is what the kernel defines to be supported" is a gross misinterpretation. It is not "defined to be supported" at all. It is the _preferred_ alignment nothing more, nothing less.
On Mon, Feb 26, 2024 at 07:06:46PM +0000, Russell King (Oracle) wrote: > On Mon, Feb 26, 2024 at 10:35:18AM -0800, Charlie Jenkins wrote: > > On Mon, Feb 26, 2024 at 05:50:57PM +0000, Russell King (Oracle) wrote: > > > On Mon, Feb 26, 2024 at 08:44:29AM -0800, Guenter Roeck wrote: > > > > On 2/26/24 03:34, Christophe Leroy wrote: > > > > > > > > > > > > > > > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > > > > > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > > > > > aligning the IP header, which were causing failures on architectures > > > > > > that do not support misaligned accesses like some ARM platforms. To > > > > > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > > > > > standard alignment of an IP header and must be supported by the > > > > > > architecture. > > > > > > > > > > I'm still wondering what we are really trying to fix here. > > > > > > > > > > All other tests are explicitely testing that it works with any alignment. > > > > > > > > > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > > > > > well ? I would expect it, I see no comment in arm code which explicits > > > > > that assumption around those functions. > > > > > > > > > > Isn't the problem only the following line, because csum_offset is > > > > > unaligned ? > > > > > > > > > > csum = *(__wsum *)(random_buf + i + csum_offset); > > > > > > > > > > Otherwise, if there really is an alignment issue for the IPv6 source or > > > > > destination address, isn't it enough to perform a 32 bits alignment ? > > > > > > > > > > > > > It isn't just arm. > > > > > > > > Question should be what alignments the functions are supposed to be able > > > > to handle, not what they are optimized for. If byte and/or half word alignments > > > > are expected to be supported, there is still architecture code which would > > > > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 > > > > and on sh4, for example. If unaligned accesses are expected to be handled, > > > > it would probably make sense to add a separate test case, though, to clarify > > > > that the test fails due to alignment issues, not due to input parameters. > > > > > > It's network driver dependent. Most network drivers receive packets > > > to the offset defined by NET_IP_ALIGN (which is normally 2) which > > > has the effect of "mis-aligning" the ethernet header, but aligning > > > the IP header. > > > > > > Whether drivers do that is up to drivers (and their capabilities). > > > Some network drivers can not do this kind of alignment, so there are > > > cases where the received packets aren't offset by two bytes, leading > > > to the IP header being aligned to an odd 16-bit word rather than an > > > even 16-bit word (and thus 32-bit aligned.) > > > > > > Then you have the possibility of other headers between the ethernet > > > and IP header - not only things like VLANs, but also possibly DSA > > > headers (for switches) and how big those are. > > > > Those additional combinations can be supported by future test cases, > > but the goal of this patch was simply to have basic testing for these > > functions. The NET_IP_ALIGN offset is what the kernel defines to be > > supported, so that is the test case I went for. > > I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > defines to be supported" is a gross misinterpretation. It is not > "defined to be supported" at all. It is the _preferred_ alignment > nothing more, nothing less. What alignment can be relied on by a test case? - Charlie > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
... > I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > defines to be supported" is a gross misinterpretation. It is not > "defined to be supported" at all. It is the _preferred_ alignment > nothing more, nothing less. I'm sure I've seen code that would realign IP headers to a 4 byte boundary before processing them - but that might not have been in Linux. I'm also sure there are cpu which will fault double length misaligned memory transfers - which might be used to marginally speed up code. Assuming more than 4 byte alignment for the IP header is likely 'wishful thinking'. There is plenty of ethernet hardware that can only write frames to even boundaries and plenty of cpu that fault misaligned accesses. There are even cases of both on the same silicon die. You also pretty much never want a fault handler to fixup misaligned ethernet frames (or really anything else for that matter). It is always going to be better to check in the code itself. x86 has just made people 'sloppy' :-) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: > ... > > I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > > defines to be supported" is a gross misinterpretation. It is not > > "defined to be supported" at all. It is the _preferred_ alignment > > nothing more, nothing less. This distinction is arbitrary in practice, but I am open to being proven wrong if you have data to back up this statement. If the driver chooses to not follow this, then the driver might not work. ARM defines the NET_IP_ALIGN to be 2 to pad out the header to be on the supported alignment. If the driver chooses to pad with one byte instead of 2 bytes, the driver may fail to work as the CPU may stall after the misaligned access. > > I'm sure I've seen code that would realign IP headers to a 4 byte > boundary before processing them - but that might not have been in > Linux. > > I'm also sure there are cpu which will fault double length misaligned > memory transfers - which might be used to marginally speed up code. > Assuming more than 4 byte alignment for the IP header is likely > 'wishful thinking'. > > There is plenty of ethernet hardware that can only write frames > to even boundaries and plenty of cpu that fault misaligned accesses. > There are even cases of both on the same silicon die. > > You also pretty much never want a fault handler to fixup misaligned > ethernet frames (or really anything else for that matter). > It is always going to be better to check in the code itself. > > x86 has just made people 'sloppy' :-) > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > If somebody has a solution they deem to be better, I am happy to change this test case. Otherwise, I would appreciate a maintainer resolving this discussion and apply this fix. - Charlie
On 2/26/24 15:17, Charlie Jenkins wrote: > On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: >> ... >>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel >>> defines to be supported" is a gross misinterpretation. It is not >>> "defined to be supported" at all. It is the _preferred_ alignment >>> nothing more, nothing less. > > This distinction is arbitrary in practice, but I am open to being proven > wrong if you have data to back up this statement. If the driver chooses > to not follow this, then the driver might not work. ARM defines the > NET_IP_ALIGN to be 2 to pad out the header to be on the supported > alignment. If the driver chooses to pad with one byte instead of 2 > bytes, the driver may fail to work as the CPU may stall after the > misaligned access. > >> >> I'm sure I've seen code that would realign IP headers to a 4 byte >> boundary before processing them - but that might not have been in >> Linux. >> >> I'm also sure there are cpu which will fault double length misaligned >> memory transfers - which might be used to marginally speed up code. >> Assuming more than 4 byte alignment for the IP header is likely >> 'wishful thinking'. >> >> There is plenty of ethernet hardware that can only write frames >> to even boundaries and plenty of cpu that fault misaligned accesses. >> There are even cases of both on the same silicon die. >> >> You also pretty much never want a fault handler to fixup misaligned >> ethernet frames (or really anything else for that matter). >> It is always going to be better to check in the code itself. >> >> x86 has just made people 'sloppy' :-) >> >> David >> >> - >> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >> Registration No: 1397386 (Wales) >> > > If somebody has a solution they deem to be better, I am happy to change > this test case. Otherwise, I would appreciate a maintainer resolving > this discussion and apply this fix. > Agreed. I do have a couple of patches which add explicit unaligned tests as well as corner case tests (which are intended to trigger as many carry overflows as possible). Once I get those working reliably, I'll be happy to submit them as additional tests. Thanks, Guenter
Le 27/02/2024 à 00:48, Guenter Roeck a écrit : > On 2/26/24 15:17, Charlie Jenkins wrote: >> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: >>> ... >>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel >>>> defines to be supported" is a gross misinterpretation. It is not >>>> "defined to be supported" at all. It is the _preferred_ alignment >>>> nothing more, nothing less. >> >> This distinction is arbitrary in practice, but I am open to being proven >> wrong if you have data to back up this statement. If the driver chooses >> to not follow this, then the driver might not work. ARM defines the >> NET_IP_ALIGN to be 2 to pad out the header to be on the supported >> alignment. If the driver chooses to pad with one byte instead of 2 >> bytes, the driver may fail to work as the CPU may stall after the >> misaligned access. >> >>> >>> I'm sure I've seen code that would realign IP headers to a 4 byte >>> boundary before processing them - but that might not have been in >>> Linux. >>> >>> I'm also sure there are cpu which will fault double length misaligned >>> memory transfers - which might be used to marginally speed up code. >>> Assuming more than 4 byte alignment for the IP header is likely >>> 'wishful thinking'. >>> >>> There is plenty of ethernet hardware that can only write frames >>> to even boundaries and plenty of cpu that fault misaligned accesses. >>> There are even cases of both on the same silicon die. >>> >>> You also pretty much never want a fault handler to fixup misaligned >>> ethernet frames (or really anything else for that matter). >>> It is always going to be better to check in the code itself. >>> >>> x86 has just made people 'sloppy' :-) >>> >>> David >>> >>> - >>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, >>> MK1 1PT, UK >>> Registration No: 1397386 (Wales) >>> >> >> If somebody has a solution they deem to be better, I am happy to change >> this test case. Otherwise, I would appreciate a maintainer resolving >> this discussion and apply this fix. >> > Agreed. > > I do have a couple of patches which add explicit unaligned tests as well as > corner case tests (which are intended to trigger as many carry overflows > as possible). Once I get those working reliably, I'll be happy to submit > them as additional tests. > The functions definitely have to work at least with and without VLAN, which means the alignment cannot be greater than 4 bytes. That's also the outcome of the discussion. Therefore, we can easily fix the tests with for instance the following changes. For the IPv6 test I switched proto and csum to keep csum aligned. (In addition expected values need to be recalculated for the IPv6 case). diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c index bf70850035c7..26b0dbc5b8fd 100644 --- a/lib/checksum_kunit.c +++ b/lib/checksum_kunit.c @@ -581,7 +581,7 @@ static void test_ip_fast_csum(struct kunit *test) u16 expected; for (int len = IPv4_MIN_WORDS; len < IPv4_MAX_WORDS; len++) { - for (int index = 0; index < NUM_IP_FAST_CSUM_TESTS; index++) { + for (int index = 0; index < NUM_IP_FAST_CSUM_TESTS; index += 4) { csum_result = ip_fast_csum(random_buf + index, len); expected = expected_fast_csum[(len - IPv4_MIN_WORDS) * @@ -603,12 +603,10 @@ static void test_csum_ipv6_magic(struct kunit *test) const int daddr_offset = sizeof(struct in6_addr); const int len_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr); - const int proto_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + - sizeof(int); - const int csum_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + - sizeof(int) + sizeof(char); + const int csum_offset = len_offset + sizeof(int); + const int proto_offset = csum_offset + sizeof(int); - for (int i = 0; i < NUM_IPv6_TESTS; i++) { + for (int i = 0; i < NUM_IPv6_TESTS; i += 4) { saddr = (const struct in6_addr *)(random_buf + i); daddr = (const struct in6_addr *)(random_buf + i + daddr_offset); --- We could do even better by taking into account CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and do +1 when it is selected and +4 when it is not selected. Christophe
On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: > > > Le 27/02/2024 à 00:48, Guenter Roeck a écrit : > > On 2/26/24 15:17, Charlie Jenkins wrote: > >> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: > >>> ... > >>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > >>>> defines to be supported" is a gross misinterpretation. It is not > >>>> "defined to be supported" at all. It is the _preferred_ alignment > >>>> nothing more, nothing less. > >> > >> This distinction is arbitrary in practice, but I am open to being proven > >> wrong if you have data to back up this statement. If the driver chooses > >> to not follow this, then the driver might not work. ARM defines the > >> NET_IP_ALIGN to be 2 to pad out the header to be on the supported > >> alignment. If the driver chooses to pad with one byte instead of 2 > >> bytes, the driver may fail to work as the CPU may stall after the > >> misaligned access. > >> > >>> > >>> I'm sure I've seen code that would realign IP headers to a 4 byte > >>> boundary before processing them - but that might not have been in > >>> Linux. > >>> > >>> I'm also sure there are cpu which will fault double length misaligned > >>> memory transfers - which might be used to marginally speed up code. > >>> Assuming more than 4 byte alignment for the IP header is likely > >>> 'wishful thinking'. > >>> > >>> There is plenty of ethernet hardware that can only write frames > >>> to even boundaries and plenty of cpu that fault misaligned accesses. > >>> There are even cases of both on the same silicon die. > >>> > >>> You also pretty much never want a fault handler to fixup misaligned > >>> ethernet frames (or really anything else for that matter). > >>> It is always going to be better to check in the code itself. > >>> > >>> x86 has just made people 'sloppy' :-) > >>> > >>> David > >>> > >>> - > >>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > >>> MK1 1PT, UK > >>> Registration No: 1397386 (Wales) > >>> > >> > >> If somebody has a solution they deem to be better, I am happy to change > >> this test case. Otherwise, I would appreciate a maintainer resolving > >> this discussion and apply this fix. > >> > > Agreed. > > > > I do have a couple of patches which add explicit unaligned tests as well as > > corner case tests (which are intended to trigger as many carry overflows > > as possible). Once I get those working reliably, I'll be happy to submit > > them as additional tests. > > > > The functions definitely have to work at least with and without VLAN, > which means the alignment cannot be greater than 4 bytes. That's also > the outcome of the discussion. Thanks for completely ignoring what I've said. No. The alignment ends up being commonly 2 bytes. As I've said several times, network drivers do _not_ have to respect NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in them which can only DMA to a 32-bit aligned address. This means that the start of the ethernet header is placed at a 32-bit aligned address making the IP header misaligned to 32-bit. I don't see what is so difficult to understand about this... but it seems that my comments on this are being ignored time and time again, and I can only think that those who are ignoring my comments have some alterior motive here.
Hi Charlie, Thanks for your patch! On Fri, Feb 23, 2024 at 11:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > aligning the IP header, which were causing failures on architectures > that do not support misaligned accesses like some ARM platforms. To > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > standard alignment of an IP header and must be supported by the > architecture. > > Furthermore, all architectures except the m68k pad "struct > csum_ipv6_magic_data" to 44 bits. To make compatible with the m68k, > manually pad this structure to 44 bits. s/bits/bytes/ everywhere > > Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum") > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > --- > The ip_fast_csum and csum_ipv6_magic tests did not work on all > architectures due to differences in misaligned access support. > Fix those issues by changing endianness of data and aligning the data. > > This patch relies upon a patch from Christophe: > > [PATCH net] kunit: Fix again checksum tests on big endian CPUs > > https://lore.kernel.org/lkml/73df3a9e95c2179119398ad1b4c84cdacbd8dfb6.1708684443.git.christophe.leroy@csgroup.eu/t/ > --- > Changes in v10: > - Christophe Leroy graciously decided to re-write my patch to fit his > style so I have dropped my endianness+sparse changes and have based by > alignment fixes on his patch. The link to his patch can be seen above. > - I dropped Guenter's tested-by but kept his reviewed-by since only the base > was changed. > - Link to v9: https://lore.kernel.org/r/20240221-fix_sparse_errors_checksum_tests-v9-0-bff4d73ab9d1@rivosinc.com > --- a/lib/checksum_kunit.c > +++ b/lib/checksum_kunit.c > @@ -595,28 +473,31 @@ static void test_ip_fast_csum(struct kunit *test) > static void test_csum_ipv6_magic(struct kunit *test) > { > #if defined(CONFIG_NET) > - const struct in6_addr *saddr; > - const struct in6_addr *daddr; > + struct csum_ipv6_magic_data { > + const struct in6_addr saddr; > + const struct in6_addr daddr; > + __le32 len; > + __wsum csum; > + unsigned char proto; > + unsigned char pad[3]; > + } *data; If having a size of 44 bytes is critical, you really want to add a BUILD_BUG_ON() check for that. Gr{oetje,eeting}s, Geert
Le 27/02/2024 à 11:28, Russell King (Oracle) a écrit : > On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: >> >> >> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : >>> On 2/26/24 15:17, Charlie Jenkins wrote: >>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: >>>>> ... >>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel >>>>>> defines to be supported" is a gross misinterpretation. It is not >>>>>> "defined to be supported" at all. It is the _preferred_ alignment >>>>>> nothing more, nothing less. >>>> >>>> This distinction is arbitrary in practice, but I am open to being proven >>>> wrong if you have data to back up this statement. If the driver chooses >>>> to not follow this, then the driver might not work. ARM defines the >>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported >>>> alignment. If the driver chooses to pad with one byte instead of 2 >>>> bytes, the driver may fail to work as the CPU may stall after the >>>> misaligned access. >>>> >>>>> >>>>> I'm sure I've seen code that would realign IP headers to a 4 byte >>>>> boundary before processing them - but that might not have been in >>>>> Linux. >>>>> >>>>> I'm also sure there are cpu which will fault double length misaligned >>>>> memory transfers - which might be used to marginally speed up code. >>>>> Assuming more than 4 byte alignment for the IP header is likely >>>>> 'wishful thinking'. >>>>> >>>>> There is plenty of ethernet hardware that can only write frames >>>>> to even boundaries and plenty of cpu that fault misaligned accesses. >>>>> There are even cases of both on the same silicon die. >>>>> >>>>> You also pretty much never want a fault handler to fixup misaligned >>>>> ethernet frames (or really anything else for that matter). >>>>> It is always going to be better to check in the code itself. >>>>> >>>>> x86 has just made people 'sloppy' :-) >>>>> >>>>> David >>>>> >>>>> - >>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, >>>>> MK1 1PT, UK >>>>> Registration No: 1397386 (Wales) >>>>> >>>> >>>> If somebody has a solution they deem to be better, I am happy to change >>>> this test case. Otherwise, I would appreciate a maintainer resolving >>>> this discussion and apply this fix. >>>> >>> Agreed. >>> >>> I do have a couple of patches which add explicit unaligned tests as well as >>> corner case tests (which are intended to trigger as many carry overflows >>> as possible). Once I get those working reliably, I'll be happy to submit >>> them as additional tests. >>> >> >> The functions definitely have to work at least with and without VLAN, >> which means the alignment cannot be greater than 4 bytes. That's also >> the outcome of the discussion. > > Thanks for completely ignoring what I've said. No. The alignment ends up > being commonly 2 bytes. > > As I've said several times, network drivers do _not_ have to respect > NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in > them which can only DMA to a 32-bit aligned address. This means that > the start of the ethernet header is placed at a 32-bit aligned address > making the IP header misaligned to 32-bit. > > I don't see what is so difficult to understand about this... but it > seems that my comments on this are being ignored time and time again, > and I can only think that those who are ignoring my comments have > some alterior motive here. > I'm sorry for this misunderstanding. I'm not ignoring what you said at all. I understood that ARM is able to handle unaligned accesses with some exception handlers at worst case and that DMA constraints may lead to the IP header beeing on a 2 bytes alignment only. However I also understood from others that some architectures can't handle such a 2 bytes only alignments. It's been suggested during the discussion that alignment tests should be added later in a follow-up patch. So for the time being I'm trying to find a compromise and get the existing tests working on all platforms but with a smaller alignment than the 16-bytes alignment brought by Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good compromise for this fix. The idea is also to make the fix as minimal as possible, unlike Charlie's patch that is churning up the tests quite heavily. But maybe I misunderstood some of the discussion and indeed 2 bytes alignment would work on all platforms and only an odd alignment is problematic ?
On Tue, Feb 27, 2024 at 11:32:19AM +0000, Christophe Leroy wrote: > > > Le 27/02/2024 à 11:28, Russell King (Oracle) a écrit : > > On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: > >> > >> > >> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : > >>> On 2/26/24 15:17, Charlie Jenkins wrote: > >>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: > >>>>> ... > >>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > >>>>>> defines to be supported" is a gross misinterpretation. It is not > >>>>>> "defined to be supported" at all. It is the _preferred_ alignment > >>>>>> nothing more, nothing less. > >>>> > >>>> This distinction is arbitrary in practice, but I am open to being proven > >>>> wrong if you have data to back up this statement. If the driver chooses > >>>> to not follow this, then the driver might not work. ARM defines the > >>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported > >>>> alignment. If the driver chooses to pad with one byte instead of 2 > >>>> bytes, the driver may fail to work as the CPU may stall after the > >>>> misaligned access. > >>>> > >>>>> > >>>>> I'm sure I've seen code that would realign IP headers to a 4 byte > >>>>> boundary before processing them - but that might not have been in > >>>>> Linux. > >>>>> > >>>>> I'm also sure there are cpu which will fault double length misaligned > >>>>> memory transfers - which might be used to marginally speed up code. > >>>>> Assuming more than 4 byte alignment for the IP header is likely > >>>>> 'wishful thinking'. > >>>>> > >>>>> There is plenty of ethernet hardware that can only write frames > >>>>> to even boundaries and plenty of cpu that fault misaligned accesses. > >>>>> There are even cases of both on the same silicon die. > >>>>> > >>>>> You also pretty much never want a fault handler to fixup misaligned > >>>>> ethernet frames (or really anything else for that matter). > >>>>> It is always going to be better to check in the code itself. > >>>>> > >>>>> x86 has just made people 'sloppy' :-) > >>>>> > >>>>> David > >>>>> > >>>>> - > >>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > >>>>> MK1 1PT, UK > >>>>> Registration No: 1397386 (Wales) > >>>>> > >>>> > >>>> If somebody has a solution they deem to be better, I am happy to change > >>>> this test case. Otherwise, I would appreciate a maintainer resolving > >>>> this discussion and apply this fix. > >>>> > >>> Agreed. > >>> > >>> I do have a couple of patches which add explicit unaligned tests as well as > >>> corner case tests (which are intended to trigger as many carry overflows > >>> as possible). Once I get those working reliably, I'll be happy to submit > >>> them as additional tests. > >>> > >> > >> The functions definitely have to work at least with and without VLAN, > >> which means the alignment cannot be greater than 4 bytes. That's also > >> the outcome of the discussion. > > > > Thanks for completely ignoring what I've said. No. The alignment ends up > > being commonly 2 bytes. > > > > As I've said several times, network drivers do _not_ have to respect > > NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in > > them which can only DMA to a 32-bit aligned address. This means that > > the start of the ethernet header is placed at a 32-bit aligned address > > making the IP header misaligned to 32-bit. > > > > I don't see what is so difficult to understand about this... but it > > seems that my comments on this are being ignored time and time again, > > and I can only think that those who are ignoring my comments have > > some alterior motive here. > > > > I'm sorry for this misunderstanding. I'm not ignoring what you said at > all. I understood that ARM is able to handle unaligned accesses with > some exception handlers at worst case and that DMA constraints may lead > to the IP header beeing on a 2 bytes alignment only. > > However I also understood from others that some architectures can't > handle such a 2 bytes only alignments. > > It's been suggested during the discussion that alignment tests should be > added later in a follow-up patch. So for the time being I'm trying to > find a compromise and get the existing tests working on all platforms > but with a smaller alignment than the 16-bytes alignment brought by > Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good > compromise for this fix. The idea is also to make the fix as minimal as > possible, unlike Charlie's patch that is churning up the tests quite > heavily. Do you have a list of platforms this is failing on? I haven't seen any reports that haven't been fixed. - Charlie > > But maybe I misunderstood some of the discussion and indeed 2 bytes > alignment would work on all platforms and only an odd alignment is > problematic ?
On Tue, Feb 27, 2024 at 12:17:58PM +0100, Geert Uytterhoeven wrote: > Hi Charlie, > > Thanks for your patch! > > On Fri, Feb 23, 2024 at 11:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > aligning the IP header, which were causing failures on architectures > > that do not support misaligned accesses like some ARM platforms. To > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > standard alignment of an IP header and must be supported by the > > architecture. > > > > Furthermore, all architectures except the m68k pad "struct > > csum_ipv6_magic_data" to 44 bits. To make compatible with the m68k, > > manually pad this structure to 44 bits. > > s/bits/bytes/ everywhere Whoops, thanks! > > > > > Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum") > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > > --- > > The ip_fast_csum and csum_ipv6_magic tests did not work on all > > architectures due to differences in misaligned access support. > > Fix those issues by changing endianness of data and aligning the data. > > > > This patch relies upon a patch from Christophe: > > > > [PATCH net] kunit: Fix again checksum tests on big endian CPUs > > > > https://lore.kernel.org/lkml/73df3a9e95c2179119398ad1b4c84cdacbd8dfb6.1708684443.git.christophe.leroy@csgroup.eu/t/ > > --- > > Changes in v10: > > - Christophe Leroy graciously decided to re-write my patch to fit his > > style so I have dropped my endianness+sparse changes and have based by > > alignment fixes on his patch. The link to his patch can be seen above. > > - I dropped Guenter's tested-by but kept his reviewed-by since only the base > > was changed. > > - Link to v9: https://lore.kernel.org/r/20240221-fix_sparse_errors_checksum_tests-v9-0-bff4d73ab9d1@rivosinc.com > > > --- a/lib/checksum_kunit.c > > +++ b/lib/checksum_kunit.c > > > @@ -595,28 +473,31 @@ static void test_ip_fast_csum(struct kunit *test) > > static void test_csum_ipv6_magic(struct kunit *test) > > { > > #if defined(CONFIG_NET) > > - const struct in6_addr *saddr; > > - const struct in6_addr *daddr; > > + struct csum_ipv6_magic_data { > > + const struct in6_addr saddr; > > + const struct in6_addr daddr; > > + __le32 len; > > + __wsum csum; > > + unsigned char proto; > > + unsigned char pad[3]; > > + } *data; > > If having a size of 44 bytes is critical, you really want to add a > BUILD_BUG_ON() check for that. Good idea, I will add that. - Charlie > > 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
Le 27/02/2024 à 18:54, Charlie Jenkins a écrit : > On Tue, Feb 27, 2024 at 11:32:19AM +0000, Christophe Leroy wrote: >> >> >> Le 27/02/2024 à 11:28, Russell King (Oracle) a écrit : >>> On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: >>>> >>>> >>>> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : >>>>> On 2/26/24 15:17, Charlie Jenkins wrote: >>>>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: >>>>>>> ... >>>>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel >>>>>>>> defines to be supported" is a gross misinterpretation. It is not >>>>>>>> "defined to be supported" at all. It is the _preferred_ alignment >>>>>>>> nothing more, nothing less. >>>>>> >>>>>> This distinction is arbitrary in practice, but I am open to being proven >>>>>> wrong if you have data to back up this statement. If the driver chooses >>>>>> to not follow this, then the driver might not work. ARM defines the >>>>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported >>>>>> alignment. If the driver chooses to pad with one byte instead of 2 >>>>>> bytes, the driver may fail to work as the CPU may stall after the >>>>>> misaligned access. >>>>>> >>>>>>> >>>>>>> I'm sure I've seen code that would realign IP headers to a 4 byte >>>>>>> boundary before processing them - but that might not have been in >>>>>>> Linux. >>>>>>> >>>>>>> I'm also sure there are cpu which will fault double length misaligned >>>>>>> memory transfers - which might be used to marginally speed up code. >>>>>>> Assuming more than 4 byte alignment for the IP header is likely >>>>>>> 'wishful thinking'. >>>>>>> >>>>>>> There is plenty of ethernet hardware that can only write frames >>>>>>> to even boundaries and plenty of cpu that fault misaligned accesses. >>>>>>> There are even cases of both on the same silicon die. >>>>>>> >>>>>>> You also pretty much never want a fault handler to fixup misaligned >>>>>>> ethernet frames (or really anything else for that matter). >>>>>>> It is always going to be better to check in the code itself. >>>>>>> >>>>>>> x86 has just made people 'sloppy' :-) >>>>>>> >>>>>>> David >>>>>>> >>>>>>> - >>>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, >>>>>>> MK1 1PT, UK >>>>>>> Registration No: 1397386 (Wales) >>>>>>> >>>>>> >>>>>> If somebody has a solution they deem to be better, I am happy to change >>>>>> this test case. Otherwise, I would appreciate a maintainer resolving >>>>>> this discussion and apply this fix. >>>>>> >>>>> Agreed. >>>>> >>>>> I do have a couple of patches which add explicit unaligned tests as well as >>>>> corner case tests (which are intended to trigger as many carry overflows >>>>> as possible). Once I get those working reliably, I'll be happy to submit >>>>> them as additional tests. >>>>> >>>> >>>> The functions definitely have to work at least with and without VLAN, >>>> which means the alignment cannot be greater than 4 bytes. That's also >>>> the outcome of the discussion. >>> >>> Thanks for completely ignoring what I've said. No. The alignment ends up >>> being commonly 2 bytes. >>> >>> As I've said several times, network drivers do _not_ have to respect >>> NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in >>> them which can only DMA to a 32-bit aligned address. This means that >>> the start of the ethernet header is placed at a 32-bit aligned address >>> making the IP header misaligned to 32-bit. >>> >>> I don't see what is so difficult to understand about this... but it >>> seems that my comments on this are being ignored time and time again, >>> and I can only think that those who are ignoring my comments have >>> some alterior motive here. >>> >> >> I'm sorry for this misunderstanding. I'm not ignoring what you said at >> all. I understood that ARM is able to handle unaligned accesses with >> some exception handlers at worst case and that DMA constraints may lead >> to the IP header beeing on a 2 bytes alignment only. >> >> However I also understood from others that some architectures can't >> handle such a 2 bytes only alignments. >> >> It's been suggested during the discussion that alignment tests should be >> added later in a follow-up patch. So for the time being I'm trying to >> find a compromise and get the existing tests working on all platforms >> but with a smaller alignment than the 16-bytes alignment brought by >> Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good >> compromise for this fix. The idea is also to make the fix as minimal as >> possible, unlike Charlie's patch that is churning up the tests quite >> heavily. > > Do you have a list of platforms this is failing on? I haven't seen any > reports that haven't been fixed. I don't have such a list, but I guess you do ? If all platforms have already been fixed, why are you sending this patch at all ? Christophe
On Tue, Feb 27, 2024 at 06:11:24PM +0000, Christophe Leroy wrote: > > > Le 27/02/2024 à 18:54, Charlie Jenkins a écrit : > > On Tue, Feb 27, 2024 at 11:32:19AM +0000, Christophe Leroy wrote: > >> > >> > >> Le 27/02/2024 à 11:28, Russell King (Oracle) a écrit : > >>> On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: > >>>> > >>>> > >>>> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : > >>>>> On 2/26/24 15:17, Charlie Jenkins wrote: > >>>>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: > >>>>>>> ... > >>>>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > >>>>>>>> defines to be supported" is a gross misinterpretation. It is not > >>>>>>>> "defined to be supported" at all. It is the _preferred_ alignment > >>>>>>>> nothing more, nothing less. > >>>>>> > >>>>>> This distinction is arbitrary in practice, but I am open to being proven > >>>>>> wrong if you have data to back up this statement. If the driver chooses > >>>>>> to not follow this, then the driver might not work. ARM defines the > >>>>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported > >>>>>> alignment. If the driver chooses to pad with one byte instead of 2 > >>>>>> bytes, the driver may fail to work as the CPU may stall after the > >>>>>> misaligned access. > >>>>>> > >>>>>>> > >>>>>>> I'm sure I've seen code that would realign IP headers to a 4 byte > >>>>>>> boundary before processing them - but that might not have been in > >>>>>>> Linux. > >>>>>>> > >>>>>>> I'm also sure there are cpu which will fault double length misaligned > >>>>>>> memory transfers - which might be used to marginally speed up code. > >>>>>>> Assuming more than 4 byte alignment for the IP header is likely > >>>>>>> 'wishful thinking'. > >>>>>>> > >>>>>>> There is plenty of ethernet hardware that can only write frames > >>>>>>> to even boundaries and plenty of cpu that fault misaligned accesses. > >>>>>>> There are even cases of both on the same silicon die. > >>>>>>> > >>>>>>> You also pretty much never want a fault handler to fixup misaligned > >>>>>>> ethernet frames (or really anything else for that matter). > >>>>>>> It is always going to be better to check in the code itself. > >>>>>>> > >>>>>>> x86 has just made people 'sloppy' :-) > >>>>>>> > >>>>>>> David > >>>>>>> > >>>>>>> - > >>>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > >>>>>>> MK1 1PT, UK > >>>>>>> Registration No: 1397386 (Wales) > >>>>>>> > >>>>>> > >>>>>> If somebody has a solution they deem to be better, I am happy to change > >>>>>> this test case. Otherwise, I would appreciate a maintainer resolving > >>>>>> this discussion and apply this fix. > >>>>>> > >>>>> Agreed. > >>>>> > >>>>> I do have a couple of patches which add explicit unaligned tests as well as > >>>>> corner case tests (which are intended to trigger as many carry overflows > >>>>> as possible). Once I get those working reliably, I'll be happy to submit > >>>>> them as additional tests. > >>>>> > >>>> > >>>> The functions definitely have to work at least with and without VLAN, > >>>> which means the alignment cannot be greater than 4 bytes. That's also > >>>> the outcome of the discussion. > >>> > >>> Thanks for completely ignoring what I've said. No. The alignment ends up > >>> being commonly 2 bytes. > >>> > >>> As I've said several times, network drivers do _not_ have to respect > >>> NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in > >>> them which can only DMA to a 32-bit aligned address. This means that > >>> the start of the ethernet header is placed at a 32-bit aligned address > >>> making the IP header misaligned to 32-bit. > >>> > >>> I don't see what is so difficult to understand about this... but it > >>> seems that my comments on this are being ignored time and time again, > >>> and I can only think that those who are ignoring my comments have > >>> some alterior motive here. > >>> > >> > >> I'm sorry for this misunderstanding. I'm not ignoring what you said at > >> all. I understood that ARM is able to handle unaligned accesses with > >> some exception handlers at worst case and that DMA constraints may lead > >> to the IP header beeing on a 2 bytes alignment only. > >> > >> However I also understood from others that some architectures can't > >> handle such a 2 bytes only alignments. > >> > >> It's been suggested during the discussion that alignment tests should be > >> added later in a follow-up patch. So for the time being I'm trying to > >> find a compromise and get the existing tests working on all platforms > >> but with a smaller alignment than the 16-bytes alignment brought by > >> Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good > >> compromise for this fix. The idea is also to make the fix as minimal as > >> possible, unlike Charlie's patch that is churning up the tests quite > >> heavily. > > > > Do you have a list of platforms this is failing on? I haven't seen any > > reports that haven't been fixed. > > I don't have such a list, but I guess you do ? If all platforms have > already been fixed, why are you sending this patch at all ? This patch is what is doing the "fixing". Over the course of 10 versions I have "fixed" the test cases to work on platforms that have various alignment and endianness constraints. The endianness changes were picked off of these patches and spun out into a different patch by you. I originally introduced these two new test cases since I wrote the riscv checksum function implementations and these tests were helpful for me and I figured they may be helpful for somebody else too. - Charlie > > Christophe
Le 27/02/2024 à 19:21, Charlie Jenkins a écrit : > On Tue, Feb 27, 2024 at 06:11:24PM +0000, Christophe Leroy wrote: >> >> >> Le 27/02/2024 à 18:54, Charlie Jenkins a écrit : >>> On Tue, Feb 27, 2024 at 11:32:19AM +0000, Christophe Leroy wrote: >>>> >>>> >>>> Le 27/02/2024 à 11:28, Russell King (Oracle) a écrit : >>>>> On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: >>>>>> >>>>>> >>>>>> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : >>>>>>> On 2/26/24 15:17, Charlie Jenkins wrote: >>>>>>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: >>>>>>>>> ... >>>>>>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel >>>>>>>>>> defines to be supported" is a gross misinterpretation. It is not >>>>>>>>>> "defined to be supported" at all. It is the _preferred_ alignment >>>>>>>>>> nothing more, nothing less. >>>>>>>> >>>>>>>> This distinction is arbitrary in practice, but I am open to being proven >>>>>>>> wrong if you have data to back up this statement. If the driver chooses >>>>>>>> to not follow this, then the driver might not work. ARM defines the >>>>>>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported >>>>>>>> alignment. If the driver chooses to pad with one byte instead of 2 >>>>>>>> bytes, the driver may fail to work as the CPU may stall after the >>>>>>>> misaligned access. >>>>>>>> >>>>>>>>> >>>>>>>>> I'm sure I've seen code that would realign IP headers to a 4 byte >>>>>>>>> boundary before processing them - but that might not have been in >>>>>>>>> Linux. >>>>>>>>> >>>>>>>>> I'm also sure there are cpu which will fault double length misaligned >>>>>>>>> memory transfers - which might be used to marginally speed up code. >>>>>>>>> Assuming more than 4 byte alignment for the IP header is likely >>>>>>>>> 'wishful thinking'. >>>>>>>>> >>>>>>>>> There is plenty of ethernet hardware that can only write frames >>>>>>>>> to even boundaries and plenty of cpu that fault misaligned accesses. >>>>>>>>> There are even cases of both on the same silicon die. >>>>>>>>> >>>>>>>>> You also pretty much never want a fault handler to fixup misaligned >>>>>>>>> ethernet frames (or really anything else for that matter). >>>>>>>>> It is always going to be better to check in the code itself. >>>>>>>>> >>>>>>>>> x86 has just made people 'sloppy' :-) >>>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>> - >>>>>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, >>>>>>>>> MK1 1PT, UK >>>>>>>>> Registration No: 1397386 (Wales) >>>>>>>>> >>>>>>>> >>>>>>>> If somebody has a solution they deem to be better, I am happy to change >>>>>>>> this test case. Otherwise, I would appreciate a maintainer resolving >>>>>>>> this discussion and apply this fix. >>>>>>>> >>>>>>> Agreed. >>>>>>> >>>>>>> I do have a couple of patches which add explicit unaligned tests as well as >>>>>>> corner case tests (which are intended to trigger as many carry overflows >>>>>>> as possible). Once I get those working reliably, I'll be happy to submit >>>>>>> them as additional tests. >>>>>>> >>>>>> >>>>>> The functions definitely have to work at least with and without VLAN, >>>>>> which means the alignment cannot be greater than 4 bytes. That's also >>>>>> the outcome of the discussion. >>>>> >>>>> Thanks for completely ignoring what I've said. No. The alignment ends up >>>>> being commonly 2 bytes. >>>>> >>>>> As I've said several times, network drivers do _not_ have to respect >>>>> NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in >>>>> them which can only DMA to a 32-bit aligned address. This means that >>>>> the start of the ethernet header is placed at a 32-bit aligned address >>>>> making the IP header misaligned to 32-bit. >>>>> >>>>> I don't see what is so difficult to understand about this... but it >>>>> seems that my comments on this are being ignored time and time again, >>>>> and I can only think that those who are ignoring my comments have >>>>> some alterior motive here. >>>>> >>>> >>>> I'm sorry for this misunderstanding. I'm not ignoring what you said at >>>> all. I understood that ARM is able to handle unaligned accesses with >>>> some exception handlers at worst case and that DMA constraints may lead >>>> to the IP header beeing on a 2 bytes alignment only. >>>> >>>> However I also understood from others that some architectures can't >>>> handle such a 2 bytes only alignments. >>>> >>>> It's been suggested during the discussion that alignment tests should be >>>> added later in a follow-up patch. So for the time being I'm trying to >>>> find a compromise and get the existing tests working on all platforms >>>> but with a smaller alignment than the 16-bytes alignment brought by >>>> Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good >>>> compromise for this fix. The idea is also to make the fix as minimal as >>>> possible, unlike Charlie's patch that is churning up the tests quite >>>> heavily. >>> >>> Do you have a list of platforms this is failing on? I haven't seen any >>> reports that haven't been fixed. >> >> I don't have such a list, but I guess you do ? If all platforms have >> already been fixed, why are you sending this patch at all ? > > This patch is what is doing the "fixing". Over the course of 10 versions > I have "fixed" the test cases to work on platforms that have various > alignment and endianness constraints. The endianness changes were picked > off of these patches and spun out into a different patch by you. > > I originally introduced these two new test cases since I wrote the riscv > checksum function implementations and these tests were helpful for me > and I figured they may be helpful for somebody else too. I see. Then you mis-understood. I don't say your patch leave any platform unfixed. I say that your patch seems bigger than required, it is a churn. In addition your patch assumes an alignment of 16-bytes which, as explained by Russell, it just wrong. At least an alignment of 4 bytes must work on any platforms because of VLANs. Christophe
On Tue, Feb 27, 2024 at 06:35:04PM +0000, Christophe Leroy wrote: > > > Le 27/02/2024 à 19:21, Charlie Jenkins a écrit : > > On Tue, Feb 27, 2024 at 06:11:24PM +0000, Christophe Leroy wrote: > >> > >> > >> Le 27/02/2024 à 18:54, Charlie Jenkins a écrit : > >>> On Tue, Feb 27, 2024 at 11:32:19AM +0000, Christophe Leroy wrote: > >>>> > >>>> > >>>> Le 27/02/2024 à 11:28, Russell King (Oracle) a écrit : > >>>>> On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: > >>>>>> > >>>>>> > >>>>>> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : > >>>>>>> On 2/26/24 15:17, Charlie Jenkins wrote: > >>>>>>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: > >>>>>>>>> ... > >>>>>>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > >>>>>>>>>> defines to be supported" is a gross misinterpretation. It is not > >>>>>>>>>> "defined to be supported" at all. It is the _preferred_ alignment > >>>>>>>>>> nothing more, nothing less. > >>>>>>>> > >>>>>>>> This distinction is arbitrary in practice, but I am open to being proven > >>>>>>>> wrong if you have data to back up this statement. If the driver chooses > >>>>>>>> to not follow this, then the driver might not work. ARM defines the > >>>>>>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported > >>>>>>>> alignment. If the driver chooses to pad with one byte instead of 2 > >>>>>>>> bytes, the driver may fail to work as the CPU may stall after the > >>>>>>>> misaligned access. > >>>>>>>> > >>>>>>>>> > >>>>>>>>> I'm sure I've seen code that would realign IP headers to a 4 byte > >>>>>>>>> boundary before processing them - but that might not have been in > >>>>>>>>> Linux. > >>>>>>>>> > >>>>>>>>> I'm also sure there are cpu which will fault double length misaligned > >>>>>>>>> memory transfers - which might be used to marginally speed up code. > >>>>>>>>> Assuming more than 4 byte alignment for the IP header is likely > >>>>>>>>> 'wishful thinking'. > >>>>>>>>> > >>>>>>>>> There is plenty of ethernet hardware that can only write frames > >>>>>>>>> to even boundaries and plenty of cpu that fault misaligned accesses. > >>>>>>>>> There are even cases of both on the same silicon die. > >>>>>>>>> > >>>>>>>>> You also pretty much never want a fault handler to fixup misaligned > >>>>>>>>> ethernet frames (or really anything else for that matter). > >>>>>>>>> It is always going to be better to check in the code itself. > >>>>>>>>> > >>>>>>>>> x86 has just made people 'sloppy' :-) > >>>>>>>>> > >>>>>>>>> David > >>>>>>>>> > >>>>>>>>> - > >>>>>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > >>>>>>>>> MK1 1PT, UK > >>>>>>>>> Registration No: 1397386 (Wales) > >>>>>>>>> > >>>>>>>> > >>>>>>>> If somebody has a solution they deem to be better, I am happy to change > >>>>>>>> this test case. Otherwise, I would appreciate a maintainer resolving > >>>>>>>> this discussion and apply this fix. > >>>>>>>> > >>>>>>> Agreed. > >>>>>>> > >>>>>>> I do have a couple of patches which add explicit unaligned tests as well as > >>>>>>> corner case tests (which are intended to trigger as many carry overflows > >>>>>>> as possible). Once I get those working reliably, I'll be happy to submit > >>>>>>> them as additional tests. > >>>>>>> > >>>>>> > >>>>>> The functions definitely have to work at least with and without VLAN, > >>>>>> which means the alignment cannot be greater than 4 bytes. That's also > >>>>>> the outcome of the discussion. > >>>>> > >>>>> Thanks for completely ignoring what I've said. No. The alignment ends up > >>>>> being commonly 2 bytes. > >>>>> > >>>>> As I've said several times, network drivers do _not_ have to respect > >>>>> NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in > >>>>> them which can only DMA to a 32-bit aligned address. This means that > >>>>> the start of the ethernet header is placed at a 32-bit aligned address > >>>>> making the IP header misaligned to 32-bit. > >>>>> > >>>>> I don't see what is so difficult to understand about this... but it > >>>>> seems that my comments on this are being ignored time and time again, > >>>>> and I can only think that those who are ignoring my comments have > >>>>> some alterior motive here. > >>>>> > >>>> > >>>> I'm sorry for this misunderstanding. I'm not ignoring what you said at > >>>> all. I understood that ARM is able to handle unaligned accesses with > >>>> some exception handlers at worst case and that DMA constraints may lead > >>>> to the IP header beeing on a 2 bytes alignment only. > >>>> > >>>> However I also understood from others that some architectures can't > >>>> handle such a 2 bytes only alignments. > >>>> > >>>> It's been suggested during the discussion that alignment tests should be > >>>> added later in a follow-up patch. So for the time being I'm trying to > >>>> find a compromise and get the existing tests working on all platforms > >>>> but with a smaller alignment than the 16-bytes alignment brought by > >>>> Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good > >>>> compromise for this fix. The idea is also to make the fix as minimal as > >>>> possible, unlike Charlie's patch that is churning up the tests quite > >>>> heavily. > >>> > >>> Do you have a list of platforms this is failing on? I haven't seen any > >>> reports that haven't been fixed. > >> > >> I don't have such a list, but I guess you do ? If all platforms have > >> already been fixed, why are you sending this patch at all ? > > > > This patch is what is doing the "fixing". Over the course of 10 versions > > I have "fixed" the test cases to work on platforms that have various > > alignment and endianness constraints. The endianness changes were picked > > off of these patches and spun out into a different patch by you. > > > > I originally introduced these two new test cases since I wrote the riscv > > checksum function implementations and these tests were helpful for me > > and I figured they may be helpful for somebody else too. > > I see. > > Then you mis-understood. I don't say your patch leave any platform > unfixed. I say that your patch seems bigger than required, it is a > churn. In addition your patch assumes an alignment of 16-bytes which, as > explained by Russell, it just wrong. At least an alignment of 4 bytes > must work on any platforms because of VLANs. Pardon my ignorance but I do not understand why VLANs cause this test case to be incorrect/introduce churn. The VLAN tag is a 4-byte field that is optionally included in an ethernet header. This causes the header to change from 14 bytes to 18 bytes. If the architecture defines NET_IP_ALIGN to 2, this pads the ethernet header by 2 bytes, causing the payload to be aligned along 16 bytes without VLAN and 20 bytes with VLAN. Another test case can be added that aligns along 18 + NET_IP_ALIGN but that does not achieve the goal of reducing churn and I would not expect those additionally 4 bytes to highlight bugs in any implementation. - Charlie > > Christophe
On 2/27/24 09:54, Charlie Jenkins wrote: >> It's been suggested during the discussion that alignment tests should be >> added later in a follow-up patch. So for the time being I'm trying to >> find a compromise and get the existing tests working on all platforms >> but with a smaller alignment than the 16-bytes alignment brought by >> Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good >> compromise for this fix. The idea is also to make the fix as minimal as >> possible, unlike Charlie's patch that is churning up the tests quite >> heavily. > > Do you have a list of platforms this is failing on? I haven't seen any > reports that haven't been fixed. > This is what I carry locally on top of v6.8-rc6: 097b149e4acb parisc: More csum_ipv6_magic fixes 15bf67a115eb kunit: Fix again checksum tests on big endian CPUs bebe776d36ea parisc: Fix csum_ipv6_magic on 64-bit systems 523208f03063 parisc: Fix csum_ipv6_magic on 32-bit systems a9dda1971c72 parisc: Fix ip_fast_csum 2ad0a6850b64 Revert "sh: Handle calling csum_partial with misaligned data" 7113cc414860 lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests I also have 0dd01a364cb7 lib: checksum: Add some corner cases to IPv6 checksum tests e767cce6598b lib: checksum: Add tests for unaligned IPv6 addresses which I may submit or not depending on the outcome of this discussion. In other words, parisc and sh4 are currently known to be broken in the upstream kernel, with fixes pending. On top of that, arm:mps2-an385 (probably all arm:nommu systems) crashes hard if csum_ipv6_magic() is called with an unaligned address. This is the "known" list of failures. I don't currently run kunit tests on nios2 or riscv32, for example, nor on any architectures with no qemu support. On a side note, most architectures don't handle "len + proto" overflows. While 'len' is a 32-bit parameter, IPv6 only allows for a 16-bit length field. Many implementations of csum_ipv6_magic() specifically do not handle such overflows because that would be pointless and require extra code for no good reason. The current test code doesn't generate such overflows, but its 'len' parameter is almost always larger than 16 bit and thus not realistic. Maybe it would make sense to limit the range of 'len' to 16 bit when calling csum_ipv6_magic(). Thanks, Guenter
.. > This is the "known" list of failures. I don't currently run kunit tests > on nios2 or riscv32, for example, nor on any architectures with no qemu > support. nios2 is definitely going to 'crash and burn' if you do a misaligned access. Although Intel (aka the Altera bit) are claiming current version of their Quartus fpga build software is the last one the will support the nios2. They are expecting everyone to move to a risc-v soft cpu instead. We aren't happy about that, I doubt some of the big telco's are either - I believe some mobile base stations have fpga with a lot of nios2 in them - almost certainly running with a few kB of code and data memory and running small control tasks. If you want to run Linux, find an fpga with an ARM core. There are some solutions - like writing a compatible soft cpu. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Feb 27, 2024 at 10:28:45AM +0000, Russell King (Oracle) wrote: > On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: > > > > > > Le 27/02/2024 à 00:48, Guenter Roeck a écrit : > > > On 2/26/24 15:17, Charlie Jenkins wrote: > > >> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: > > >>> ... > > >>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > > >>>> defines to be supported" is a gross misinterpretation. It is not > > >>>> "defined to be supported" at all. It is the _preferred_ alignment > > >>>> nothing more, nothing less. > > >> > > >> This distinction is arbitrary in practice, but I am open to being proven > > >> wrong if you have data to back up this statement. If the driver chooses > > >> to not follow this, then the driver might not work. ARM defines the > > >> NET_IP_ALIGN to be 2 to pad out the header to be on the supported > > >> alignment. If the driver chooses to pad with one byte instead of 2 > > >> bytes, the driver may fail to work as the CPU may stall after the > > >> misaligned access. > > >> > > >>> > > >>> I'm sure I've seen code that would realign IP headers to a 4 byte > > >>> boundary before processing them - but that might not have been in > > >>> Linux. > > >>> > > >>> I'm also sure there are cpu which will fault double length misaligned > > >>> memory transfers - which might be used to marginally speed up code. > > >>> Assuming more than 4 byte alignment for the IP header is likely > > >>> 'wishful thinking'. > > >>> > > >>> There is plenty of ethernet hardware that can only write frames > > >>> to even boundaries and plenty of cpu that fault misaligned accesses. > > >>> There are even cases of both on the same silicon die. > > >>> > > >>> You also pretty much never want a fault handler to fixup misaligned > > >>> ethernet frames (or really anything else for that matter). > > >>> It is always going to be better to check in the code itself. > > >>> > > >>> x86 has just made people 'sloppy' :-) > > >>> > > >>> David > > >>> > > >>> - > > >>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > > >>> MK1 1PT, UK > > >>> Registration No: 1397386 (Wales) > > >>> > > >> > > >> If somebody has a solution they deem to be better, I am happy to change > > >> this test case. Otherwise, I would appreciate a maintainer resolving > > >> this discussion and apply this fix. > > >> > > > Agreed. > > > > > > I do have a couple of patches which add explicit unaligned tests as well as > > > corner case tests (which are intended to trigger as many carry overflows > > > as possible). Once I get those working reliably, I'll be happy to submit > > > them as additional tests. > > > > > > > The functions definitely have to work at least with and without VLAN, > > which means the alignment cannot be greater than 4 bytes. That's also > > the outcome of the discussion. > > Thanks for completely ignoring what I've said. No. The alignment ends up > being commonly 2 bytes. > > As I've said several times, network drivers do _not_ have to respect > NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in > them which can only DMA to a 32-bit aligned address. This means that > the start of the ethernet header is placed at a 32-bit aligned address > making the IP header misaligned to 32-bit. > > I don't see what is so difficult to understand about this... but it > seems that my comments on this are being ignored time and time again, > and I can only think that those who are ignoring my comments have > some alterior motive here. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! I don't understand how the capabilities of some ARM drivers factor in here. It appears that a common case for calling this function is to pass in an IP header that is aligned along an ethernet header + NET_IP_ALIGN. It is perfectly acceptable that some drivers don't align along NET_IP_ALIGN, but that does not seem relevant here. This test case is supposed to be as true to the "general case" as possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that does not follow this may not be appropriately tested by this test case, but anyone is welcome to submit additional test cases that address this additional alignment concern. - Charlie
On Tue, Feb 27, 2024 at 11:31:01AM -0800, Guenter Roeck wrote: > On 2/27/24 09:54, Charlie Jenkins wrote: > > > > It's been suggested during the discussion that alignment tests should be > > > added later in a follow-up patch. So for the time being I'm trying to > > > find a compromise and get the existing tests working on all platforms > > > but with a smaller alignment than the 16-bytes alignment brought by > > > Charlie's v10 patch. And a 4 bytes alignment seemed to me to be a good > > > compromise for this fix. The idea is also to make the fix as minimal as > > > possible, unlike Charlie's patch that is churning up the tests quite > > > heavily. > > > > Do you have a list of platforms this is failing on? I haven't seen any > > reports that haven't been fixed. > > > > This is what I carry locally on top of v6.8-rc6: > > 097b149e4acb parisc: More csum_ipv6_magic fixes > 15bf67a115eb kunit: Fix again checksum tests on big endian CPUs > bebe776d36ea parisc: Fix csum_ipv6_magic on 64-bit systems > 523208f03063 parisc: Fix csum_ipv6_magic on 32-bit systems > a9dda1971c72 parisc: Fix ip_fast_csum > 2ad0a6850b64 Revert "sh: Handle calling csum_partial with misaligned data" > 7113cc414860 lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests > > I also have > 0dd01a364cb7 lib: checksum: Add some corner cases to IPv6 checksum tests > e767cce6598b lib: checksum: Add tests for unaligned IPv6 addresses > > which I may submit or not depending on the outcome of this discussion. > > In other words, parisc and sh4 are currently known to be broken in the > upstream kernel, with fixes pending. On top of that, arm:mps2-an385 > (probably all arm:nommu systems) crashes hard if csum_ipv6_magic() > is called with an unaligned address. > > This is the "known" list of failures. I don't currently run kunit tests > on nios2 or riscv32, for example, nor on any architectures with no qemu > support. > > On a side note, most architectures don't handle "len + proto" overflows. > While 'len' is a 32-bit parameter, IPv6 only allows for a 16-bit length > field. Many implementations of csum_ipv6_magic() specifically do > not handle such overflows because that would be pointless and require > extra code for no good reason. The current test code doesn't generate > such overflows, but its 'len' parameter is almost always larger than > 16 bit and thus not realistic. Maybe it would make sense to limit > the range of 'len' to 16 bit when calling csum_ipv6_magic(). Thank you for the suggestion, I can limit len to 16-bit. - Charlie > > Thanks, > Guenter >
On 2/27/24 14:44, David Laight wrote: > .. >> This is the "known" list of failures. I don't currently run kunit tests >> on nios2 or riscv32, for example, nor on any architectures with no qemu >> support. > > nios2 is definitely going to 'crash and burn' if you do a misaligned access. > Curiously enough, it doesn't. I get lots of kernel unaligned access @ 0xc848eb78; BADADDR 0xc86f1d01; cause=6, isn=0x20800017 but a checksum test with unaligned data does pass, so the kernel somehow handles it. It does crash, later, though, if CONFIG_NET_TEST is enabled. Apparently the gso tests trigger lots of unaligned accesses, and those are just too much for the kernel to handle. Guenter > Although Intel (aka the Altera bit) are claiming current version > of their Quartus fpga build software is the last one the will > support the nios2. > They are expecting everyone to move to a risc-v soft cpu instead. > We aren't happy about that, I doubt some of the big telco's are > either - I believe some mobile base stations have fpga with a > lot of nios2 in them - almost certainly running with a few kB > of code and data memory and running small control tasks. > If you want to run Linux, find an fpga with an ARM core. > > There are some solutions - like writing a compatible soft cpu. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
Le 28/02/2024 à 01:21, Charlie Jenkins a écrit : > On Tue, Feb 27, 2024 at 10:28:45AM +0000, Russell King (Oracle) wrote: >> On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote: >>> >>> >>> Le 27/02/2024 à 00:48, Guenter Roeck a écrit : >>>> On 2/26/24 15:17, Charlie Jenkins wrote: >>>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote: >>>>>> ... >>>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel >>>>>>> defines to be supported" is a gross misinterpretation. It is not >>>>>>> "defined to be supported" at all. It is the _preferred_ alignment >>>>>>> nothing more, nothing less. >>>>> >>>>> This distinction is arbitrary in practice, but I am open to being proven >>>>> wrong if you have data to back up this statement. If the driver chooses >>>>> to not follow this, then the driver might not work. ARM defines the >>>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported >>>>> alignment. If the driver chooses to pad with one byte instead of 2 >>>>> bytes, the driver may fail to work as the CPU may stall after the >>>>> misaligned access. >>>>> >>>>>> >>>>>> I'm sure I've seen code that would realign IP headers to a 4 byte >>>>>> boundary before processing them - but that might not have been in >>>>>> Linux. >>>>>> >>>>>> I'm also sure there are cpu which will fault double length misaligned >>>>>> memory transfers - which might be used to marginally speed up code. >>>>>> Assuming more than 4 byte alignment for the IP header is likely >>>>>> 'wishful thinking'. >>>>>> >>>>>> There is plenty of ethernet hardware that can only write frames >>>>>> to even boundaries and plenty of cpu that fault misaligned accesses. >>>>>> There are even cases of both on the same silicon die. >>>>>> >>>>>> You also pretty much never want a fault handler to fixup misaligned >>>>>> ethernet frames (or really anything else for that matter). >>>>>> It is always going to be better to check in the code itself. >>>>>> >>>>>> x86 has just made people 'sloppy' :-) >>>>>> >>>>>> David >>>>>> >>>>>> - >>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, >>>>>> MK1 1PT, UK >>>>>> Registration No: 1397386 (Wales) >>>>>> >>>>> >>>>> If somebody has a solution they deem to be better, I am happy to change >>>>> this test case. Otherwise, I would appreciate a maintainer resolving >>>>> this discussion and apply this fix. >>>>> >>>> Agreed. >>>> >>>> I do have a couple of patches which add explicit unaligned tests as well as >>>> corner case tests (which are intended to trigger as many carry overflows >>>> as possible). Once I get those working reliably, I'll be happy to submit >>>> them as additional tests. >>>> >>> >>> The functions definitely have to work at least with and without VLAN, >>> which means the alignment cannot be greater than 4 bytes. That's also >>> the outcome of the discussion. >> >> Thanks for completely ignoring what I've said. No. The alignment ends up >> being commonly 2 bytes. >> >> As I've said several times, network drivers do _not_ have to respect >> NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in >> them which can only DMA to a 32-bit aligned address. This means that >> the start of the ethernet header is placed at a 32-bit aligned address >> making the IP header misaligned to 32-bit. >> >> I don't see what is so difficult to understand about this... but it >> seems that my comments on this are being ignored time and time again, >> and I can only think that those who are ignoring my comments have >> some alterior motive here. >> >> -- >> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ >> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! > > I don't understand how the capabilities of some ARM drivers factor in > here. It appears that a common case for calling this function is to pass > in an IP header that is aligned along an ethernet header + NET_IP_ALIGN. > It is perfectly acceptable that some drivers don't align along > NET_IP_ALIGN, but that does not seem relevant here. > > This test case is supposed to be as true to the "general case" as > possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM > this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that > does not follow this may not be appropriately tested by this test case, > but anyone is welcome to submit additional test cases that address this > additional alignment concern. But then this test case is becoming less and less true to the "general case" with this patch, whereas your initial implementation was almost perfect as it was covering most cases, a lot more than what we get with that patch applied.
On 2/27/24 23:25, Christophe Leroy wrote: [ ... ] >> >> This test case is supposed to be as true to the "general case" as >> possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM >> this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that >> does not follow this may not be appropriately tested by this test case, >> but anyone is welcome to submit additional test cases that address this >> additional alignment concern. > > But then this test case is becoming less and less true to the "general > case" with this patch, whereas your initial implementation was almost > perfect as it was covering most cases, a lot more than what we get with > that patch applied. > NP with me if that is where people want to go. I'll simply disable checksum tests on all architectures which don't support unaligned accesses (so far it looks like that is only arm with thumb instructions, and possibly nios2). I personally find that less desirable and would have preferred a second configurable set of tests for unaligned accesses, but I have no problem with it. Guenter
CC testing On Wed, Feb 28, 2024 at 8:59 AM Guenter Roeck <linux@roeck-us.net> wrote: > On 2/27/24 23:25, Christophe Leroy wrote: > [ ... ] > >> > >> This test case is supposed to be as true to the "general case" as > >> possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM > >> this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that > >> does not follow this may not be appropriately tested by this test case, > >> but anyone is welcome to submit additional test cases that address this > >> additional alignment concern. > > > > But then this test case is becoming less and less true to the "general > > case" with this patch, whereas your initial implementation was almost > > perfect as it was covering most cases, a lot more than what we get with > > that patch applied. > > > NP with me if that is where people want to go. I'll simply disable checksum > tests on all architectures which don't support unaligned accesses (so far > it looks like that is only arm with thumb instructions, and possibly nios2). > I personally find that less desirable and would have preferred a second > configurable set of tests for unaligned accesses, but I have no problem > with it. IMHO the tests should validate the expected functionality. If a test fails, either functionality is missing or behaves wrong, or the test is wrong. What is the point of writing tests for a core functionality like network checksumming that do not match the expected functionality? Gr{oetje,eeting}s, Geert
On 2/28/24 02:15, Geert Uytterhoeven wrote: > CC testing > > On Wed, Feb 28, 2024 at 8:59 AM Guenter Roeck <linux@roeck-us.net> wrote: >> On 2/27/24 23:25, Christophe Leroy wrote: >> [ ... ] >>>> >>>> This test case is supposed to be as true to the "general case" as >>>> possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM >>>> this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that >>>> does not follow this may not be appropriately tested by this test case, >>>> but anyone is welcome to submit additional test cases that address this >>>> additional alignment concern. >>> >>> But then this test case is becoming less and less true to the "general >>> case" with this patch, whereas your initial implementation was almost >>> perfect as it was covering most cases, a lot more than what we get with >>> that patch applied. >>> >> NP with me if that is where people want to go. I'll simply disable checksum >> tests on all architectures which don't support unaligned accesses (so far >> it looks like that is only arm with thumb instructions, and possibly nios2). >> I personally find that less desirable and would have preferred a second >> configurable set of tests for unaligned accesses, but I have no problem >> with it. > > IMHO the tests should validate the expected functionality. If a test > fails, either functionality is missing or behaves wrong, or the test > is wrong. > > What is the point of writing tests for a core functionality like network > checksumming that do not match the expected functionality? > Tough one. I can't enable CONFIG_NET_TEST on nios2, parisc, and arm with THUMB enabled due to crashes or hangs in gso tests. I accept that. Downside is that I have to disable CONFIG_NET_TEST on those architectures/platforms entirely, meaning a whole class of tests are missing for those architectures. I would prefer to have a configuration option such as CONFIG_NET_GSO_TEST to let me disable the problematic tests for the affected platforms so I can run all the other network unit tests. Yes, obviously something is wrong either with the affected tests or with the implementation of the tested functionality on the affected systems, but that could be handled separately if a separate configuration option existed, and new regressions in other tests on the affected architectures could be identified as they happen. This case is similar. I'd prefer to have a separate configuration option, say, CONFIG_CHECKSUM_MISALIGNED_KUNIT, which I can disable to be able to run the common checksum tests on platforms / architectures which don't support unaligned accesses. However, as I said, if the community wants to take a harsh stance, I have no problem with just disabling groups of tests entirely on platforms which have a problem with part of it. Guenter
On Wed, 28 Feb 2024 at 23:40, Guenter Roeck <linux@roeck-us.net> wrote: > > On 2/28/24 02:15, Geert Uytterhoeven wrote: > > CC testing > > > > On Wed, Feb 28, 2024 at 8:59 AM Guenter Roeck <linux@roeck-us.net> wrote: > >> On 2/27/24 23:25, Christophe Leroy wrote: > >> [ ... ] > >>>> > >>>> This test case is supposed to be as true to the "general case" as > >>>> possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM > >>>> this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that > >>>> does not follow this may not be appropriately tested by this test case, > >>>> but anyone is welcome to submit additional test cases that address this > >>>> additional alignment concern. > >>> > >>> But then this test case is becoming less and less true to the "general > >>> case" with this patch, whereas your initial implementation was almost > >>> perfect as it was covering most cases, a lot more than what we get with > >>> that patch applied. > >>> > >> NP with me if that is where people want to go. I'll simply disable checksum > >> tests on all architectures which don't support unaligned accesses (so far > >> it looks like that is only arm with thumb instructions, and possibly nios2). > >> I personally find that less desirable and would have preferred a second > >> configurable set of tests for unaligned accesses, but I have no problem > >> with it. > > > > IMHO the tests should validate the expected functionality. If a test > > fails, either functionality is missing or behaves wrong, or the test > > is wrong. > > > > What is the point of writing tests for a core functionality like network > > checksumming that do not match the expected functionality? > > > > Tough one. I can't enable CONFIG_NET_TEST on nios2, parisc, and arm with THUMB > enabled due to crashes or hangs in gso tests. I accept that. Downside is that I > have to disable CONFIG_NET_TEST on those architectures/platforms entirely, > meaning a whole class of tests are missing for those architectures. I would > prefer to have a configuration option such as CONFIG_NET_GSO_TEST to let me > disable the problematic tests for the affected platforms so I can run all > the other network unit tests. Yes, obviously something is wrong either with > the affected tests or with the implementation of the tested functionality > on the affected systems, but that could be handled separately if a separate > configuration option existed, and new regressions in other tests on the affected > architectures could be identified as they happen. > > This case is similar. I'd prefer to have a separate configuration option, > say, CONFIG_CHECKSUM_MISALIGNED_KUNIT, which I can disable to be able to > run the common checksum tests on platforms / architectures which don't > support unaligned accesses. > > However, as I said, if the community wants to take a harsh stance, I have no > problem with just disabling groups of tests entirely on platforms which have > a problem with part of it. > > Guenter > I think the ideal solution is for there to be some official stance on the required alignment, for every architecture to support that, and for the tests to exercise it. Now, judging from the sheer number of replies in this thread, it seems like there isn't any real agreement on that. (From my quick reading of some of the checksum code, my assumption was that this was either 1- or 2- byte alignment required, with 4-byte alignment being ideal for performance reasons in most setups). If different architectures have different alignment requirements (ouch!), my feeling is that the test suite should be written to the maximum such alignment (as any non-architecture-specific code will need to align things anyway), and architectures/drivers with non-aligned buffers can have their own tests. If it turns out there are a lot of such drivers/architectures, then we can add the extra config option. I'd rather, if there is a config option to disable these tests, it be of the form ARCH_HAS_UNALIGNED_CHECKSUM to enable it, or similar. There's also the option of having the test 'skip' itself on a configuration which doesn't support it. That way it'll still show up in the list of tests, but with a description, like "Disabled due to checksum alignment requirements" or something, which may be more obvious to people debugging it later. For the gso test hangs, I think it's probably quite sensible to have a config option for the GSO tests generally. I'd be more hesitant to have a separate CONFIG_NET_GSO_FREQUENTLY_BROKEN_TESTS, which is selected automatically by a bunch of architectures. At that point, I think we need to either just fix the bugs, or start thinking about a better solution for these tests / architectures. One of the things I'm hoping to work on this year is some improvements to KUnit tooling to automatically run tests across a wider set of architectures and configs, so test authors can catch this sort of thing before even sending patches out. We can do a bit of this with the manual --arch <arch> option to kunit.py, but very few people will test things across more than a couple of architectures, and rarely will we get good testing on the less common architectures, like 32-bit ones, big endian ones, or ones with stricter alignment requirements. So we can do better there. tl;dr: I think it's a good idea for tests to sit behind config options. Obviously they shouldn't be either too broad, or too granular, but common sense usually prevails here. I'd rather not have config options explicitly for "broken" tests, though: if you have to, try to make the config option for the missing/broken feature (HAS_xxx) rather than the test if possible. Otherwise, 'skip' the test, with a suitable reason string if you can. -- David
On Wed, Feb 28, 2024 at 07:40:43AM -0800, Guenter Roeck wrote: > On 2/28/24 02:15, Geert Uytterhoeven wrote: > > CC testing > > > > On Wed, Feb 28, 2024 at 8:59 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > On 2/27/24 23:25, Christophe Leroy wrote: > > > [ ... ] > > > > > > > > > > This test case is supposed to be as true to the "general case" as > > > > > possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM > > > > > this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that > > > > > does not follow this may not be appropriately tested by this test case, > > > > > but anyone is welcome to submit additional test cases that address this > > > > > additional alignment concern. > > > > > > > > But then this test case is becoming less and less true to the "general > > > > case" with this patch, whereas your initial implementation was almost > > > > perfect as it was covering most cases, a lot more than what we get with > > > > that patch applied. > > > > > > > NP with me if that is where people want to go. I'll simply disable checksum > > > tests on all architectures which don't support unaligned accesses (so far > > > it looks like that is only arm with thumb instructions, and possibly nios2). > > > I personally find that less desirable and would have preferred a second > > > configurable set of tests for unaligned accesses, but I have no problem > > > with it. > > > > IMHO the tests should validate the expected functionality. If a test > > fails, either functionality is missing or behaves wrong, or the test > > is wrong. > > > > What is the point of writing tests for a core functionality like network > > checksumming that do not match the expected functionality? > > > > Tough one. I can't enable CONFIG_NET_TEST on nios2, parisc, and arm with THUMB > enabled due to crashes or hangs in gso tests. I accept that. Downside is that I > have to disable CONFIG_NET_TEST on those architectures/platforms entirely, > meaning a whole class of tests are missing for those architectures. I would > prefer to have a configuration option such as CONFIG_NET_GSO_TEST to let me > disable the problematic tests for the affected platforms so I can run all > the other network unit tests. Yes, obviously something is wrong either with > the affected tests or with the implementation of the tested functionality > on the affected systems, but that could be handled separately if a separate > configuration option existed, and new regressions in other tests on the affected > architectures could be identified as they happen. I think I got confused here, is this an issue with the tests included in this patch or is it unrelated? - Charlie > > This case is similar. I'd prefer to have a separate configuration option, > say, CONFIG_CHECKSUM_MISALIGNED_KUNIT, which I can disable to be able to > run the common checksum tests on platforms / architectures which don't > support unaligned accesses. > > However, as I said, if the community wants to take a harsh stance, I have no > problem with just disabling groups of tests entirely on platforms which have > a problem with part of it. > > Guenter >
On 2/29/24 11:38, Charlie Jenkins wrote: [ ... ] >> Tough one. I can't enable CONFIG_NET_TEST on nios2, parisc, and arm with THUMB >> enabled due to crashes or hangs in gso tests. I accept that. Downside is that I >> have to disable CONFIG_NET_TEST on those architectures/platforms entirely, >> meaning a whole class of tests are missing for those architectures. I would >> prefer to have a configuration option such as CONFIG_NET_GSO_TEST to let me >> disable the problematic tests for the affected platforms so I can run all >> the other network unit tests. Yes, obviously something is wrong either with >> the affected tests or with the implementation of the tested functionality >> on the affected systems, but that could be handled separately if a separate >> configuration option existed, and new regressions in other tests on the affected >> architectures could be identified as they happen. > > I think I got confused here, is this an issue with the tests included in > this patch or is it unrelated? > Unrelated. It was intended to be an example of another set of tests which suffer from a similar problem (crash on certain architectures if enabled). Sorry for the confusion. Guenter
Le 26/02/2024 à 17:44, Guenter Roeck a écrit : > On 2/26/24 03:34, Christophe Leroy wrote: >> >> >> Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : >>> The test cases for ip_fast_csum and csum_ipv6_magic were not properly >>> aligning the IP header, which were causing failures on architectures >>> that do not support misaligned accesses like some ARM platforms. To >>> solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the >>> standard alignment of an IP header and must be supported by the >>> architecture. >> >> I'm still wondering what we are really trying to fix here. >> >> All other tests are explicitely testing that it works with any alignment. >> >> Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as >> well ? I would expect it, I see no comment in arm code which explicits >> that assumption around those functions. >> >> Isn't the problem only the following line, because csum_offset is >> unaligned ? >> >> csum = *(__wsum *)(random_buf + i + csum_offset); >> >> Otherwise, if there really is an alignment issue for the IPv6 source or >> destination address, isn't it enough to perform a 32 bits alignment ? >> > > It isn't just arm. > > Question should be what alignments the functions are supposed to be able > to handle, not what they are optimized for. If byte and/or half word > alignments > are expected to be supported, there is still architecture code which would > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 > and on sh4, for example. If unaligned accesses are expected to be handled, > it would probably make sense to add a separate test case, though, to > clarify > that the test fails due to alignment issues, not due to input parameters. > When you say "Unaligned accesses are known to fail on hppa64/parisc64 and on sh4", do you mean unaligned accesses in general or do you mean ip_fast_csum() with unaligned ip header and csum_ipv6_magic() with unaligned source and dest addresses ? Because later in this thread it is said that only ARM and NIOS2 potentially have an issue. And when you say "unaligned", to what level is that ? Is it 4-bytes alignment or more or less ? Christophe
Le 28/02/2024 à 08:59, Guenter Roeck a écrit : > On 2/27/24 23:25, Christophe Leroy wrote: > [ ... ] >>> >>> This test case is supposed to be as true to the "general case" as >>> possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM >>> this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that >>> does not follow this may not be appropriately tested by this test case, >>> but anyone is welcome to submit additional test cases that address this >>> additional alignment concern. >> >> But then this test case is becoming less and less true to the "general >> case" with this patch, whereas your initial implementation was almost >> perfect as it was covering most cases, a lot more than what we get with >> that patch applied. >> > NP with me if that is where people want to go. I'll simply disable checksum > tests on all architectures which don't support unaligned accesses (so far > it looks like that is only arm with thumb instructions, and possibly > nios2). > I personally find that less desirable and would have preferred a second > configurable set of tests for unaligned accesses, but I have no problem > with it. > Can you tell more about the symptoms you encounter on ARM ? According to Russell (ARM Maintainer) it should work, quoting him below: However, that may not always be the case for incoming packets, and what saves 32-bit Arm is the ability to do unaligned loads in later revisions of the architecture, or the alignment fault handler (slow) on older revisions. NIOS2 doesn't have her how functions and relies on CONFIG_GENERIC_CSUM. Which means that ip_fast_csum() is from lib/checksum.c and is implemented using do_csum() which handles unaligned accesses by splitting accesses into smaller aligned accesses. Therefore, ip_fast_csum() shouldn't be an issue for NIOS2. Regarding csum_ipv6_magic(), NIOS2 uses the function in net/ipv6/ip6_checksum.c This function dereferences saddr and daddr with 32-bits accesses: saddr->s6_addr32[0], is that a problem when saddr and daddr are not 32-bits aligned ? Does it Oops ?
On 2/29/24 22:46, Christophe Leroy wrote: > > > Le 26/02/2024 à 17:44, Guenter Roeck a écrit : >> On 2/26/24 03:34, Christophe Leroy wrote: >>> >>> >>> Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : >>>> The test cases for ip_fast_csum and csum_ipv6_magic were not properly >>>> aligning the IP header, which were causing failures on architectures >>>> that do not support misaligned accesses like some ARM platforms. To >>>> solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the >>>> standard alignment of an IP header and must be supported by the >>>> architecture. >>> >>> I'm still wondering what we are really trying to fix here. >>> >>> All other tests are explicitely testing that it works with any alignment. >>> >>> Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as >>> well ? I would expect it, I see no comment in arm code which explicits >>> that assumption around those functions. >>> >>> Isn't the problem only the following line, because csum_offset is >>> unaligned ? >>> >>> csum = *(__wsum *)(random_buf + i + csum_offset); >>> >>> Otherwise, if there really is an alignment issue for the IPv6 source or >>> destination address, isn't it enough to perform a 32 bits alignment ? >>> >> >> It isn't just arm. >> >> Question should be what alignments the functions are supposed to be able >> to handle, not what they are optimized for. If byte and/or half word >> alignments >> are expected to be supported, there is still architecture code which would >> have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 >> and on sh4, for example. If unaligned accesses are expected to be handled, >> it would probably make sense to add a separate test case, though, to >> clarify >> that the test fails due to alignment issues, not due to input parameters. >> > > When you say "Unaligned accesses are known to fail on hppa64/parisc64 > and on sh4", do you mean unaligned accesses in general or do you mean > ip_fast_csum() with unaligned ip header and csum_ipv6_magic() with > unaligned source and dest addresses ? > > Because later in this thread it is said that only ARM and NIOS2 > potentially have an issue. > > And when you say "unaligned", to what level is that ? Is it 4-bytes > alignment or more or less ? > This e-mail chain is getting too long. Here is an attempt of a quick summary. - Someone else suggested that unaligned accesses with nios2 should fail. I since then tested and found that they pass at least for the checksum tests, while dumping "unaligned access" messages into the kernel log. Other tests (specifically gso) cause crashes, but that is unrelated. - checksum tests on sh4 fail for unaligned data because of a bug introduced to the architecture's checksum core with commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data"). The tests pass after reverting that patch. I reported this, but that revert (or a fix of the problem it introduced) has not been applied to linux-next. - Checksum tests on unaligned data fail on parisc in mainline due to a number of bugs in checksum assembler code (and with upstream qemu due to a bug in qemu's hppa64 emulation). All those issues should by now be fixed in linux-next. For reference, the following patches (SHAs from next-20240301) are needed to fix the known problems: 0568b6f0d863 parisc: Strip upper 32 bit of sum in csum_ipv6_magic for 64-bit builds 4b75b12d7050 parisc: Fix csum_ipv6_magic on 64-bit systems 4408ba75e4ba parisc: Fix csum_ipv6_magic on 32-bit systems a2abae8f0b63 parisc: Fix ip_fast_csum qemu (v8.2 and later) needs https://lore.kernel.org/all/20240217015811.1975411-1-linux@roeck-us.net/T/ for the hppa64/parisc64 tests to work with qemu. - Checksum tests on unaligned data cause a crash on arm systems with "thumb" instruction set enabled (such as mps2_defconfig and an385). I didn't bother checking if the crash is with 1-byte or 2-byte alignment. - There used to be a crash with checksum tests on m68k because of word alignment which the implementation of the unit tests for csum_ipv6_magic() did not take into account (this is fixed by the padding in struct csum_ipv6_magic_data). I don't know if this patch is needed to fix that problem or if it was since fixed differently. I hope that covers everything. As I said above, the chain is getting long and I may have missed something. I am currently re-testing on all platforms/architectures available in qemu with the known bugs outside lib/checksum_kunit.c fixed and with the sh4 patch reverted, but without this patch. I'll send an update in response to the v11 patch as soon as I have the results. Guenter
On 3/1/24 08:24, Guenter Roeck wrote: > On 2/29/24 22:46, Christophe Leroy wrote: >> >> >> Le 26/02/2024 à 17:44, Guenter Roeck a écrit : >>> On 2/26/24 03:34, Christophe Leroy wrote: >>>> >>>> >>>> Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : >>>>> The test cases for ip_fast_csum and csum_ipv6_magic were not properly >>>>> aligning the IP header, which were causing failures on architectures >>>>> that do not support misaligned accesses like some ARM platforms. To >>>>> solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the >>>>> standard alignment of an IP header and must be supported by the >>>>> architecture. >>>> >>>> I'm still wondering what we are really trying to fix here. >>>> >>>> All other tests are explicitely testing that it works with any alignment. >>>> >>>> Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as >>>> well ? I would expect it, I see no comment in arm code which explicits >>>> that assumption around those functions. >>>> >>>> Isn't the problem only the following line, because csum_offset is >>>> unaligned ? >>>> >>>> csum = *(__wsum *)(random_buf + i + csum_offset); >>>> >>>> Otherwise, if there really is an alignment issue for the IPv6 source or >>>> destination address, isn't it enough to perform a 32 bits alignment ? >>>> >>> >>> It isn't just arm. >>> >>> Question should be what alignments the functions are supposed to be able >>> to handle, not what they are optimized for. If byte and/or half word >>> alignments >>> are expected to be supported, there is still architecture code which would >>> have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 >>> and on sh4, for example. If unaligned accesses are expected to be handled, >>> it would probably make sense to add a separate test case, though, to >>> clarify >>> that the test fails due to alignment issues, not due to input parameters. >>> >> >> When you say "Unaligned accesses are known to fail on hppa64/parisc64 >> and on sh4", do you mean unaligned accesses in general or do you mean >> ip_fast_csum() with unaligned ip header and csum_ipv6_magic() with >> unaligned source and dest addresses ? >> >> Because later in this thread it is said that only ARM and NIOS2 >> potentially have an issue. >> >> And when you say "unaligned", to what level is that ? Is it 4-bytes >> alignment or more or less ? >> > > This e-mail chain is getting too long. Here is an attempt of a quick summary. > > - Someone else suggested that unaligned accesses with nios2 should fail. > I since then tested and found that they pass at least for the checksum tests, > while dumping "unaligned access" messages into the kernel log. Other tests > (specifically gso) cause crashes, but that is unrelated. > > - checksum tests on sh4 fail for unaligned data because of a bug introduced > to the architecture's checksum core with commit cadc4e1a2b4d ("sh: Handle > calling csum_partial with misaligned data"). The tests pass after reverting > that patch. I reported this, but that revert (or a fix of the problem it > introduced) has not been applied to linux-next. > > - Checksum tests on unaligned data fail on parisc in mainline due to a number > of bugs in checksum assembler code (and with upstream qemu due to a bug in > qemu's hppa64 emulation). All those issues should by now be fixed in linux-next. > For reference, the following patches (SHAs from next-20240301) are needed to fix > the known problems: > 0568b6f0d863 parisc: Strip upper 32 bit of sum in csum_ipv6_magic for 64-bit builds > 4b75b12d7050 parisc: Fix csum_ipv6_magic on 64-bit systems > 4408ba75e4ba parisc: Fix csum_ipv6_magic on 32-bit systems > a2abae8f0b63 parisc: Fix ip_fast_csum > qemu (v8.2 and later) needs > https://lore.kernel.org/all/20240217015811.1975411-1-linux@roeck-us.net/T/ > for the hppa64/parisc64 tests to work with qemu. > > - Checksum tests on unaligned data cause a crash on arm systems with "thumb" > instruction set enabled (such as mps2_defconfig and an385). I didn't bother > checking if the crash is with 1-byte or 2-byte alignment. > > - There used to be a crash with checksum tests on m68k because of word alignment > which the implementation of the unit tests for csum_ipv6_magic() did not take > into account (this is fixed by the padding in struct csum_ipv6_magic_data). > I don't know if this patch is needed to fix that problem or if it was since > fixed differently. > > I hope that covers everything. As I said above, the chain is getting long > and I may have missed something. > I knew I missed something. I forgot to mention upstream commit d55347bfe4e6 ("MIPS: Add 'memory' clobber to csum_ipv6_magic() inline assembler") witch fixed problems with csum_ipv6_magic() for mips. Guenter
diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c index bf70850035c7..e653c6a212bf 100644 --- a/lib/checksum_kunit.c +++ b/lib/checksum_kunit.c @@ -13,8 +13,9 @@ #define IPv4_MIN_WORDS 5 #define IPv4_MAX_WORDS 15 -#define NUM_IPv6_TESTS 200 -#define NUM_IP_FAST_CSUM_TESTS 181 +#define WORD_ALIGNMENT 4 +/* Ethernet headers are 14 bytes and NET_IP_ALIGN is used to align them */ +#define IP_ALIGNMENT (14 + NET_IP_ALIGN) /* Values for a little endian CPU. Byte swap each half on big endian CPU. */ static const u32 random_init_sum = 0x2847aab; @@ -216,234 +217,106 @@ static const u32 init_sums_no_overflow[] = { }; static const u16 expected_csum_ipv6_magic[] = { - 0x18d4, 0x3085, 0x2e4b, 0xd9f4, 0xbdc8, 0x78f, 0x1034, 0x8422, 0x6fc0, - 0xd2f6, 0xbeb5, 0x9d3, 0x7e2a, 0x312e, 0x778e, 0xc1bb, 0x7cf2, 0x9d1e, - 0xca21, 0xf3ff, 0x7569, 0xb02e, 0xca86, 0x7e76, 0x4539, 0x45e3, 0xf28d, - 0xdf81, 0x8fd5, 0x3b5d, 0x8324, 0xf471, 0x83be, 0x1daf, 0x8c46, 0xe682, - 0xd1fb, 0x6b2e, 0xe687, 0x2a33, 0x4833, 0x2d67, 0x660f, 0x2e79, 0xd65e, - 0x6b62, 0x6672, 0x5dbd, 0x8680, 0xbaa5, 0x2229, 0x2125, 0x2d01, 0x1cc0, - 0x6d36, 0x33c0, 0xee36, 0xd832, 0x9820, 0x8a31, 0x53c5, 0x2e2, 0xdb0e, - 0x49ed, 0x17a7, 0x77a0, 0xd72e, 0x3d72, 0x7dc8, 0x5b17, 0xf55d, 0xa4d9, - 0x1446, 0x5d56, 0x6b2e, 0x69a5, 0xadb6, 0xff2a, 0x92e, 0xe044, 0x3402, - 0xbb60, 0xec7f, 0xe7e6, 0x1986, 0x32f4, 0x8f8, 0x5e00, 0x47c6, 0x3059, - 0x3969, 0xe957, 0x4388, 0x2854, 0x3334, 0xea71, 0xa6de, 0x33f9, 0x83fc, - 0x37b4, 0x5531, 0x3404, 0x1010, 0xed30, 0x610a, 0xc95, 0x9aed, 0x6ff, - 0x5136, 0x2741, 0x660e, 0x8b80, 0xf71, 0xa263, 0x88af, 0x7a73, 0x3c37, - 0x1908, 0x6db5, 0x2e92, 0x1cd2, 0x70c8, 0xee16, 0xe80, 0xcd55, 0x6e6, - 0x6434, 0x127, 0x655d, 0x2ea0, 0xb4f4, 0xdc20, 0x5671, 0xe462, 0xe52b, - 0xdb44, 0x3589, 0xc48f, 0xe60b, 0xd2d2, 0x66ad, 0x498, 0x436, 0xb917, - 0xf0ca, 0x1a6e, 0x1cb7, 0xbf61, 0x2870, 0xc7e8, 0x5b30, 0xe4a5, 0x168, - 0xadfc, 0xd035, 0xe690, 0xe283, 0xfb27, 0xe4ad, 0xb1a5, 0xf2d5, 0xc4b6, - 0x8a30, 0xd7d5, 0x7df9, 0x91d5, 0x63ed, 0x2d21, 0x312b, 0xab19, 0xa632, - 0x8d2e, 0xef06, 0x57b9, 0xc373, 0xbd1f, 0xa41f, 0x8444, 0x9975, 0x90cb, - 0xc49c, 0xe965, 0x4eff, 0x5a, 0xef6d, 0xe81a, 0xe260, 0x853a, 0xff7a, - 0x99aa, 0xb06b, 0xee19, 0xcc2c, 0xf34c, 0x7c49, 0xdac3, 0xa71e, 0xc988, - 0x3845, 0x1014 + 0x9359, 0x5630, 0xd659, 0x5b4d, 0x511e, 0x627c, 0x4e30, 0x73f1, 0x063c, + 0xf2da, 0x007b, 0xa98b, 0x4fb7, 0x87a6, 0x2500, 0x34e4, 0xf0cd, 0xdc69, + 0x7bde, 0x73f0, 0xd85d, 0x722d, 0x9776, 0x03c8, 0x7e07, 0xdca9, 0x9ecc, + 0xc6c0, 0xbec1, 0x8de5, 0x6f7f, 0x1a09, 0xdbe6, 0x7c4b, 0x3787, 0xdb38, + 0x0fac, 0xbed9, 0x3039, 0x6501, 0xae1a, 0xed89, 0xd982, 0xc530, 0xccf6, + 0xd888, 0xf369, 0x2c4e, 0x38c0, 0xcff5, 0xdc9d, 0x5998, 0xe0d1, 0x7e23, + 0x91ae, 0x93d3, 0xa0fa, 0xd87a, 0xcd1c, 0x3e12, 0x6b81, 0x1736, 0xe8bb, + 0xdc4a, 0x4ce2, 0x08b5, 0xdc26, 0x217e, 0x74b2, 0x85e2, 0xb8bc, 0x0290, + 0x88d7, 0xc552, 0x52df, 0x6145, 0x2d3e, 0xb96f, 0x48d7, 0xff3b, 0xa30a, + 0xb8d7, 0x2030, 0xf091, 0xc4b9, 0x42b7, 0xa87d, 0x0f2c, 0xa8b3, 0x49c2, + 0x7884, 0xf86e, 0x43db, 0x402d, 0x1fab, 0xad1b, 0xf2fe, 0xdcf1, 0x6913, + 0xbfab, 0xd5c6, 0xe589, 0xcc4c, 0x1084, 0xd3c6, 0xa04a, 0x9328, 0xfa07, + 0x9c83, 0xd0d0, 0xc16b, 0x7be5, 0x7dcc, 0xe9a5, 0x4b31, 0x790b, 0x6e38, }; static const u16 expected_fast_csum[] = { - 0xda83, 0x45da, 0x4f46, 0x4e4f, 0x34e, 0xe902, 0xa5e9, 0x87a5, 0x7187, - 0x5671, 0xf556, 0x6df5, 0x816d, 0x8f81, 0xbb8f, 0xfbba, 0x5afb, 0xbe5a, - 0xedbe, 0xabee, 0x6aac, 0xe6b, 0xea0d, 0x67ea, 0x7e68, 0x8a7e, 0x6f8a, - 0x3a70, 0x9f3a, 0xe89e, 0x75e8, 0x7976, 0xfa79, 0x2cfa, 0x3c2c, 0x463c, - 0x7146, 0x7a71, 0x547a, 0xfd53, 0x99fc, 0xb699, 0x92b6, 0xdb91, 0xe8da, - 0x5fe9, 0x1e60, 0xae1d, 0x39ae, 0xf439, 0xa1f4, 0xdda1, 0xede, 0x790f, - 0x579, 0x1206, 0x9012, 0x2490, 0xd224, 0x5cd2, 0xa65d, 0xca7, 0x220d, - 0xf922, 0xbf9, 0x920b, 0x1b92, 0x361c, 0x2e36, 0x4d2e, 0x24d, 0x2, - 0xcfff, 0x90cf, 0xa591, 0x93a5, 0x7993, 0x9579, 0xc894, 0x50c8, 0x5f50, - 0xd55e, 0xcad5, 0xf3c9, 0x8f4, 0x4409, 0x5043, 0x5b50, 0x55b, 0x2205, - 0x1e22, 0x801e, 0x3780, 0xe137, 0x7ee0, 0xf67d, 0x3cf6, 0xa53c, 0x2ea5, - 0x472e, 0x5147, 0xcf51, 0x1bcf, 0x951c, 0x1e95, 0xc71e, 0xe4c7, 0xc3e4, - 0x3dc3, 0xee3d, 0xa4ed, 0xf9a4, 0xcbf8, 0x75cb, 0xb375, 0x50b4, 0x3551, - 0xf835, 0x19f8, 0x8c1a, 0x538c, 0xad52, 0xa3ac, 0xb0a3, 0x5cb0, 0x6c5c, - 0x5b6c, 0xc05a, 0x92c0, 0x4792, 0xbe47, 0x53be, 0x1554, 0x5715, 0x4b57, - 0xe54a, 0x20e5, 0x21, 0xd500, 0xa1d4, 0xa8a1, 0x57a9, 0xca57, 0x5ca, - 0x1c06, 0x4f1c, 0xe24e, 0xd9e2, 0xf0d9, 0x4af1, 0x474b, 0x8146, 0xe81, - 0xfd0e, 0x84fd, 0x7c85, 0xba7c, 0x17ba, 0x4a17, 0x964a, 0xf595, 0xff5, - 0x5310, 0x3253, 0x6432, 0x4263, 0x2242, 0xe121, 0x32e1, 0xf632, 0xc5f5, - 0x21c6, 0x7d22, 0x8e7c, 0x418e, 0x5641, 0x3156, 0x7c31, 0x737c, 0x373, - 0x2503, 0xc22a, 0x3c2, 0x4a04, 0x8549, 0x5285, 0xa352, 0xe8a3, 0x6fe8, - 0x1a6f, 0x211a, 0xe021, 0x38e0, 0x7638, 0xf575, 0x9df5, 0x169e, 0xf116, - 0x23f1, 0xcd23, 0xece, 0x660f, 0x4866, 0x6a48, 0x716a, 0xee71, 0xa2ee, - 0xb8a2, 0x61b9, 0xa361, 0xf7a2, 0x26f7, 0x1127, 0x6611, 0xe065, 0x36e0, - 0x1837, 0x3018, 0x1c30, 0x721b, 0x3e71, 0xe43d, 0x99e4, 0x9e9a, 0xb79d, - 0xa9b7, 0xcaa, 0xeb0c, 0x4eb, 0x1305, 0x8813, 0xb687, 0xa9b6, 0xfba9, - 0xd7fb, 0xccd8, 0x2ecd, 0x652f, 0xae65, 0x3fae, 0x3a40, 0x563a, 0x7556, - 0x2776, 0x1228, 0xef12, 0xf9ee, 0xcef9, 0x56cf, 0xa956, 0x24a9, 0xba24, - 0x5fba, 0x665f, 0xf465, 0x8ff4, 0x6d8f, 0x346d, 0x5f34, 0x385f, 0xd137, - 0xb8d0, 0xacb8, 0x55ac, 0x7455, 0xe874, 0x89e8, 0xd189, 0xa0d1, 0xb2a0, - 0xb8b2, 0x36b8, 0x5636, 0xd355, 0x8d3, 0x1908, 0x2118, 0xc21, 0x990c, - 0x8b99, 0x158c, 0x7815, 0x9e78, 0x6f9e, 0x4470, 0x1d44, 0x341d, 0x2634, - 0x3f26, 0x793e, 0xc79, 0xcc0b, 0x26cc, 0xd126, 0x1fd1, 0xb41f, 0xb6b4, - 0x22b7, 0xa122, 0xa1, 0x7f01, 0x837e, 0x3b83, 0xaf3b, 0x6fae, 0x916f, - 0xb490, 0xffb3, 0xceff, 0x50cf, 0x7550, 0x7275, 0x1272, 0x2613, 0xaa26, - 0xd5aa, 0x7d5, 0x9607, 0x96, 0xb100, 0xf8b0, 0x4bf8, 0xdd4c, 0xeddd, - 0x98ed, 0x2599, 0x9325, 0xeb92, 0x8feb, 0xcc8f, 0x2acd, 0x392b, 0x3b39, - 0xcb3b, 0x6acb, 0xd46a, 0xb8d4, 0x6ab8, 0x106a, 0x2f10, 0x892f, 0x789, - 0xc806, 0x45c8, 0x7445, 0x3c74, 0x3a3c, 0xcf39, 0xd7ce, 0x58d8, 0x6e58, - 0x336e, 0x1034, 0xee10, 0xe9ed, 0xc2e9, 0x3fc2, 0xd53e, 0xd2d4, 0xead2, - 0x8fea, 0x2190, 0x1162, 0xbe11, 0x8cbe, 0x6d8c, 0xfb6c, 0x6dfb, 0xd36e, - 0x3ad3, 0xf3a, 0x870e, 0xc287, 0x53c3, 0xc54, 0x5b0c, 0x7d5a, 0x797d, - 0xec79, 0x5dec, 0x4d5e, 0x184e, 0xd618, 0x60d6, 0xb360, 0x98b3, 0xf298, - 0xb1f2, 0x69b1, 0xf969, 0xef9, 0xab0e, 0x21ab, 0xe321, 0x24e3, 0x8224, - 0x5481, 0x5954, 0x7a59, 0xff7a, 0x7dff, 0x1a7d, 0xa51a, 0x46a5, 0x6b47, - 0xe6b, 0x830e, 0xa083, 0xff9f, 0xd0ff, 0xffd0, 0xe6ff, 0x7de7, 0xc67d, - 0xd0c6, 0x61d1, 0x3a62, 0xc3b, 0x150c, 0x1715, 0x4517, 0x5345, 0x3954, - 0xdd39, 0xdadd, 0x32db, 0x6a33, 0xd169, 0x86d1, 0xb687, 0x3fb6, 0x883f, - 0xa487, 0x39a4, 0x2139, 0xbe20, 0xffbe, 0xedfe, 0x8ded, 0x368e, 0xc335, - 0x51c3, 0x9851, 0xf297, 0xd6f2, 0xb9d6, 0x95ba, 0x2096, 0xea1f, 0x76e9, - 0x4e76, 0xe04d, 0xd0df, 0x80d0, 0xa280, 0xfca2, 0x75fc, 0xef75, 0x32ef, - 0x6833, 0xdf68, 0xc4df, 0x76c4, 0xb77, 0xb10a, 0xbfb1, 0x58bf, 0x5258, - 0x4d52, 0x6c4d, 0x7e6c, 0xb67e, 0xccb5, 0x8ccc, 0xbe8c, 0xc8bd, 0x9ac8, - 0xa99b, 0x52a9, 0x2f53, 0xc30, 0x3e0c, 0xb83d, 0x83b7, 0x5383, 0x7e53, - 0x4f7e, 0xe24e, 0xb3e1, 0x8db3, 0x618e, 0xc861, 0xfcc8, 0x34fc, 0x9b35, - 0xaa9b, 0xb1aa, 0x5eb1, 0x395e, 0x8639, 0xd486, 0x8bd4, 0x558b, 0x2156, - 0xf721, 0x4ef6, 0x14f, 0x7301, 0xdd72, 0x49de, 0x894a, 0x9889, 0x8898, - 0x7788, 0x7b77, 0x637b, 0xb963, 0xabb9, 0x7cab, 0xc87b, 0x21c8, 0xcb21, - 0xdfca, 0xbfdf, 0xf2bf, 0x6af2, 0x626b, 0xb261, 0x3cb2, 0xc63c, 0xc9c6, - 0xc9c9, 0xb4c9, 0xf9b4, 0x91f9, 0x4091, 0x3a40, 0xcc39, 0xd1cb, 0x7ed1, - 0x537f, 0x6753, 0xa167, 0xba49, 0x88ba, 0x7789, 0x3877, 0xf037, 0xd3ef, - 0xb5d4, 0x55b6, 0xa555, 0xeca4, 0xa1ec, 0xb6a2, 0x7b7, 0x9507, 0xfd94, - 0x82fd, 0x5c83, 0x765c, 0x9676, 0x3f97, 0xda3f, 0x6fda, 0x646f, 0x3064, - 0x5e30, 0x655e, 0x6465, 0xcb64, 0xcdca, 0x4ccd, 0x3f4c, 0x243f, 0x6f24, - 0x656f, 0x6065, 0x3560, 0x3b36, 0xac3b, 0x4aac, 0x714a, 0x7e71, 0xda7e, - 0x7fda, 0xda7f, 0x6fda, 0xff6f, 0xc6ff, 0xedc6, 0xd4ed, 0x70d5, 0xeb70, - 0xa3eb, 0x80a3, 0xca80, 0x3fcb, 0x2540, 0xf825, 0x7ef8, 0xf87e, 0x73f8, - 0xb474, 0xb4b4, 0x92b5, 0x9293, 0x93, 0x3500, 0x7134, 0x9071, 0xfa8f, - 0x51fa, 0x1452, 0xba13, 0x7ab9, 0x957a, 0x8a95, 0x6e8a, 0x6d6e, 0x7c6d, - 0x447c, 0x9744, 0x4597, 0x8945, 0xef88, 0x8fee, 0x3190, 0x4831, 0x8447, - 0xa183, 0x1da1, 0xd41d, 0x2dd4, 0x4f2e, 0xc94e, 0xcbc9, 0xc9cb, 0x9ec9, - 0x319e, 0xd531, 0x20d5, 0x4021, 0xb23f, 0x29b2, 0xd828, 0xecd8, 0x5ded, - 0xfc5d, 0x4dfc, 0xd24d, 0x6bd2, 0x5f6b, 0xb35e, 0x7fb3, 0xee7e, 0x56ee, - 0xa657, 0x68a6, 0x8768, 0x7787, 0xb077, 0x4cb1, 0x764c, 0xb175, 0x7b1, - 0x3d07, 0x603d, 0x3560, 0x3e35, 0xb03d, 0xd6b0, 0xc8d6, 0xd8c8, 0x8bd8, - 0x3e8c, 0x303f, 0xd530, 0xf1d4, 0x42f1, 0xca42, 0xddca, 0x41dd, 0x3141, - 0x132, 0xe901, 0x8e9, 0xbe09, 0xe0bd, 0x2ce0, 0x862d, 0x3986, 0x9139, - 0x6d91, 0x6a6d, 0x8d6a, 0x1b8d, 0xac1b, 0xedab, 0x54ed, 0xc054, 0xcebf, - 0xc1ce, 0x5c2, 0x3805, 0x6038, 0x5960, 0xd359, 0xdd3, 0xbe0d, 0xafbd, - 0x6daf, 0x206d, 0x2c20, 0x862c, 0x8e86, 0xec8d, 0xa2ec, 0xa3a2, 0x51a3, - 0x8051, 0xfd7f, 0x91fd, 0xa292, 0xaf14, 0xeeae, 0x59ef, 0x535a, 0x8653, - 0x3986, 0x9539, 0xb895, 0xa0b8, 0x26a0, 0x2227, 0xc022, 0x77c0, 0xad77, - 0x46ad, 0xaa46, 0x60aa, 0x8560, 0x4785, 0xd747, 0x45d7, 0x2346, 0x5f23, - 0x25f, 0x1d02, 0x71d, 0x8206, 0xc82, 0x180c, 0x3018, 0x4b30, 0x4b, - 0x3001, 0x1230, 0x2d12, 0x8c2d, 0x148d, 0x4015, 0x5f3f, 0x3d5f, 0x6b3d, - 0x396b, 0x473a, 0xf746, 0x44f7, 0x8945, 0x3489, 0xcb34, 0x84ca, 0xd984, - 0xf0d9, 0xbcf0, 0x63bd, 0x3264, 0xf332, 0x45f3, 0x7346, 0x5673, 0xb056, - 0xd3b0, 0x4ad4, 0x184b, 0x7d18, 0x6c7d, 0xbb6c, 0xfeba, 0xe0fe, 0x10e1, - 0x5410, 0x2954, 0x9f28, 0x3a9f, 0x5a3a, 0xdb59, 0xbdc, 0xb40b, 0x1ab4, - 0x131b, 0x5d12, 0x6d5c, 0xe16c, 0xb0e0, 0x89b0, 0xba88, 0xbb, 0x3c01, - 0xe13b, 0x6fe1, 0x446f, 0xa344, 0x81a3, 0xfe81, 0xc7fd, 0x38c8, 0xb38, - 0x1a0b, 0x6d19, 0xf36c, 0x47f3, 0x6d48, 0xb76d, 0xd3b7, 0xd8d2, 0x52d9, - 0x4b53, 0xa54a, 0x34a5, 0xc534, 0x9bc4, 0xed9b, 0xbeed, 0x3ebe, 0x233e, - 0x9f22, 0x4a9f, 0x774b, 0x4577, 0xa545, 0x64a5, 0xb65, 0x870b, 0x487, - 0x9204, 0x5f91, 0xd55f, 0x35d5, 0x1a35, 0x71a, 0x7a07, 0x4e7a, 0xfc4e, - 0x1efc, 0x481f, 0x7448, 0xde74, 0xa7dd, 0x1ea7, 0xaa1e, 0xcfaa, 0xfbcf, - 0xedfb, 0x6eee, 0x386f, 0x4538, 0x6e45, 0xd96d, 0x11d9, 0x7912, 0x4b79, - 0x494b, 0x6049, 0xac5f, 0x65ac, 0x1366, 0x5913, 0xe458, 0x7ae4, 0x387a, - 0x3c38, 0xb03c, 0x76b0, 0x9376, 0xe193, 0x42e1, 0x7742, 0x6476, 0x3564, - 0x3c35, 0x6a3c, 0xcc69, 0x94cc, 0x5d95, 0xe5e, 0xee0d, 0x4ced, 0xce4c, - 0x52ce, 0xaa52, 0xdaaa, 0xe4da, 0x1de5, 0x4530, 0x5445, 0x3954, 0xb639, - 0x81b6, 0x7381, 0x1574, 0xc215, 0x10c2, 0x3f10, 0x6b3f, 0xe76b, 0x7be7, - 0xbc7b, 0xf7bb, 0x41f7, 0xcc41, 0x38cc, 0x4239, 0xa942, 0x4a9, 0xc504, - 0x7cc4, 0x437c, 0x6743, 0xea67, 0x8dea, 0xe88d, 0xd8e8, 0xdcd8, 0x17dd, - 0x5718, 0x958, 0xa609, 0x41a5, 0x5842, 0x159, 0x9f01, 0x269f, 0x5a26, - 0x405a, 0xc340, 0xb4c3, 0xd4b4, 0xf4d3, 0xf1f4, 0x39f2, 0xe439, 0x67e4, - 0x4168, 0xa441, 0xdda3, 0xdedd, 0x9df, 0xab0a, 0xa5ab, 0x9a6, 0xba09, - 0x9ab9, 0xad9a, 0x5ae, 0xe205, 0xece2, 0xecec, 0x14ed, 0xd614, 0x6bd5, - 0x916c, 0x3391, 0x6f33, 0x206f, 0x8020, 0x780, 0x7207, 0x2472, 0x8a23, - 0xb689, 0x3ab6, 0xf739, 0x97f6, 0xb097, 0xa4b0, 0xe6a4, 0x88e6, 0x2789, - 0xb28, 0x350b, 0x1f35, 0x431e, 0x1043, 0xc30f, 0x79c3, 0x379, 0x5703, - 0x3256, 0x4732, 0x7247, 0x9d72, 0x489d, 0xd348, 0xa4d3, 0x7ca4, 0xbf7b, - 0x45c0, 0x7b45, 0x337b, 0x4034, 0x843f, 0xd083, 0x35d0, 0x6335, 0x4d63, - 0xe14c, 0xcce0, 0xfecc, 0x35ff, 0x5636, 0xf856, 0xeef8, 0x2def, 0xfc2d, - 0x4fc, 0x6e04, 0xb66d, 0x78b6, 0xbb78, 0x3dbb, 0x9a3d, 0x839a, 0x9283, - 0x593, 0xd504, 0x23d5, 0x5424, 0xd054, 0x61d0, 0xdb61, 0x17db, 0x1f18, - 0x381f, 0x9e37, 0x679e, 0x1d68, 0x381d, 0x8038, 0x917f, 0x491, 0xbb04, - 0x23bb, 0x4124, 0xd41, 0xa30c, 0x8ba3, 0x8b8b, 0xc68b, 0xd2c6, 0xebd2, - 0x93eb, 0xbd93, 0x99bd, 0x1a99, 0xea19, 0x58ea, 0xcf58, 0x73cf, 0x1073, - 0x9e10, 0x139e, 0xea13, 0xcde9, 0x3ecd, 0x883f, 0xf89, 0x180f, 0x2a18, - 0x212a, 0xce20, 0x73ce, 0xf373, 0x60f3, 0xad60, 0x4093, 0x8e40, 0xb98e, - 0xbfb9, 0xf1bf, 0x8bf1, 0x5e8c, 0xe95e, 0x14e9, 0x4e14, 0x1c4e, 0x7f1c, - 0xe77e, 0x6fe7, 0xf26f, 0x13f2, 0x8b13, 0xda8a, 0x5fda, 0xea5f, 0x4eea, - 0xa84f, 0x88a8, 0x1f88, 0x2820, 0x9728, 0x5a97, 0x3f5b, 0xb23f, 0x70b2, - 0x2c70, 0x232d, 0xf623, 0x4f6, 0x905, 0x7509, 0xd675, 0x28d7, 0x9428, - 0x3794, 0xf036, 0x2bf0, 0xba2c, 0xedb9, 0xd7ed, 0x59d8, 0xed59, 0x4ed, - 0xe304, 0x18e3, 0x5c19, 0x3d5c, 0x753d, 0x6d75, 0x956d, 0x7f95, 0xc47f, - 0x83c4, 0xa84, 0x2e0a, 0x5f2e, 0xb95f, 0x77b9, 0x6d78, 0xf46d, 0x1bf4, - 0xed1b, 0xd6ed, 0xe0d6, 0x5e1, 0x3905, 0x5638, 0xa355, 0x99a2, 0xbe99, - 0xb4bd, 0x85b4, 0x2e86, 0x542e, 0x6654, 0xd765, 0x73d7, 0x3a74, 0x383a, - 0x2638, 0x7826, 0x7677, 0x9a76, 0x7e99, 0x2e7e, 0xea2d, 0xa6ea, 0x8a7, - 0x109, 0x3300, 0xad32, 0x5fad, 0x465f, 0x2f46, 0xc62f, 0xd4c5, 0xad5, - 0xcb0a, 0x4cb, 0xb004, 0x7baf, 0xe47b, 0x92e4, 0x8e92, 0x638e, 0x1763, - 0xc17, 0xf20b, 0x1ff2, 0x8920, 0x5889, 0xcb58, 0xf8cb, 0xcaf8, 0x84cb, - 0x9f84, 0x8a9f, 0x918a, 0x4991, 0x8249, 0xff81, 0x46ff, 0x5046, 0x5f50, - 0x725f, 0xf772, 0x8ef7, 0xe08f, 0xc1e0, 0x1fc2, 0x9e1f, 0x8b9d, 0x108b, - 0x411, 0x2b04, 0xb02a, 0x1fb0, 0x1020, 0x7a0f, 0x587a, 0x8958, 0xb188, - 0xb1b1, 0x49b2, 0xb949, 0x7ab9, 0x917a, 0xfc91, 0xe6fc, 0x47e7, 0xbc47, - 0x8fbb, 0xea8e, 0x34ea, 0x2635, 0x1726, 0x9616, 0xc196, 0xa6c1, 0xf3a6, - 0x11f3, 0x4811, 0x3e48, 0xeb3e, 0xf7ea, 0x1bf8, 0xdb1c, 0x8adb, 0xe18a, - 0x42e1, 0x9d42, 0x5d9c, 0x6e5d, 0x286e, 0x4928, 0x9a49, 0xb09c, 0xa6b0, - 0x2a7, 0xe702, 0xf5e6, 0x9af5, 0xf9b, 0x810f, 0x8080, 0x180, 0x1702, - 0x5117, 0xa650, 0x11a6, 0x1011, 0x550f, 0xd554, 0xbdd5, 0x6bbe, 0xc66b, - 0xfc7, 0x5510, 0x5555, 0x7655, 0x177, 0x2b02, 0x6f2a, 0xb70, 0x9f0b, - 0xcf9e, 0xf3cf, 0x3ff4, 0xcb40, 0x8ecb, 0x768e, 0x5277, 0x8652, 0x9186, - 0x9991, 0x5099, 0xd350, 0x93d3, 0x6d94, 0xe6d, 0x530e, 0x3153, 0xa531, - 0x64a5, 0x7964, 0x7c79, 0x467c, 0x1746, 0x3017, 0x3730, 0x538, 0x5, - 0x1e00, 0x5b1e, 0x955a, 0xae95, 0x3eaf, 0xff3e, 0xf8ff, 0xb2f9, 0xa1b3, - 0xb2a1, 0x5b2, 0xad05, 0x7cac, 0x2d7c, 0xd32c, 0x80d2, 0x7280, 0x8d72, - 0x1b8e, 0x831b, 0xac82, 0xfdac, 0xa7fd, 0x15a8, 0xd614, 0xe0d5, 0x7be0, - 0xb37b, 0x61b3, 0x9661, 0x9d95, 0xc79d, 0x83c7, 0xd883, 0xead7, 0xceb, - 0xf60c, 0xa9f5, 0x19a9, 0xa019, 0x8f9f, 0xd48f, 0x3ad5, 0x853a, 0x985, - 0x5309, 0x6f52, 0x1370, 0x6e13, 0xa96d, 0x98a9, 0x5198, 0x9f51, 0xb69f, - 0xa1b6, 0x2ea1, 0x672e, 0x2067, 0x6520, 0xaf65, 0x6eaf, 0x7e6f, 0xee7e, - 0x17ef, 0xa917, 0xcea8, 0x9ace, 0xff99, 0x5dff, 0xdf5d, 0x38df, 0xa39, - 0x1c0b, 0xe01b, 0x46e0, 0xcb46, 0x90cb, 0xba90, 0x4bb, 0x9104, 0x9d90, - 0xc89c, 0xf6c8, 0x6cf6, 0x886c, 0x1789, 0xbd17, 0x70bc, 0x7e71, 0x17e, - 0x1f01, 0xa01f, 0xbaa0, 0x14bb, 0xfc14, 0x7afb, 0xa07a, 0x3da0, 0xbf3d, - 0x48bf, 0x8c48, 0x968b, 0x9d96, 0xfd9d, 0x96fd, 0x9796, 0x6b97, 0xd16b, - 0xf4d1, 0x3bf4, 0x253c, 0x9125, 0x6691, 0xc166, 0x34c1, 0x5735, 0x1a57, - 0xdc19, 0x77db, 0x8577, 0x4a85, 0x824a, 0x9182, 0x7f91, 0xfd7f, 0xb4c3, - 0xb5b4, 0xb3b5, 0x7eb3, 0x617e, 0x4e61, 0xa4f, 0x530a, 0x3f52, 0xa33e, - 0x34a3, 0x9234, 0xf091, 0xf4f0, 0x1bf5, 0x311b, 0x9631, 0x6a96, 0x386b, - 0x1d39, 0xe91d, 0xe8e9, 0x69e8, 0x426a, 0xee42, 0x89ee, 0x368a, 0x2837, - 0x7428, 0x5974, 0x6159, 0x1d62, 0x7b1d, 0xf77a, 0x7bf7, 0x6b7c, 0x696c, - 0xf969, 0x4cf9, 0x714c, 0x4e71, 0x6b4e, 0x256c, 0x6e25, 0xe96d, 0x94e9, - 0x8f94, 0x3e8f, 0x343e, 0x4634, 0xb646, 0x97b5, 0x8997, 0xe8a, 0x900e, - 0x8090, 0xfd80, 0xa0fd, 0x16a1, 0xf416, 0xebf4, 0x95ec, 0x1196, 0x8911, - 0x3d89, 0xda3c, 0x9fd9, 0xd79f, 0x4bd7, 0x214c, 0x3021, 0x4f30, 0x994e, - 0x5c99, 0x6f5d, 0x326f, 0xab31, 0x6aab, 0xe969, 0x90e9, 0x1190, 0xff10, - 0xa2fe, 0xe0a2, 0x66e1, 0x4067, 0x9e3f, 0x2d9e, 0x712d, 0x8170, 0xd180, - 0xffd1, 0x25ff, 0x3826, 0x2538, 0x5f24, 0xc45e, 0x1cc4, 0xdf1c, 0x93df, - 0xc793, 0x80c7, 0x2380, 0xd223, 0x7ed2, 0xfc7e, 0x22fd, 0x7422, 0x1474, - 0xb714, 0x7db6, 0x857d, 0xa85, 0xa60a, 0x88a6, 0x4289, 0x7842, 0xc278, - 0xf7c2, 0xcdf7, 0x84cd, 0xae84, 0x8cae, 0xb98c, 0x1aba, 0x4d1a, 0x884c, - 0x4688, 0xcc46, 0xd8cb, 0x2bd9, 0xbe2b, 0xa2be, 0x72a2, 0xf772, 0xd2f6, - 0x75d2, 0xc075, 0xa3c0, 0x63a3, 0xae63, 0x8fae, 0x2a90, 0x5f2a, 0xef5f, - 0x5cef, 0xa05c, 0x89a0, 0x5e89, 0x6b5e, 0x736b, 0x773, 0x9d07, 0xe99c, - 0x27ea, 0x2028, 0xc20, 0x980b, 0x4797, 0x2848, 0x9828, 0xc197, 0x48c2, - 0x2449, 0x7024, 0x570, 0x3e05, 0xd3e, 0xf60c, 0xbbf5, 0x69bb, 0x3f6a, - 0x740, 0xf006, 0xe0ef, 0xbbe0, 0xadbb, 0x56ad, 0xcf56, 0xbfce, 0xa9bf, - 0x205b, 0x6920, 0xae69, 0x50ae, 0x2050, 0xf01f, 0x27f0, 0x9427, 0x8993, - 0x8689, 0x4087, 0x6e40, 0xb16e, 0xa1b1, 0xe8a1, 0x87e8, 0x6f88, 0xfe6f, - 0x4cfe, 0xe94d, 0xd5e9, 0x47d6, 0x3148, 0x5f31, 0xc35f, 0x13c4, 0xa413, - 0x5a5, 0x2405, 0xc223, 0x66c2, 0x3667, 0x5e37, 0x5f5e, 0x2f5f, 0x8c2f, - 0xe48c, 0xd0e4, 0x4d1, 0xd104, 0xe4d0, 0xcee4, 0xfcf, 0x480f, 0xa447, - 0x5ea4, 0xff5e, 0xbefe, 0x8dbe, 0x1d8e, 0x411d, 0x1841, 0x6918, 0x5469, - 0x1155, 0xc611, 0xaac6, 0x37ab, 0x2f37, 0xca2e, 0x87ca, 0xbd87, 0xabbd, - 0xb3ab, 0xcb4, 0xce0c, 0xfccd, 0xa5fd, 0x72a5, 0xf072, 0x83f0, 0xfe83, - 0x97fd, 0xc997, 0xb0c9, 0xadb0, 0xe6ac, 0x88e6, 0x1088, 0xbe10, 0x16be, - 0xa916, 0xa3a8, 0x46a3, 0x5447, 0xe953, 0x84e8, 0x2085, 0xa11f, 0xfa1, - 0xdd0f, 0xbedc, 0x5abe, 0x805a, 0xc97f, 0x6dc9, 0x826d, 0x4a82, 0x934a, - 0x5293, 0xd852, 0xd3d8, 0xadd3, 0xf4ad, 0xf3f4, 0xfcf3, 0xfefc, 0xcafe, - 0xb7ca, 0x3cb8, 0xa13c, 0x18a1, 0x1418, 0xea13, 0x91ea, 0xf891, 0x53f8, - 0xa254, 0xe9a2, 0x87ea, 0x4188, 0x1c41, 0xdc1b, 0xf5db, 0xcaf5, 0x45ca, - 0x6d45, 0x396d, 0xde39, 0x90dd, 0x1e91, 0x1e, 0x7b00, 0x6a7b, 0xa46a, - 0xc9a3, 0x9bc9, 0x389b, 0x1139, 0x5211, 0x1f52, 0xeb1f, 0xabeb, 0x48ab, - 0x9348, 0xb392, 0x17b3, 0x1618, 0x5b16, 0x175b, 0xdc17, 0xdedb, 0x1cdf, - 0xeb1c, 0xd1ea, 0x4ad2, 0xd4b, 0xc20c, 0x24c2, 0x7b25, 0x137b, 0x8b13, - 0x618b, 0xa061, 0xff9f, 0xfffe, 0x72ff, 0xf572, 0xe2f5, 0xcfe2, 0xd2cf, - 0x75d3, 0x6a76, 0xc469, 0x1ec4, 0xfc1d, 0x59fb, 0x455a, 0x7a45, 0xa479, - 0xb7a4 + 0xda83, 0xc22a, 0x1162, 0xba49, 0xaf14, 0x4530, 0x4093, 0xb09c, 0xb4c3, + 0x205b, 0xdf2c, 0x034e, 0x5285, 0xfb6c, 0xf037, 0x8653, 0x81b6, 0xf1bf, + 0xf5e6, 0x617e, 0x2050, 0x6a91, 0x7187, 0x1a6f, 0x0f3a, 0xa555, 0xa0b8, + 0x10c2, 0x14e9, 0x8080, 0x3f52, 0x8993, 0x4a70, 0x816d, 0x7638, 0x0c54, + 0x07b7, 0x77c0, 0x7be7, 0xe77e, 0xa650, 0xf091, 0xb16e, 0x8ac5, 0x5afb, + 0xf116, 0xec79, 0x5c83, 0x60aa, 0xcc41, 0x8b13, 0xd554, 0x9631, 0x6f88, + 0x5c54, 0x6aac, 0x660f, 0xd618, 0xda3f, 0x45d7, 0x04a9, 0x4eea, 0x0fc7, + 0xe91d, 0xd5e9, 0xab06, 0x7e68, 0xee71, 0xf298, 0x5e30, 0x1d02, 0x6743, + 0x2820, 0x0177, 0xee42, 0xc35f, 0x733c, 0x9f3a, 0xa361, 0x0ef9, 0xcdca, + 0x180c, 0xd8e8, 0xb23f, 0x9f0b, 0x7428, 0x2405, 0x071f, 0xfa79, 0x6611, + 0x24e3, 0x6f24, 0x3001, 0x0958, 0xf623, 0xcb40, 0x7b1d, 0x5e37, 0xd957, + 0x7146, 0x3018, 0x7a59, 0x3b36, 0x148d, 0x0159, 0xd675, 0x8652, 0x696c, + 0xe48c, 0x7aec, 0x99fc, 0xe43d, 0xa51a, 0x7e71, 0x6b3d, 0x405a, 0xf036, + 0xd350, 0x4e71, 0xe4d0, 0x9faa, 0xe8da, 0xa9b7, 0x830e, 0x6fda, 0x44f7, + 0xf4d3, 0xd7ed, 0x530e, 0xe96d, 0xa447, 0xfdc7, 0x39ae, 0x1305, 0xffd0, + 0xd4ed, 0x84ca, 0x67e4, 0xe304, 0x7964, 0x343e, 0x8dbe, 0x6d3f, 0x0ede, + 0xfba9, 0xd0c6, 0x80a3, 0x63bd, 0xdedd, 0x753d, 0x3017, 0x8997, 0x6918, + 0x165e, 0x9012, 0x652f, 0x150c, 0xf825, 0x7346, 0x09a6, 0xc47f, 0x1e00, + 0xfd80, 0xaac6, 0x469c, 0xa65d, 0x563a, 0x3954, 0xb474, 0x4ad4, 0x05ae, + 0x5f2e, 0x3eaf, 0xebf4, 0x87ca, 0x56f5, 0x0bf9, 0xef12, 0x6a33, 0x0093, + 0xbb6c, 0x14ed, 0xf46d, 0xa1b3, 0x3d89, 0x0cb4, 0x3382, 0x2e36, 0xa956, + 0x3fb6, 0xfa8f, 0x5410, 0x3391, 0xe0d6, 0x7cac, 0x4bd7, 0x72a5, 0x7154, + 0xcfff, 0x665f, 0x2139, 0x7ab9, 0x5a3a, 0x0780, 0xa355, 0x7280, 0x994e, + 0x97fd, 0xd378, 0x7993, 0x346d, 0x8ded, 0x6d6e, 0x1ab4, 0xb689, 0x85b4, + 0xac82, 0xab31, 0xe6ac, 0xebda, 0x5f50, 0xb8d0, 0x9851, 0x4597, 0xe16c, + 0xb097, 0xd765, 0xd614, 0x1190, 0x16be, 0x0424, 0x08f4, 0xe874, 0x95ba, + 0x3190, 0x00bb, 0x2789, 0x2638, 0x61b3, 0x66e1, 0x5447, 0x8439, 0x055b, + 0xb2a0, 0x4e76, 0x1da1, 0x446f, 0x431e, 0x7e99, 0x83c7, 0x712d, 0xa11f, + 0xd5de, 0x3780, 0xd355, 0xa280, 0xc94e, 0xc7fd, 0x0379, 0x08a7, 0xf60c, + 0x25ff, 0x5abe, 0x18cd, 0x3cf6, 0x0c21, 0x32ef, 0x319e, 0x6d19, 0x7247, + 0x5fad, 0x8f9f, 0xc45e, 0x826d, 0x932c, 0x5147, 0x7815, 0x76c4, 0xb23f, + 0xb76d, 0xa4d3, 0xd4c5, 0x0985, 0xc793, 0xd852, 0x4d75, 0x1e95, 0x1d44, + 0x58bf, 0x5ded, 0x4b53, 0x7b45, 0xb004, 0x6e13, 0x7ed2, 0xf3f4, 0x974b, + 0x3dc3, 0x793e, 0x7e6c, 0x6bd2, 0x9bc4, 0xd083, 0x8e92, 0x9f51, 0x1474, + 0xb7ca, 0xc15d, 0xcbf8, 0xd126, 0xbe8c, 0xee7e, 0x233e, 0xe14c, 0xf20b, + 0x672e, 0x0a85, 0x1418, 0xefcd, 0x3551, 0x22b7, 0x52a9, 0x8768, 0x4577, + 0x5636, 0xcb58, 0x6eaf, 0x7842, 0x53f8, 0x10b3, 0x538c, 0x837e, 0xb83d, + 0x764c, 0x870b, 0xfc2d, 0x9f84, 0xa917, 0x84cd, 0x4188, 0xf1c3, 0x5cb0, + 0x916f, 0x4f7e, 0x603d, 0xd55f, 0x78b6, 0x8249, 0x5dff, 0x1aba, 0xcaf5, + 0xdce8, 0x92c0, 0x50cf, 0x618e, 0xd6b0, 0x7a07, 0x839a, 0x5f50, 0x1c0b, + 0xcc46, 0xde39, 0xb67b, 0x1554, 0x2613, 0x9b35, 0x3e8c, 0x481f, 0x23d5, + 0xe08f, 0x90cb, 0xa2be, 0x7b00, 0xa0f7, 0x20e5, 0x9607, 0x395e, 0x42f1, + 0x1ea7, 0xdb61, 0x8b9d, 0x9d90, 0x75d2, 0x9bc9, 0x0cb8, 0xa8a1, 0x4bf8, + 0x558b, 0x3141, 0xedfb, 0x9e37, 0xb02a, 0x886c, 0xae63, 0x1f52, 0xc33a, + 0x1c06, 0x2599, 0x014f, 0xbe09, 0x6e45, 0x8038, 0x587a, 0x7e71, 0xef5f, + 0x9348, 0x8fd5, 0xf0d9, 0xcc8f, 0x894a, 0x3986, 0x4b79, 0x23bb, 0x49b2, + 0xbaa0, 0x5e89, 0x5b16, 0xdaed, 0x0e81, 0xcb3b, 0x7b77, 0x8d6a, 0x65ac, + 0x8ba3, 0xfc91, 0xa07a, 0x9d07, 0x1cdf, 0x1e0a, 0xba7c, 0x6ab8, 0x7cab, + 0x54ed, 0x7ae4, 0xebd2, 0x8fbb, 0x8c48, 0x0c20, 0x0d4b, 0x889d, 0xf595, + 0x0789, 0xdfca, 0x05c2, 0x76b0, 0x1a99, 0x1726, 0x96fd, 0x9828, 0x137b, + 0xa2f6, 0x6432, 0x3c74, 0x626b, 0xd359, 0x7742, 0x73cf, 0xf3a6, 0xf4d1, + 0x7024, 0xff9f, 0xec88, 0x32e1, 0x58d8, 0xc9c6, 0x6daf, 0x6a3c, 0xea13, + 0xeb3e, 0x6691, 0xf60c, 0xe2f5, 0x462c, 0x7d22, 0xee10, 0x91f9, 0x8e86, + 0x0e5e, 0x0f89, 0x8adb, 0x1a57, 0x0740, 0x6a76, 0x16b6, 0x3156, 0xd53e, + 0xd1cb, 0x51a3, 0x52ce, 0xce20, 0x5d9c, 0x4a85, 0xadbb, 0x59fb, 0x67e0, + 0x2503, 0x2190, 0xa167, 0xa292, 0x1de5, 0xad60, 0x9a49, 0xfd7f, 0xa9bf, + 0xb7a4, 0xd378, 0x0f9d, 0x8f74, 0x909f, 0x0bf2, 0x9b6d, 0x8856, 0xeb8c, + 0x97cc, 0xa5b1, 0xc185, 0x49ee, 0xb732, 0xb85d, 0x33b0, 0xc32b, 0xb014, + 0x134b, 0xbf8a, 0xcd6f, 0xe943, 0x71ac, 0xd9e4, 0x9266, 0x0db9, 0x9d34, + 0x8a1d, 0xed53, 0x9993, 0xa778, 0xc34c, 0x4bb5, 0xb3ed, 0x4e5a, 0x9cca, + 0x2c46, 0x192f, 0x7c65, 0x28a5, 0x368a, 0x525e, 0xdac6, 0x42ff, 0xdd6b, + 0x714b, 0x885d, 0x7546, 0xd87c, 0x84bc, 0x92a1, 0xae75, 0x36de, 0x9f16, + 0x3983, 0xcd62, 0x207d, 0x78b9, 0xdbef, 0x882f, 0x9614, 0xb1e8, 0x3a51, + 0xa289, 0x3cf6, 0xd0d5, 0x23f0, 0x357c, 0x5c18, 0x0858, 0x163d, 0x3211, + 0xba79, 0x22b2, 0xbd1e, 0x50fe, 0xa418, 0xb5a4, 0xc30d, 0x072d, 0x1512, + 0x30e6, 0xb94e, 0x2187, 0xbbf3, 0x4fd3, 0xa2ed, 0xb479, 0xc1e2, 0x71a6, + 0x99bf, 0xb593, 0x3dfc, 0xa634, 0x40a1, 0xd480, 0x279b, 0x3927, 0x4690, + 0xf653, 0x23e6, 0x2618, 0xae80, 0x16b9, 0xb125, 0x4505, 0x981f, 0xa9ab, + 0xb714, 0x66d8, 0x946a, 0xa5fe, 0xc197, 0x29d0, 0xc43c, 0x581c, 0xab36, + 0xbcc2, 0xca2b, 0x79ef, 0xa781, 0xb915, 0xd950, 0xc699, 0x6106, 0xf4e5, + 0x4800, 0x598c, 0x66f5, 0x16b9, 0x444b, 0x55df, 0x761a, 0xdd70, 0xb4c6, + 0x48a6, 0x9bc0, 0xad4c, 0xbab5, 0x6a79, 0x980b, 0xa99f, 0xc9da, 0x3131, + 0xcb00, 0x3ac1, 0x8ddb, 0x9f67, 0xacd0, 0x5c94, 0x8a26, 0x9bba, 0xbbf5, + 0x234c, 0xbd1b, 0x92d9, 0x7207, 0x8393, 0x90fc, 0x40c0, 0x6e52, 0x7fe6, + 0xa021, 0x0778, 0xa147, 0x7705, 0xcda4, 0xfb2a, 0x0894, 0xb857, 0xe5e9, + 0xf77d, 0x17b9, 0x7f0f, 0x18df, 0xee9c, 0x453c, 0xaf9a, 0xa05b, 0x501f, + 0x7db1, 0x8f45, 0xaf80, 0x16d7, 0xb0a6, 0x8664, 0xdd03, 0x4762, 0x54ce, + 0xb5b2, 0xe344, 0xf4d8, 0x1514, 0x7c6a, 0x163a, 0xebf7, 0x4297, 0xacf5, + 0xba61, 0x4bc4, 0x4f65, 0x60f9, 0x8134, 0xe88a, 0x825a, 0x5818, 0xaeb7, + 0x1916, 0x2682, 0xb7e4, 0x2019, 0x0ddf, 0x2e1a, 0x9570, 0x2f40, 0x04fe, + 0x5b9d, 0xc5fb, 0xd367, 0x64ca, 0xccfe, 0xea68, 0x1c8e, 0x83e4, 0x1db4, + 0xf371, 0x4a11, 0xb46f, 0xc1db, 0x533e, 0xbb72, 0xd8dc, 0x246b, 0x767b, + 0x104b, 0xe608, 0x3ca8, 0xa706, 0xb472, 0x45d5, 0xae09, 0xcb73, 0x1702, + 0x0396, }; static u8 tmp_buf[TEST_BUFLEN]; @@ -579,14 +452,19 @@ static void test_ip_fast_csum(struct kunit *test) { __sum16 csum_result; u16 expected; + int num_tests = (MAX_LEN / WORD_ALIGNMENT - IPv4_MAX_WORDS * WORD_ALIGNMENT); - for (int len = IPv4_MIN_WORDS; len < IPv4_MAX_WORDS; len++) { - for (int index = 0; index < NUM_IP_FAST_CSUM_TESTS; index++) { - csum_result = ip_fast_csum(random_buf + index, len); - expected = - expected_fast_csum[(len - IPv4_MIN_WORDS) * - NUM_IP_FAST_CSUM_TESTS + - index]; + for (int i = 0; i < num_tests; i++) { + memcpy(&tmp_buf[IP_ALIGNMENT], + random_buf + (i * WORD_ALIGNMENT), + IPv4_MAX_WORDS * WORD_ALIGNMENT); + + for (int len = IPv4_MIN_WORDS; len <= IPv4_MAX_WORDS; len++) { + int index = (len - IPv4_MIN_WORDS) + + i * ((IPv4_MAX_WORDS - IPv4_MIN_WORDS) + 1); + + expected = expected_fast_csum[index]; + csum_result = ip_fast_csum(tmp_buf + IP_ALIGNMENT, len); CHECK_EQ(to_sum16(expected), csum_result); } } @@ -595,28 +473,31 @@ static void test_ip_fast_csum(struct kunit *test) static void test_csum_ipv6_magic(struct kunit *test) { #if defined(CONFIG_NET) - const struct in6_addr *saddr; - const struct in6_addr *daddr; + struct csum_ipv6_magic_data { + const struct in6_addr saddr; + const struct in6_addr daddr; + __le32 len; + __wsum csum; + unsigned char proto; + unsigned char pad[3]; + } *data; unsigned int len; - unsigned char proto; - __wsum csum; - - const int daddr_offset = sizeof(struct in6_addr); - const int len_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr); - const int proto_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + - sizeof(int); - const int csum_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + - sizeof(int) + sizeof(char); - - for (int i = 0; i < NUM_IPv6_TESTS; i++) { - saddr = (const struct in6_addr *)(random_buf + i); - daddr = (const struct in6_addr *)(random_buf + i + - daddr_offset); - len = le32_to_cpu(*(__le32 *)(random_buf + i + len_offset)); - proto = *(random_buf + i + proto_offset); - csum = *(__wsum *)(random_buf + i + csum_offset); + __sum16 csum_result; + int ipv6_num_tests = ((MAX_LEN - sizeof(struct csum_ipv6_magic_data)) / WORD_ALIGNMENT); + + for (int i = 0; i < ipv6_num_tests; i++) { + int index = i * WORD_ALIGNMENT; + + data = (struct csum_ipv6_magic_data *)(random_buf + index); + + len = le32_to_cpu(*(__le32 *)(&data->len)); + + csum_result = csum_ipv6_magic(&data->saddr, &data->daddr, + len, + data->proto, + data->csum); CHECK_EQ(to_sum16(expected_csum_ipv6_magic[i]), - csum_ipv6_magic(saddr, daddr, len, proto, csum)); + csum_result); } #endif /* !CONFIG_NET */ }
The test cases for ip_fast_csum and csum_ipv6_magic were not properly aligning the IP header, which were causing failures on architectures that do not support misaligned accesses like some ARM platforms. To solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the standard alignment of an IP header and must be supported by the architecture. Furthermore, all architectures except the m68k pad "struct csum_ipv6_magic_data" to 44 bits. To make compatible with the m68k, manually pad this structure to 44 bits. Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum") Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> --- The ip_fast_csum and csum_ipv6_magic tests did not work on all architectures due to differences in misaligned access support. Fix those issues by changing endianness of data and aligning the data. This patch relies upon a patch from Christophe: [PATCH net] kunit: Fix again checksum tests on big endian CPUs https://lore.kernel.org/lkml/73df3a9e95c2179119398ad1b4c84cdacbd8dfb6.1708684443.git.christophe.leroy@csgroup.eu/t/ --- Changes in v10: - Christophe Leroy graciously decided to re-write my patch to fit his style so I have dropped my endianness+sparse changes and have based by alignment fixes on his patch. The link to his patch can be seen above. - I dropped Guenter's tested-by but kept his reviewed-by since only the base was changed. - Link to v9: https://lore.kernel.org/r/20240221-fix_sparse_errors_checksum_tests-v9-0-bff4d73ab9d1@rivosinc.com Changes in v9: - Revert back to v7, the changes to v8 were not needed - Link to v8: https://lore.kernel.org/r/20240214-fix_sparse_errors_checksum_tests-v8-0-36b60e673593@rivosinc.com Changes in v8: - Change ipv6_magic test case to use memcpy to avoid misalignment - Add Guenter's tested-by to patch 1 but not patch 2 since the later has changed - Link to v7: https://lore.kernel.org/r/20240212-fix_sparse_errors_checksum_tests-v7-0-bbd3b8f64746@rivosinc.com Changes in v7: - Incorporate feedback for test_csum_ipv6_magic from Guenter and Al - Link to v6: https://lore.kernel.org/r/20240207-fix_sparse_errors_checksum_tests-v6-0-4caa9629705b@rivosinc.com Changes in v6: - Fix for big endian (Guenter) - Link to v5: https://lore.kernel.org/r/20240130-fix_sparse_errors_checksum_tests-v5-0-4d8a0a337e5e@rivosinc.com Changes in v5: - Add Guenter's tested-by - CC Andrew Morton - Link to v4: https://lore.kernel.org/r/20240124-fix_sparse_errors_checksum_tests-v4-0-bc2b8d23a35c@rivosinc.com Changes in v4: - Pad test values with zeroes (David) - Link to v3: https://lore.kernel.org/r/20240123-fix_sparse_errors_checksum_tests-v3-0-efecc7f94297@rivosinc.com Changes in v3: - Don't read memory out of bounds - Link to v2: https://lore.kernel.org/r/20240123-fix_sparse_errors_checksum_tests-v2-0-b306b6ce7da5@rivosinc.com Changes in v2: - Add additional patch to fix alignment issues - Link to v1: https://lore.kernel.org/r/20240119-fix_sparse_errors_checksum_tests-v1-1-2d3df86d8d78@rivosinc.com --- lib/checksum_kunit.c | 389 ++++++++++++++++++--------------------------------- 1 file changed, 135 insertions(+), 254 deletions(-) --- base-commit: 27345f2bb9eb800480cbe644026038e96c4d4b67 change-id: 20240119-fix_sparse_errors_checksum_tests-26b86b34d784