Message-ID: <0e523795-a4b2-4276-b665-969c745b20f6@intel.com>
Date: Fri, 16 Aug 2024 16:37:22 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: lib/packing.c behaving weird if buffer length is not multiple of
 4 with QUIRK_LSW32_IS_FIRST



On 8/15/2024 2:16 PM, Jacob Keller wrote:
> Hi Vladimir!
> 
> I am recently attempting to refactor some bespoke code for packing data
> in the ice networking driver into a bit packed hardware array, using the
> <linux/packing.h> and lib/packing.c infrastructure.
> 
> My hardware packed buffer needs QUIRK_LITTLE_ENDIAN and
> QUIRK_LSW32_IS_FIRST, as the entire buffer is packed as little endian,
> and the lower words come first in the buffer.
> 
> Everything was working ok in initial testing, until I tried packing a
> buffer that was 22 bytes long. Everything seemed to shift by 2 bytes:
> 
> Here, I print the initial details such as offset, lsb, width, then the
> hex dump of the u64 value I'm trying to insert.
> 
> I do the call to packing() with the appropriate quirks set, and then hex
> dump the 22-byte buffer after.
> 
>> kernel: offset=0, size=8, lsb=0, width=57
>> kernel: value: 60 9b fe 01 00 00 00 00
>> kernel: 0x0000000001fe9b60 -> 000-056: fe 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> kernel: 0x0000000001fe9b60 -> 000-056: 00 00 00 00 00 00
> 
> It seems to have failed to copy the first two bytes in to the buffer.
> 
> I discovered that if I pretend the buffer is 24 bytes (a multiple of 4
> bytes, i.e. a word size), then everything works as expected:
> 
>> kernel: offset=0, size=8, lsb=0, width=57
>> kernel: value: 60 fc fe 01 00 00 00 00
>> kernel: 0x0000000001fefc60 -> 000-056: 60 fc fe 01 00 00 00 00 00 00 00 00 00 00 00 00
>> kernel: 0x0000000001fefc60 -> 000-056: 00 00 00 00 00 00 00 00
> 
> It seems like this is either a misunderstanding of a fundamental
> requirement of the packing() infrastructure, a bug in the
> QUIRK_LSW32_IS_FIRST, or I need a new quirk for the packing infrastructure?
> 
> Essentially, I think the problem is that it uses the length of the
> buffer to find the word, but when the length is not a multiple of 2, the
> word offset picked is incorrect.
> 
> Using a larger length than the size of the buffer "works" as long as I
> never use a bit offset thats larger than the *actual* buffer.. but that
> seems like a poor solution.
> 
> Essentially, it seems like the default indexing for big endian is
> searching for each byte from the end of the array and then using the
> quirks to swap back to the inverse ending.
> 
> I think the fix is probably just to do a round-up division on the len/4
> check in get_reverse_lsw32_offset, since its calculating the total
> number of words in the length, and effectively rounds down for lengths
> that aren't multiples of 4.
> 

This doesn't fix the problem. I printed out the logical box number and
its mapped box address in the various modes with a length of 22 bytes:

> kernel: default: 00 => 21
> kernel: default: 01 => 20
> kernel: default: 02 => 19
> kernel: default: 03 => 18
> kernel: default: 04 => 17
> kernel: default: 05 => 16
> kernel: default: 06 => 15
> kernel: default: 07 => 14
> kernel: default: 08 => 13
> kernel: default: 09 => 12
> kernel: default: 10 => 11
> kernel: default: 11 => 10
> kernel: default: 12 => 09
> kernel: default: 13 => 08
> kernel: default: 14 => 07
> kernel: default: 15 => 06
> kernel: default: 16 => 05
> kernel: default: 17 => 04
> kernel: default: 18 => 03
> kernel: default: 19 => 02
> kernel: default: 20 => 01
> kernel: default: 21 => 00

Here, you can see that it correctly maps in "big endian" order the
entire array. Basically it just reverses the ordering. Ok, this seems
reasonable.

