Received-SPF: pass (google.com: domain of linux-kernel+bounces-34300-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 22 Jan 2024 15:39:16 -0800 (PST)
Subject:     RE: [PATCH v15 5/5] kunit: Add tests for csum_ipv6_magic and ip_fast_csum
In-Reply-To: <be959a4bb660466faba5ade7976485c8@AcuMS.aculab.com>
CC: linux@roeck-us.net, charlie@rivosinc.com, Conor Dooley <conor@kernel.org>,
  samuel.holland@sifive.com, xiao.w.wang@intel.com, Evan Green <evan@rivosinc.com>, guoren@kernel.org,
  linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
  Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu, Arnd Bergmann <arnd@arndb.de>
From: Palmer Dabbelt <palmer@dabbelt.com>
To: David.Laight@ACULAB.COM
Message-ID: <mhng-b5f26a34-7632-4423-9f07-3224170bae9f@palmer-ri-x1c9>
Precedence: bulk
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

On Mon, 22 Jan 2024 13:41:48 PST (-0800), David.Laight@ACULAB.COM 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@rivosinc.com>
>> >>> ---
>> >>
>> >> 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...

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