Other than minor nit, LGTM

LGTM but I'm also curious if @corvink suggestion of %u helps

These changes are good but not really necessary from a functionality point of view. In both cases, the emulation is returning an error, making the LID undefined from the guest's perspective. But I'm all for good hygiene.

LGTM

I see what you are saying. Unfortunately, I can't figure out how to remove the "revisions requested" status.

The existing code isn't quite right, but I'm not quite sure what to do here. The number of firmware slots should not ever be zero (i.e., "This field shall specify a value from one to seven"), and my guess is, for most drives, this condition will always be true. Perhaps removing the check and unconditionally printing the slot count and RO status makes more sense as the field description (e.g., "Number of Firmware Slots") are always present.

The code changes do not match the title or summary. Did you update the wrong review?

Switch bzero to memset

I will commit the change to npss but drop the power_state[] changes

The 16.0c test pass with these changes.

The 16.0c test pass with these changes.

In D36119#820473, @markj wrote:

Part of the longer-term solution will be to enable stricter compiler warnings in the bhyve build, but for that, existing warnings have to be addressed first. PR 265749 mentions an NVMe compliance test suite, but we don't currently run it in our regression test suite; would it be possible to do so? Finally, we really need some stubs that would let us compile and fuzz device models in userspace.

All of these changes are fine, but the fact that they are necessary has me worried. Does what I've coded wander into undefined behavior territory?

! In D35328#811978, @gusev.vitaliy_gmail.com wrote:

I would remove "range_is_contiguous" usage and unite all in one condition as "contiguous" case is only if both conditions are true:

(req->prev_size != 0 && (req->prev_gpaddr + req->prev_size) == gpaddr)

In D35328#811976, @andy_omniosce.org wrote:

This change looks good to me. Is there anything stopping this being merged?
(I'm doing another sync to illumos at the end of the month and I'd like to include this if possible)

Update PR list

In D35328#801068, @jhb wrote:

I had read 'initial address of zero' as meaning that the first iovec had iov_base of 0. Saying something like "the first vector" probably is a bit less ambiguous.

Implement suggested rename of variable

In D35328#800851, @gusev.vitaliy_gmail.com wrote:

1. I think patch is good, however I have questions (not directly related to the patch) for lba naming. LBA - logical block address - it should be enumerated with blocks, i.e. 1,.... However pci_nvme_append_iov_req() is called from nvme_write_read_blockif() and offset is passed as lba.

Do you think it is worth renaming lba to offset?

Agreed, that is a change worth making

2. And one more suggestion, what if code will be:

	if (req->io_req.br_iovcnt > 0 && (req->prev_gpaddr + req->prev_size) == gpaddr) {

What you posted is very close to the original change I made. My hope was the intent of this version was clearer and compiled down to roughly the same assembly code.

3. Question related to description.

 The test for contiguous regions has a bug such that it
mistakenly treats an initial address of zero as a contiguous range and
concatenates it with the previous. But because there is no previous IOV,
the concatenation code corrupts the IO request structure and leads to a
segmentation fault when the blockif request completes.

As I understood the first call of pci_nvme_append_iov_req() is always with req->io_req.br_iovcnt == 0, so this concatenation should be involved only for the subsequent calls of pci_nvme_append_iov_req() and it would good to point to it.

I had hoped the last sentence covered this, but it sounds like it missed. Is

I apologize if my word choice was poor. By "synthetic", I meant the emulation would appear to perform a self-test, but in fact would not perform any tests at all. How would this benefit a guest?

In D32903#786264, @wanpengqian_gmail.com wrote:

From NVMe spec(1.4c), here is the quota:
https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4c-2021.06.28-Ratified.pdf

Timestamp values should not be used for security applications. Other application use of the Timestamp is outside the scope of this specification.

The specification didn't say it relates to Persistent Logs and/or Events. so I just implement it as spec documents.

D24202 is not a good example as the compliance tests check some of those values. To the best of my knowledge, the tests do not check power on hours.

After getting some clarification on the wording in the specification (which generated an ECN ...), my preference would be to set NPSS to zero but not report any power state information. I.e. all fields of power_state[0] would be zero. Since pci_nvme_init() uses calloc to allocate the softc (which includes the struct nvme_controller_data), I believe all initialization of cd->power_state can be removed from pci_nvme_init_ctrldata().

This does not appear to implement either Set or Get correctly. For example, it does not use the Timestamp Origin field on Get's.

Give me a few days to check on some wording in the specification.

What is the long term plan for this functionality? Applications can use LBA Range Type to provide usage hints to the Controller such that it can optimize its performance characteristics. Are there kernels / applications out there that use this functionality? If so, is the goal to use these hints to modify the emulation's behavior?

While I agree that using emulated hardware to test drivers and system tools is useful, I'm concerned about adding synthetic functionality to the core code.

After some further thought, I've changed my mind and don't believe it makes sense to include the "power on hours" calculation. The goal of this value in actual hardware is to provide a (weak) heuristic to predict when to replace the drive. But this doesn't make sense for an emulated drive. Correcting the initialization of power on count seems like a good change, but the addition of a synthetic hours doesn't.