With LITTLE_ENDIAN set, things get weird:
> kernel: LITTLE_ENDIAN: 00 => 22
> kernel: LITTLE_ENDIAN: 01 => 23
> kernel: LITTLE_ENDIAN: 02 => 16
> kernel: LITTLE_ENDIAN: 03 => 17
> kernel: LITTLE_ENDIAN: 04 => 18
> kernel: LITTLE_ENDIAN: 05 => 19
> kernel: LITTLE_ENDIAN: 06 => 12
> kernel: LITTLE_ENDIAN: 07 => 13
> kernel: LITTLE_ENDIAN: 08 => 14
> kernel: LITTLE_ENDIAN: 09 => 15
> kernel: LITTLE_ENDIAN: 10 => 08
> kernel: LITTLE_ENDIAN: 11 => 09
> kernel: LITTLE_ENDIAN: 12 => 10
> kernel: LITTLE_ENDIAN: 13 => 11
> kernel: LITTLE_ENDIAN: 14 => 04
> kernel: LITTLE_ENDIAN: 15 => 05
> kernel: LITTLE_ENDIAN: 16 => 06
> kernel: LITTLE_ENDIAN: 17 => 07
> kernel: LITTLE_ENDIAN: 18 => 00
> kernel: LITTLE_ENDIAN: 19 => 01
> kernel: LITTLE_ENDIAN: 20 => 02
> kernel: LITTLE_ENDIAN: 21 => 03

We map *outside* the buffer! 0 goes to 22, 1 goes to 23! Nothing at all
maps into 20 or 21. This is extremely not-intuitive. In practice,
without having read the docs I would have expected LITTLE_ENDIAN to
directly map 0 to 0. After having read docs, (where it mentions that
LITTLE_ENDIAN only does by word), I honestly have no idea what I would
expect. If we try to just do 4 byte blocks at a time, we end up having
to swap bytes from outside the length.

With LSW32_IS_FIRST set, things are even weirder:

> kernel: LSW32_IS_FIRST: 00 => -3
> kernel: LSW32_IS_FIRST: 01 => -4
> kernel: LSW32_IS_FIRST: 02 => 03
> kernel: LSW32_IS_FIRST: 03 => 02
> kernel: LSW32_IS_FIRST: 04 => 01
> kernel: LSW32_IS_FIRST: 05 => 00
> kernel: LSW32_IS_FIRST: 06 => 07
> kernel: LSW32_IS_FIRST: 07 => 06
> kernel: LSW32_IS_FIRST: 08 => 05
> kernel: LSW32_IS_FIRST: 09 => 04
> kernel: LSW32_IS_FIRST: 10 => 11
> kernel: LSW32_IS_FIRST: 11 => 10
> kernel: LSW32_IS_FIRST: 12 => 09
> kernel: LSW32_IS_FIRST: 13 => 08
> kernel: LSW32_IS_FIRST: 14 => 15
> kernel: LSW32_IS_FIRST: 15 => 14
> kernel: LSW32_IS_FIRST: 16 => 13
> kernel: LSW32_IS_FIRST: 17 => 12
> kernel: LSW32_IS_FIRST: 18 => 19
> kernel: LSW32_IS_FIRST: 19 => 18
> kernel: LSW32_IS_FIRST: 20 => 17
> kernel: LSW32_IS_FIRST: 21 => 16

We access bytes with negative indexes...

And with both set:

> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 00 => -2
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 01 => -1
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 02 => 00
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 03 => 01
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 04 => 02
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 05 => 03
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 06 => 04
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 07 => 05
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 08 => 06
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 09 => 07
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 10 => 08
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 11 => 09
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 12 => 10
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 13 => 11
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 14 => 12
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 15 => 13
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 16 => 14
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 17 => 15
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 18 => 16
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 19 => 17
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 20 => 18
> kernel: LITTLE_ENDIAN | LSW32_IS_FIRST: 21 => 19

We still get negative indexes, but things are close. Everything is off
by 2 bytes from what I would expect.

I'm honestly not sure what the right solution here is, because the way
LITTLE_ENDIAN and LSW32_IS_FIRST work they effectively *require*
word-aligned sizes. If we use a word-aligned size, then they both make
sense, but my hardware buffer isn't word aligned. I can cheat, and just
make sure I never use bits that access the invalid parts of the buffer..
but that seems like the wrong solution... A larger size would break
normal Big endian ordering without quirks...

Really, what my hardware buffer wants is to map the lowest byte of the
data to the lowest byte of the buffer. This is what i would consider
traditionally little endian ordering.

This also happens to be is equivalent to LSW32_IS_FIRST and
LITTLE_ENDIAN when sizes are multiples of 4.

Powered by blists - more mailing lists