In D37356#848742, @corvink wrote:

You've just renamed the variable and inverted the logic from is_set to is_setable. That's unneccessary. Please keep the old name.

The only thing which really changed is that the reset of the variable is done in nvme_init_queues instead of nvme_reset.

So, the question is: Do we have to reset the variable on nvme_reset. I don't know. Your quote of the specification isn't clear on that.

address reviewer feedback.

address feedback

drop init value of report_crit

I agree with chuck. fw_num_slots can't be zero. If it's zero, your controller will be broken and you can't rely on any of these values. So, I think it makes no sense to keep the if.

unconditionly print fw_slot1_ro.

I think Chuck is right. output fw_num_slots unconditionally, but for Firmware Slot 1 Read-Only, we have to make sure the fw_num_slots is bigger than zero. othersize it is meaningless. Although the NVMe controller should at least have one firmware slot.

unconditionally print fw_num_slots.

All right, I think for unimplemented features, always return FEATURE NOT CHANGEABLE is compliant to the spec. and for implemented features, it is up to the implementation.so I am reclaiming this patch. thanks.

@corvink Can you please help me verify if this patch solve the issue? it needs an Intel SSD for verifying.

Address reviewers' feedback.

Address reviews' feedback.

• remove a unnecessary include
• fix typo
• fix code style of switch

• combine all changes to one commit.
• build check with CURRENT source.
• correct manpages

I have built a test ISO and it works as expected within the BHYVE virtual machine with following conditions

Update as reviewer suggested.

@imp I am sorry for the previous samsung patch that I didn't compile on the i386 platform to have checked.

@kevans I am sorry I just picked up the latest commiter from bsdinstall history,
maybe you can take a look for this patch. I have been suffering from this issue for many years.

In D32936#826849, @chuck wrote:

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

Rollback to previous diff.

Update code as requested.

Can we give a go for this patch?

catch up with the latest source.

In D33866#838287, @imp wrote:

Just some minor style cleanup, and I'll commit.
I'll use the author name of:
Wanpeng Qian <wanpengqian@gmail.com>

• Fix code style, match style(9) as @imp requested.
• Fix typo and uppercase vendor‘s name as @pauamma requested.

• Commit new files(by miss).

• Update manpage, fix typo and uppercase vendor's name.
• Fix code style, match style(9)

I tested with 14.0-CURRENT Oct 10 2022, and it works. Is it the right direction for dynamically changing the disk size after hosts resize the disk?

Update to match the latest source.
Update manpage.

In D33749#835893, @bcr wrote:

Who's going to commit this? Seems like it's as complete as it can get.

Can we have a universal PCI vendor/subvendor setting for all PCI devices instead of for particular PCI devices? other pci devices may want to change the ID/subID too. For example, if I want to fake the NVMe controller as an Intel one.

remove comments.

remove comments

Here is the compare result with smartctl, for a intel optane 16GB nvme drive

Although I insist on adding the calculation of PowerOnHours.  For now, this patch only adds the init value of PowerCycles.

As reviewer suggested, only init PowerCycles value for now.

@imp Can we give a go for this?

Since D32659 is committed. Can we give a go for this? @imp

I got a intel Xe DG1 dedicated graphics adapter. I will try it later.

Any comments?

In D32831#785808, @chuck wrote:

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.

In D32798#786036, @chuck wrote:

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?

Any comments?

In D32903#786080, @chuck wrote:

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

In D32558#785800, @chuck wrote:

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.

remove incorrect oacs setting.

Can we merge this patch to HEAD?

Any comments on this?
Obviously guest OS will work properly without this patch, but we can make the simulation more real.