Page MenuHomeFreeBSD

nvmecontrol: add device self-test op and log page
ClosedPublic

Authored by chuck on Dec 8 2020, 9:27 PM.

Details

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 <muhammad.ahmad@seagate.com>

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chuck requested review of this revision.Dec 8 2020, 9:27 PM
bcr added inline comments.
sbin/nvmecontrol/nvmecontrol.8
476

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

chuck marked an inline comment as done.Dec 8 2020, 9:44 PM

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?

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!

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;

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.

In D27517#615206, @mav 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.

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

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

This revision is now accepted and ready to land.Dec 14 2020, 7:12 PM
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.

Fixed various endianess issues:

  • made log page structure element failing_lba a byte array
  • added htole32() conversion in the selftest command
This revision now requires review to proceed.Dec 14 2020, 11:20 PM

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

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?

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.

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. 🤮

This revision was not accepted when it landed; it landed in state Needs Review.Jan 8 2021, 5:29 PM
This revision was automatically updated to reflect the committed changes.