bhyve nvme: Add Temperature Threshold support
ClosedPublic
Actions

Authored by chuck on Dec 19 2021, 4:17 PM.

Details

Reviewers

grehan
jhb
imp
rpokala

Group Reviewers

bhyve

Commits

rGea9ee35583a8: bhyve nvme: Add Temperature Threshold support

Summary

This adds the ability for a guest OS to send Set / Get Feature,
Temperature Threshold commands. The implementation assumes a constant
temperature and will generate an Asynchronous Event Notification if the
specified threshold is above/below this value. Although the
specification allows 9 temperature values, this implementation only
implements the Composite Temperature.

While in the neighborhood, move the clear of the CSTS register in the
reset function after all other cleanup. This avoids a race with the
guest thinking the reset is complete (i.e. CSTS.RDY = 0) before the NVMe
emulation is actually complete with the reset.

Fixes UNH IOL 16.0 Test 1.7, cases 1, 2, and 4.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 43485
Build 40373: arc lint + arc unit

Event Timeline

chuck created this revision.Dec 19 2021, 4:17 PM

Herald added a reviewer: bhyve. · View Herald TranscriptDec 19 2021, 4:17 PM

Herald added subscribers: dab, bcran, rgrimes. · View Herald Transcript

chuck requested review of this revision.Dec 19 2021, 4:17 PM

Harbormaster completed remote builds in B43485: Diff 100301.Dec 19 2021, 4:17 PM

chuck added a parent revision: D33571: bhyve nvme: Fix Set Features.Dec 19 2021, 4:17 PM

chuck added a child revision: D33573: bhyve nvme: Update v1.4 Identify Controller data.

Do we need to keep the first temperature warning timestamp? Later we can calculate the following warning time in the healthy log.
Warning Temp Composite Time:
Error Temp Composite Time:

usr.sbin/bhyve/pci_nvme.c
1694	uint8_t for tmpsel is enough.
1706	how about `((thsel != NVME_TEMP_THRESH_OVER) && (thsel != NVME_TEMP_THRESH_UNDER))` for easy understanding?

allanjude added a subscriber: allanjude.Dec 23 2021, 5:07 PM

allanjude added inline comments.

usr.sbin/bhyve/pci_nvme.c
685	This changes the value from 310 to 296, is that intentional?

I like wanpengqian_gmail.com's suggestions as well.
310 vs 296 isn't a meaningful change. 37C vs 23C
Personally, I'd report 303 since that's 30C, but the actual number really doesn't matter.

usr.sbin/bhyve/pci_nvme.c
549	how are these numbers computed?
1068	seems unrelated, but good.

chuck marked an inline comment as done.Dec 23 2021, 9:03 PM

chuck added inline comments.

usr.sbin/bhyve/pci_nvme.c
549	NVMe 2.0, Figure 275 includes in the description for `WCTEMP` It is recommended that implementations report a value of 0157h in this field.
685	Yes, the change was intentional. Both values are arbitrary, but 296 is room temperature, which I liked better.

chuck added inline comments.Dec 23 2021, 9:23 PM

usr.sbin/bhyve/pci_nvme.c
1706	No, because unsupported `TMPSEL` values need to return an error.

Reduced size of tmpsel

Harbormaster completed remote builds in B43574: Diff 100519.Dec 23 2021, 9:26 PM

wanpengqian_gmail.com added inline comments.Dec 23 2021, 9:33 PM

usr.sbin/bhyve/pci_nvme.c
1706	sorry, I mean this part ' (thsel > NVME_TEMP_THRESH_UNDER)' first time I read it I have to check specification to find out what it means. Its fine, I understand now.