nvmecontrol: add device self-test op and log page

[chuck]

Summary

Add decoding of the Device Self-test log page and the ability to start
or abort a test

Test Plan

Tested by: Muhammad Ahmad <[e-mail]>

[bcr]

sbin/nvmecontrol/nvmecontrol.8

477

You need to make a line break after the sentence stop.

[mav]

I don't see here nvme_self_test_page_swapbytes() implemented and called in read_logpage() and may be kernel nvme_ctrlr_async_event_log_page_cb().

Also use of misaligned 64-bit fields in struct nvme_device_self_test_page may work fine for x86, but generally not great. Not so long ago we've had sparc64, that happily crashed on such accesses in kernel. Do we no longer care?

[chuck]

In D27517#615047, @mav wrote:

I don't see here nvme_self_test_page_swapbytes() implemented and called in read_logpage() and may be kernel nvme_ctrlr_async_event_log_page_cb().

Oops, I missed that. Thanks!

[chuck]

In D27517#615047, @mav wrote:

Also use of misaligned 64-bit fields in struct nvme_device_self_test_page may work fine for x86, but generally not great. Not so long ago we've had sparc64, that happily crashed on such accesses in kernel. Do we no longer care?

The structure matches the NVMe specification. That said, would you prefer to see this as

		uint32_t	failing_lba_lo;
		uint32_t	failing_lba_hi;

[mav]

In D27517#615200, @chuck wrote:

The structure matches the NVMe specification. That said, would you prefer to see this as

		uint32_t	failing_lba_lo;
		uint32_t	failing_lba_hi;

I have feeling we already have some field(s) like that (or as array?) somewhere, just don't remember where from the to of my head.

[chuck]

In D27517#615206, @mav wrote:

In D27517#615200, @chuck wrote:

The structure matches the NVMe specification. That said, would you prefer to see this as

		uint32_t	failing_lba_lo;
		uint32_t	failing_lba_hi;

I have feeling we already have some field(s) like that (or as array?) somewhere, just don't remember where from the to of my head.

The struct nvme_registers splits out the 64-bit capability register as two 32-bit entities, cap_lo and cap_hi. But this is the only instance I've found.

[mav]

We also have several 128-bit capacity values represented as arrays of bytes.

[chuck]

Addressed new-line in the man page and added a swap bytes routine for the device self-test log page

[imp]

In D27517#615211, @mav wrote:

We also have several 128-bit capacity values represented as arrays of bytes.

Those are that way mostly because there's no 'long long long' or 'super long' data type :). This means they could be changed if necessary...

I think _lo / _hi is fine. It simplifies a number of things to have things constrained to 4-byte units.

[chuck]

Fixed various endianess issues:

• made log page structure element failing_lba a byte array
• added htole32() conversion in the selftest command

[bcr]

OK from manpages.

[chuck]

Any feedback on the change of failing_lba to uint8_t [8]? Does anything else stick out?

[rpokala]

In D27517#623771, @chuck wrote:

Any feedback on the change of failing_lba to uint8_t [8]? Does anything else stick out?

FWIW, I prefer the _lo / _hi dwords, rather than the byte array. NVMe only uses byte arrays for reserved or inconvenient-length items.

That having been said - this is because the failing_lba field isn't aligned, right? But according to NVMe-1.4, it is aligned: failing_lba is bytes [23:16] of the data structure, which is clearly both 4-byte and 8-byte aligned. So, what's the problem with just having it be a uint64_t?

[chuck]

In D27517#623929, @rpokala wrote:

In D27517#623771, @chuck wrote:

Any feedback on the change of failing_lba to uint8_t [8]? Does anything else stick out?

FWIW, I prefer the _lo / _hi dwords, rather than the byte array. NVMe only uses byte arrays for reserved or inconvenient-length items.

With lo/hi, the endianess was confusing me (does endianess change which variable is << 32? i.e. (hi << 32) | lo vs (lo << 32) | hi). This approach hurt my brain less.

That having been said - this is because the failing_lba field isn't aligned, right? But according to NVMe-1.4, it is aligned: failing_lba is bytes [23:16] of the data structure, which is clearly both 4-byte and 8-byte aligned. So, what's the problem with just having it be a uint64_t?

In the first element of the results[] array, yes, failing_lba is nicely aligned. But the packed array element size (28 bytes) is not divisible by 8 which will force every other failing_lba field to only be 4 byte aligned.

[rpokala]

In D27517#624368, @chuck wrote:

In D27517#623929, @rpokala wrote:

That having been said - this is because the failing_lba field isn't aligned, right? But according to NVMe-1.4, it is aligned: failing_lba is bytes [23:16] of the data structure, which is clearly both 4-byte and 8-byte aligned. So, what's the problem with just having it be a uint64_t?

In the first element of the results[] array, yes, failing_lba is nicely aligned. But the packed array element size (28 bytes) is not divisible by 8 which will force every other failing_lba field to only be 4 byte aligned.

Oh, damn, I didn't even notice that Get Log Page – Self-test Result Data Structure is only 28 bytes, or that the first instance of the array of those structures starts at offset [4] in Get Log Page – Device Self-test Log. 🤮