Re: [PATCH v10] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests

[Charlie Jenkins <charlie@rivosinc.com>]

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!