> > On 25-Jul-24 5:47 AM, Akhil Goyal wrote: > >> On 24-Jul-24 2:04 PM, Akhil Goyal wrote: > >>>> On 24-Jul-24 12:20 PM, Akhil Goyal wrote: > >>>>>> On 23-Jul-24 5:57 PM, Akhil Goyal wrote: > >>>>>>>> Hi all, > >>>>>>>> > >>>>>>>> This patch breaks ipsec tests with ipsec-secgw: > >>>>>>>> > >>>>>>>> > >>>>>>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1 > >>>>>>>> ... > >>>>>>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for > >>>>>> dst=192.168.31.14, > >>>>>>>> sz=1 > >>>>>>>> test IPv4 trs_aesctr_sha1 finished with status 1 > >>>>>>>> ERROR test trs_aesctr_sha1 FAILED > >>>>>>>> > >>>>>>> The patch seems to be correct. > >>>>>>> Please check endianness in the PMD you are testing. > >>>>>> In my opinion salt should not be affected by endianness and it should > >>>>>> be > >>>>>> used as it is in the key parameter. I think the patch is wrong to make > >>>>>> it CPU endianness dependent before being passed to the PMDs, any PMD > >>>>>> that needs the endianness swapped should do it in the PMD code. Indeed > >>>>>> it's passed around as a 32 bit integer but it's not used as such, and > >>>>>> when it's actually used it should be evaluated as a byte array. > >>>>>> > >>>>> As per the rfc, it should be treated as byte order(i.e. big endian). > >>>>> But here the problem is we treat it as uint32_t which makes it CPU > >>>>> endian > >>>> when stored in ipsec_sa struct. > >>>>> The keys are stored as an array of uint8_t, so keys are stored in byte > >> order(Big > >>>> endian). > >>>>> So either we save salt as "uint8_t salt[4]" or do a conversion of > >>>>> cpu_to_be > >>>>> So that when it is stored in PMD/HW, and we convert it from uint32_t to > >> uint_8 > >>>> *, there wont be issue. > >>>> > >>>> RFC treats it as a "four octet value" - there is no endianness until > >>>> it's treated like an integer, which it never is. Even if it code it's > >>>> being stored and passed as an unsigned 32bit integer it is never > >>>> evaluated as such so its endianness doesn't matter. > >>> The endianness matters the moment it is stored as uint32_t in ipsec_sa > >>> It means the value is stored in CPU endianness in that integer unless it > >>> is > >> specified. > >> > >> What matters is that the four byte value in the key ends up in the > >> memory in the same order, and that was always the case before the patch, > >> regardless of the endianness of the CPU because load and store > >> operations are not affected by endianness. With the patch the same four > >> bytes from the configuration file will be stored differently in memory > >> depending on the CPU. There is no need to fix the endianness of the > >> salt, just as there is no need to fix the byte order of the key itself. > >> > > When a uint32 is used, there is no clarity whether it is in BE or LE. > > So that is where the confusion comes. > > For any new person, uint32 means it is in CPU byte order. > > This patch tried to address that but it failed to address all the cases. > > So my simple suggestion is instead of fixing the byte order at every place, > > Lets just change the ipsec_sa->salt as rte_be32_t from uint32_t and revert > > this > patch. > > This will make things clear. > > I am suggesting to convert this uint32_t to rte_be32_t for library as well > > for salt. > > This change is not swapping anything, but making things explicitly clear > > that salt > is in BE. > > I agree with the suggestion of using rte_be32_t and reverting the patch.
Can you send a patch with both the changes? > > (I still think it would be even better to use uint8_t[4] to reflect that > it is a four byte value with no endianness but that's just IMHO, the > important thing is to revert the patch that broke the functionality) > That can be for next release in library.
