Received-SPF: pass (google.com: domain of linux-kernel+bounces-82200-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 26 Feb 2024 19:06:46 +0000
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	David Laight <David.Laight@aculab.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Helge Deller <deller@gmx.de>,
	"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
	Parisc List <linux-parisc@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Palmer Dabbelt <palmer@rivosinc.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v10] lib: checksum: Use aligned accesses for ip_fast_csum
 and csum_ipv6_magic tests
Message-ID: <ZdzhRntTHApp0doV@shell.armlinux.org.uk>
References: <20240223-fix_sparse_errors_checksum_tests-v10-1-b6a45914b7d8@rivosinc.com>
 <7ae930a7-3b10-4470-94ee-89cb650b3349@csgroup.eu>
 <e11fea7a-e99e-4539-a489-0aa145ee65f0@roeck-us.net>
 <ZdzPgSCTntY7JD5i@shell.armlinux.org.uk>
 <ZdzZ5tk459bgUrgz@ghost>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZdzZ5tk459bgUrgz@ghost>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!