bhyve: initial PowerCycles value

[wanpengqian_gmail.com]

Summary

Currently PowerCycles field of Log Page is 0 and it is an invalid value.

this patch will initial the PowerCycles data to 1

Signed-off-by: Wanpeng Qian <[e-mail]>

Test Plan

In Windows/Linux/FreeBSD guests, print the Log Page to comfirm.

[wanpengqian_gmail.com]

Correct the PowerOnHours formula.

[chuck]

Thank you for doing this! Overall, it looks good but needs a few minor changes.

usr.sbin/bhyve/pci_nvme.c

248

I think style(9) suggests this shouldn't be done:

Avoid typedefs ending in “_t”, except as specified in Standard C or by
POSIX.

but perhaps I'm misinterpreting it.

706

Can you help me understand the use of memcpy here? Would a simple assignment not work?

1480

This copy can be performed outside of the locked region.

The IO completion updates the read/write counters in a different thread, and thus, the code here (and there) require a lock to maintain the values' consistencies.

[wanpengqian_gmail.com]

Update according comments.

[wanpengqian_gmail.com]

Update as comments requested.

usr.sbin/bhyve/pci_nvme.c

248

I think style(9) suggests this shouldn't be done:

Avoid typedefs ending in “_t”, except as specified in Standard C or by
POSIX.

Right, It didn't match the style(9).

706

Can you help me understand the use of memcpy here? Would a simple assignment not work?

The NVMe PowerCycles value is a 128-bit counter, but sys/dev/nvme/nvme.h define it as an array of 2 64-bit values. it can not simply be assigned in one line. So I assign it as other 128-bit values do.

Now it changes to two line assignment, (Although the initial low value is enough, I prefer to do it whole.)

[chuck]

Go ahead and add back the memcpy, and I am happy with this change.

usr.sbin/bhyve/pci_nvme.c

706

Bah! I forgot about that. Yes, your first approach was fine.

[imp]

usr.sbin/bhyve/pci_nvme.c

706

Last I checked, there was no int128_t data type on 32-bit platforms. If that's changed, I can update the few uses of 2 64-bit quantities to a single 128-bit int.

[chuck]

usr.sbin/bhyve/pci_nvme.c

706

Hmm, then the emulation is already in trouble as it uses __uint128_t in several other places. If this doesn't exist for 32-bit platforms, what is the portable way to do math on 16-byte values?

[wanpengqian_gmail.com]

Rollback using memcpy to initial power_cycles value

[jhb]

usr.sbin/bhyve/pci_nvme.c

706

bhyve currently only runs on amd64 (and arm64 out of tree). If at some point a 32-bit port (armv7 is the most likely candidate) grows bhyve support, then we can worry about this. However, for now I think it's fine for bhyve to assume 64-bit architectures and __uint128_t

[imp]

usr.sbin/bhyve/pci_nvme.c

706

The problem is with the shared nvme driver which runs on i386...

[chuck]

usr.sbin/bhyve/pci_nvme.c

706

I think we're OK as this file isn't shared with the nvme(4)

[wanpengqian_gmail.com]

create a macro to calculate power_on_hours. it can be shared with others.

[wanpengqian_gmail.com]

use a more suitable name for macro.

[wanpengqian_gmail.com]

Can anyone help this patch be committed to HEAD?

[chuck]

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.

[wanpengqian_gmail.com]

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.

I totally agree with you that PowerOnHours absolute value is meaningless within the virtual machine level. But the difference between each PowerOnHours is usable. For example, After 3 hours, software should inspect that the PowerOnHours value should be increased 3 instead of a fixed value or remain 0. That is abnormal behavior for this very NVMe controller.

Take the D24202 patch as a real example. the virtual controller temperature is meaningless, But without this patch, some software/OS will reject running.

[chuck]

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.

[wanpengqian_gmail.com]

As reviewer suggested, only init PowerCycles value for now.

[wanpengqian_gmail.com]

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

[wanpengqian_gmail.com]

Can we give a go for this patch?

[corvink]

@manu I'm going to commit if you accept it.

usr.sbin/bhyve/pci_nvme.c

691

I'd prefer converting it to little endian as required by the nvme spec.

Nevertheless, big endian architectures aren't supported by bhyve and as far as I can see, FreeBSD has no htole128 macro yet. So, I think it's fine.

[manu]

In D32558#846022, @corvink wrote:

@manu I'm going to commit if you accept it.

Approved by: manu (mentor)