Page MenuHomeFreeBSD

bhyve: initial PowerCycles value
ClosedPublic

Authored by wanpengqian_gmail.com on Oct 19 2021, 11:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 3, 8:48 AM
Unknown Object (File)
Sun, Mar 3, 4:56 AM
Unknown Object (File)
Jan 29 2024, 5:08 AM
Unknown Object (File)
Jan 29 2024, 5:08 AM
Unknown Object (File)
Jan 29 2024, 5:08 AM
Unknown Object (File)
Jan 29 2024, 5:08 AM
Unknown Object (File)
Jan 29 2024, 5:08 AM
Unknown Object (File)
Dec 20 2023, 8:31 AM

Details

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 <wanpengqian@gmail.com>

Test Plan

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 42707
Build 39595: arc lint + arc unit

Event Timeline

wanpengqian_gmail.com retitled this revision from bhyve: initial PowerCycles value and implement PowerOnHours statistics to bhyve: initial PowerCycles value and implement PowerOnHours caculation.Oct 20 2021, 1:24 AM
wanpengqian_gmail.com edited the summary of this revision. (Show Details)
chuck requested changes to this revision.Oct 20 2021, 2:52 PM

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.

620

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

1121

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.

This revision now requires changes to proceed.Oct 20 2021, 2:52 PM

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

620

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

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

usr.sbin/bhyve/pci_nvme.c
620

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

usr.sbin/bhyve/pci_nvme.c
620

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.

usr.sbin/bhyve/pci_nvme.c
620

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?

Rollback using memcpy to initial power_cycles value

usr.sbin/bhyve/pci_nvme.c
620

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

usr.sbin/bhyve/pci_nvme.c
620

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

usr.sbin/bhyve/pci_nvme.c
620

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

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

This revision is now accepted and ready to land.Nov 12 2021, 9:42 PM

Can anyone help this patch be committed to HEAD?

chuck requested changes to this revision.Mar 27 2022, 1:51 PM

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.

This revision now requires changes to proceed.Mar 27 2022, 1:51 PM

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.

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.

As reviewer suggested, only init PowerCycles value for now.

wanpengqian_gmail.com retitled this revision from bhyve: initial PowerCycles value and implement PowerOnHours caculation to bhyve: initial PowerCycles value.Aug 25 2022, 4:08 AM
wanpengqian_gmail.com edited the summary of this revision. (Show Details)
wanpengqian_gmail.com edited the test plan for this revision. (Show Details)

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

corvink added a reviewer: manu.
corvink added subscribers: manu, corvink.

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

usr.sbin/bhyve/pci_nvme.c
606

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 I'm going to commit if you accept it.

Approved by: manu (mentor)

This revision was not accepted when it landed; it landed in state Needs Review.Nov 4 2022, 9:14 AM
This revision was automatically updated to reflect the committed changes.