Page MenuHomeFreeBSD

nvmecontrol: Fix to128 for big endian targets
ClosedPublic

Authored by imp on Apr 6 2024, 6:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 9:14 PM
Unknown Object (File)
Fri, Oct 18, 6:50 AM
Unknown Object (File)
Sep 26 2024, 1:25 AM
Unknown Object (File)
Sep 26 2024, 1:24 AM
Unknown Object (File)
Sep 26 2024, 1:24 AM
Unknown Object (File)
Sep 26 2024, 1:24 AM
Unknown Object (File)
Sep 25 2024, 5:18 PM
Unknown Object (File)
Sep 25 2024, 4:20 AM
Subscribers

Details

Summary

The source is always 128-bits in little endian format. For big endian
hosts, we have to convert, or we print bogus numbers.

Sponsored by: Netflix

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Apr 8 2024, 2:08 PM

This probably doesn't quite work for 32-bit big endian? At least, I used this:

static __inline uint128_t
to128(const void *p)
{
#if __SIZEOF_LONG__ == 4
	return (le64dec(p));
#else
	uint64_t lo, hi;

	lo = le64dec(p);
	hi = le64dec((const char *)p + 8);
	return ((uint128_t)hi << 64 | lo);
#endif
}

(I think it's a bit cleaner to use le64dec instead of assuming it is aligned). Also, if it weren't invasive to do the rename I would rather call this le128dec instead of to128

In D44651#1018919, @jhb wrote:

This probably doesn't quite work for 32-bit big endian? At least, I used this:

static __inline uint128_t
to128(const void *p)
{
#if __SIZEOF_LONG__ == 4
	return (le64dec(p));
#else
	uint64_t lo, hi;

	lo = le64dec(p);
	hi = le64dec((const char *)p + 8);
	return ((uint128_t)hi << 64 | lo);
#endif
}

(I think it's a bit cleaner to use le64dec instead of assuming it is aligned). Also, if it weren't invasive to do the rename I would rather call this le128dec instead of to128

edited There's reasons I was so lazy... but they don't matter.

Here's what I landed on...

#if __STDC_VERSION__ >= 202311L
typedef unsigned __BitInt(128) uint128_t;
#elif defined(__SIZEOF_INT128__)
typedef __uint128_t uint128_t;
#else
typedef uint64_t uint128_t;
#endif

static __inline uint128_t
to128(void *p)
{
#if __STDC_VERSION__ >= 202311L || defined(__SIZEOF_INT128__)
        uint64_t lo, hi;

        lo = le64dec(p);
        hi = le64dec((const char *)p + 8);
        return ((uint128_t)hi << 64 | lo);
#else
     	return (le64dec(p));
#endif
}

but I think you need gcc14 or clang 18 though... We'd have to add CSTD=c23 to the Makefile too...

Use C23 features, if available for 128-bit support.

This revision now requires review to proceed.Apr 9 2024, 6:03 AM

ah, turns out

sbin/nvmecontrol/nvmecontrol.h
101–103

I need to check if gcc really supports this or not... may need to refine

I do like the idea of using C23 for the future as a way to fix 32-bit platforms.

sbin/nvmecontrol/Makefile
0

Maybe we can add c23 to COMPILER_FEATURES for relevant compilers in bsd.compiler.mk and set CSTD conditionally if the compiler supports it? 'c2x' is marked deprecated for GCC currently, so it's probably not best to use it long term.

sbin/nvmecontrol/Makefile
0

Hmmm... That's an interesting idea. I'm going to drop this line from this commit and push.

sbin/nvmecontrol/nvmecontrol.h
101–103

I'll change this to the c23 version 202308L before pushing.

imp marked an inline comment as done.Apr 16 2024, 10:36 PM
imp added inline comments.
sbin/nvmecontrol/nvmecontrol.h
101–103

202311L I mean

This revision was not accepted when it landed; it landed in state Needs Review.Apr 17 2024, 3:34 AM
This revision was automatically updated to reflect the committed changes.