Re: [PATCH v15 5/5] kunit: Add tests for csum_ipv6_magic and ip_fast_csum

From: Charlie Jenkins
Date: Mon Jan 22 2024 - 19:42:11 EST

On Mon, Jan 22, 2024 at 03:39:16PM -0800, Palmer Dabbelt wrote:
> On Mon, 22 Jan 2024 13:41:48 PST (-0800), David.Laight@xxxxxxxxxx wrote:
> > From: Guenter Roeck
> > > Sent: 22 January 2024 17:16
> > >
> > > On 1/22/24 08:52, David Laight wrote:
> > > > From: Guenter Roeck
> > > >> Sent: 22 January 2024 16:40
> > > >>
> > > >> Hi,
> > > >>
> > > >> On Mon, Jan 08, 2024 at 03:57:06PM -0800, Charlie Jenkins wrote:
> > > >>> Supplement existing checksum tests with tests for csum_ipv6_magic and
> > > >>> ip_fast_csum.
> > > >>>
> > > >>> Signed-off-by: Charlie Jenkins <charlie@xxxxxxxxxxxx>
> > > >>> ---
> > > >>
> > > >> With this patch in the tree, the arm:mps2-an385 qemu emulation gets a bad hiccup.
> > > >>
> > > >> [ 1.839556] Unhandled exception: IPSR = 00000006 LR = fffffff1
> > > >> [ 1.839804] CPU: 0 PID: 164 Comm: kunit_try_catch Tainted: G N 6.8.0-rc1 #1
> > > >> [ 1.839948] Hardware name: Generic DT based system
> > > >> [ 1.840062] PC is at __csum_ipv6_magic+0x8/0xb4
> > > >> [ 1.840408] LR is at test_csum_ipv6_magic+0x3d/0xa4
> > > >> [ 1.840493] pc : [<21212f34>] lr : [<21117fd5>] psr: 0100020b
> > > >> [ 1.840586] sp : 2180bebc ip : 46c7f0d2 fp : 21275b38
> > > >> [ 1.840664] r10: 21276b60 r9 : 21275b28 r8 : 21465cfc
> > > >> [ 1.840751] r7 : 00003085 r6 : 21275b4e r5 : 2138702c r4 : 00000001
> > > >> [ 1.840847] r3 : 2c000000 r2 : 1ac7f0d2 r1 : 21275b39 r0 : 21275b29
> > > >> [ 1.840942] xPSR: 0100020b
> > > >>
> > > >> This translates to:
> > > >>
> > > >> PC is at __csum_ipv6_magic (arch/arm/lib/csumipv6.S:15)
> > > >> LR is at test_csum_ipv6_magic (./arch/arm/include/asm/checksum.h:60
> > > >> ./arch/arm/include/asm/checksum.h:163 lib/checksum_kunit.c:617)
> > > >>
> > > >> Obviously I can not say if this is a problem with qemu or a problem with
> > > >> the Linux kernel. Given that, and the presumably low interest in
> > > >> running mps2-an385 with Linux, I'll simply disable that test. Just take
> > > >> it as a heads up that there _may_ be a problem with this on arm
> > > >> nommu systems.
> > > >
> > > > Can you drop in a disassembly of __csum_ipv6_magic ?
> > > > Actually I think it is:
> > >
> > > It is, as per the PC pointer above. I don't know anything about arm assembler,
> > > much less about its behavior with THUMB code.
> >
> > Doesn't look like thumb to me (offset 8 is two 4-byte instructions) and
> > the code I found looks like arm to me.
> > (I haven't written any arm asm since before they invented thumb!)
> >
> > > > ENTRY(__csum_ipv6_magic)
> > > > str lr, [sp, #-4]!
> > > > adds ip, r2, r3
> > > > ldmia r1, {r1 - r3, lr}
> > > >
> > > > So the fault is (probably) a misaligned ldmia ?
> > > > Are they ever supported?
> > > >
> > >
> > > Good question. My primary guess is that this never worked. As I said,
> > > this was just intended to be informational, (probably) no reason to bother.
> > >
> > > Of course one might ask if it makes sense to even keep the arm nommu code
> > > in the kernel, but that is of course a different question. I do wonder though
> > > if anyone but me is running it.
> >
> > If it is an alignment fault it isn't a 'nommu' bug.
> >
> > And traditionally arm didn't support misaligned transfers (well not
> > in anyway any other cpu did!).
> > It might be that the kernel assumes that all ethernet packets are
> > aligned, but the test suite isn't aligning the buffer.
> > Which would make it a test suite bug.
>
> From talking to Evan and Vineet, I think you're right and this is a test
> suite bug: specifically the tests weren't respecting NET_IP_ALIGN. That
> didn't crop up for ip_fast_csum() as it just uses ldr which supports
> misaligned accesses on the M3 (at least as far as I can tell).
>
> So I think the right fix is something like
>
> diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c
> index 225bb7701460..2dd282e27dd4 100644
> --- a/lib/checksum_kunit.c
> +++ b/lib/checksum_kunit.c
> @@ -5,6 +5,7 @@
> #include <kunit/test.h>
> #include <asm/checksum.h>
> +#include <asm/checksum.h>
> #include <net/ip6_checksum.h>
> #define MAX_LEN 512
> @@ -15,6 +16,7 @@
> #define IPv4_MAX_WORDS 15
> #define NUM_IPv6_TESTS 200
> #define NUM_IP_FAST_CSUM_TESTS 181
> +#define SUPPORTED_ALIGNMENT (1 << NET_IP_ALIGN)
> /* Values for a little endian CPU. Byte swap each half on big endian CPU. */
> static const u32 random_init_sum = 0x2847aab;
> @@ -486,7 +488,7 @@ static void test_csum_fixed_random_inputs(struct kunit *test)
> __sum16 result, expec;
> assert_setup_correct(test);
> - for (align = 0; align < TEST_BUFLEN; ++align) {
> + for (align = 0; align < TEST_BUFLEN; align += SUPPORTED_ALIGNMENT) {
> memcpy(&tmp_buf[align], random_buf,
> min(MAX_LEN, TEST_BUFLEN - align));
> for (len = 0; len < MAX_LEN && (align + len) < TEST_BUFLEN;
> @@ -513,7 +515,7 @@ static void test_csum_all_carry_inputs(struct kunit *test)
> assert_setup_correct(test);
> memset(tmp_buf, 0xff, TEST_BUFLEN);
> - for (align = 0; align < TEST_BUFLEN; ++align) {
> + for (align = 0; align < TEST_BUFLEN; align += SUPPORTED_ALIGNMENT) {
> for (len = 0; len < MAX_LEN && (align + len) < TEST_BUFLEN;
> ++len) {
> /*
> @@ -553,7 +555,7 @@ static void test_csum_no_carry_inputs(struct kunit *test)
> assert_setup_correct(test);
> memset(tmp_buf, 0x4, TEST_BUFLEN);
> - for (align = 0; align < TEST_BUFLEN; ++align) {
> + for (align = 0; align < TEST_BUFLEN; align += SUPPORTED_ALIGNMENT) {
> for (len = 0; len < MAX_LEN && (align + len) < TEST_BUFLEN;
> ++len) {
> /*
>
> but I haven't even build tested it...

This doesn't fix the test_csum_ipv6_magic test case that was causing the
initial problem, but the same trick can be done in that test.

- Charlie

>
> >
> > David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)