From: Guenter Roeck <linux@roeck-us.net>
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: David Laight <David.Laight@aculab.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Andrew Morton <akpm@linux-foundation.org>,
Al Viro <viro@zeniv.linux.org.uk>,
linux-kernel@vger.kernel.org, Helge Deller <deller@gmx.de>,
"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
Parisc List <linux-parisc@vger.kernel.org>
Subject: Re: [PATCH v7 2/2] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests [issues with parisc64]
Date: Mon, 12 Feb 2024 22:57:47 -0800 [thread overview]
Message-ID: <a430a0b7-28b3-45aa-b9b3-6edbfda36acc@roeck-us.net> (raw)
In-Reply-To: <Zcq4jj3vsVtqQIHr@ghost>
On 2/12/24 16:32, Charlie Jenkins wrote:
> On Mon, Feb 12, 2024 at 04:14:49PM -0800, Guenter Roeck wrote:
>> On 2/12/24 12:33, Charlie Jenkins wrote:
>>> The test cases for ip_fast_csum and csum_ipv6_magic were failing on a
>>> variety of architectures that are big endian or do not support
>>> misalgined accesses. Both of these test cases are changed to support big
>>> and little endian architectures.
>>>
>>> The test for ip_fast_csum is changed to align the data along (14 +
>>> NET_IP_ALIGN) bytes which is the alignment of an IP header. The test for
>>> csum_ipv6_magic aligns the data using a struct. An extra padding field
>>> is added to the struct to ensure that the size of the struct is the same
>>> on all architectures (44 bytes).
>>>
>>> Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum")
>>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>>
>> This thing really wants to annoy me. Now I get:
>>
>> # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:494
>> Expected ( u64)csum_result == ( u64)expected, but
>> ( u64)csum_result == 46543 (0xb5cf)
>> ( u64)expected == 46544 (0xb5d0)
>> not ok 5 test_csum_ipv6_magic
>>
>> with the parisc64 tests. All other architectures / platforms work fine
>> after applying the various pending fixes. It looks like a carry gets
>> lost somewhere, but I have not been able to figure out where exactly
>> that happens. This only happens with the 64-bit hppa assembler code.
>>
It turns out that hppa64 triggers an unaligned instruction trap if an
access is unaligned (where unaligned means not aligned to 64 bit).
It appears that this trap either wrongly drops or adds a carry flag
under some circumstances, and that this is causing the problem to be seen.
I can confirm this with:
diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h
index e619e67440db..9bddf3231a28 100644
--- a/arch/parisc/include/asm/checksum.h
+++ b/arch/parisc/include/asm/checksum.h
@@ -128,9 +128,9 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
" ldd,ma 8(%1), %4\n" /* get 1st saddr word */
" ldd,ma 8(%2), %5\n" /* get 1st daddr word */
-" add %4, %0, %0\n"
" ldd,ma 8(%1), %6\n" /* 2nd saddr */
" ldd,ma 8(%2), %7\n" /* 2nd daddr */
+" add %4, %0, %0\n"
" add,dc %5, %0, %0\n"
" add,dc %6, %0, %0\n"
" add,dc %7, %0, %0\n"
or, simpler,
diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h
index e619e67440db..7e2bb797dfe0 100644
--- a/arch/parisc/include/asm/checksum.h
+++ b/arch/parisc/include/asm/checksum.h
@@ -131,7 +131,7 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
" add %4, %0, %0\n"
" ldd,ma 8(%1), %6\n" /* 2nd saddr */
" ldd,ma 8(%2), %7\n" /* 2nd daddr */
-" add,dc %5, %0, %0\n"
+" add %5, %0, %0\n"
" add,dc %6, %0, %0\n"
" add,dc %7, %0, %0\n"
" add,dc %3, %0, %0\n" /* fold in proto+len | carry bit */
both of which make the problem disappear. Unless I am missing something, this
means that the unaligned access trap adds an additional carry flag.
Question now is if this is a bug in the qemu emulation or a bug in the unaligned
trap handler. FWIW, both changes above should be possible workarounds since the
first add operation should never overflow (it adds two 32 bit numbers into a 64
bit register). I'd rather fix the root cause, though.
Any thoughts ? I am adding the parisc maintainers and its e-mail list for
additional feedback.
Thanks,
Guenter
parent reply other threads:[~2024-02-13 6:57 UTC|newest]
Thread overview: expand[flat|nested] mbox.gz Atom feed
[parent not found: <Zcq4jj3vsVtqQIHr@ghost>]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a430a0b7-28b3-45aa-b9b3-6edbfda36acc@roeck-us.net \
--to=linux@roeck-us.net \
--cc=David.Laight@aculab.com \
--cc=James.Bottomley@HansenPartnership.com \
--cc=akpm@linux-foundation.org \
--cc=charlie@rivosinc.com \
--cc=deller@gmx.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=palmer@dabbelt.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).