bhyve: Fix the wrong Number of Power States Support value also set a reasonable Max Power for NVMe controller
ClosedPublic
Actions

Authored by wanpengqian_gmail.com on Oct 26 2021, 5:46 AM.

Details

Reviewers

chuck
jhb
markj

Group Reviewers

bhyve

Commits

rGeae0210cdd2e: bhyve: Fix Number of Power States Supported value

Summary

Number of Power States Support is a 0's based value.
0 indicate 1 power state.

This patch will set 1 power state, also set the Max Power to 10 Watts.
(Max Power Scale is 0, means 0.01 Watts based)

Signed-off-by: Wanpeng Qian <wanpengqian@gmail.com>

Test Plan

Within FreeBSD/Linux guests, list the number of power states and check the Max Power value.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

wanpengqian_gmail.com created this revision.Oct 26 2021, 5:46 AM

Herald added a reviewer: bhyve. · View Herald TranscriptOct 26 2021, 5:46 AM

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

wanpengqian_gmail.com requested review of this revision.Oct 26 2021, 5:46 AM

Harbormaster completed remote builds in B42363: Diff 97453.Oct 26 2021, 5:46 AM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)Oct 26 2021, 7:16 AM

wanpengqian_gmail.com retitled this revision from bhyve: correct the Number of Power States Support and set a reasonable Max Power for NVMe controller to bhyve: Fix the wrong Number of Power States Support value also set a reasonable Max Power for NVMe controller.Nov 25 2021, 8:57 AM

Looks ok. I'd explicitly initialize MXPS and NOPS to zero as well.

This revision is now accepted and ready to land.Nov 29 2021, 4:45 PM

explicitly initialize MXPS and NOPS to zero as well

This revision now requires review to proceed.Nov 29 2021, 9:26 PM

Harbormaster completed remote builds in B43030: Diff 99179.Nov 29 2021, 9:26 PM

In D32657#749683, @wanpengqian_gmail.com wrote:

explicitly initialize MXPS and NOPS to zero as well

Thank you very much. Now initialize the mps_nops to zero as well.

Sorry to nitpick a bit more. Please let me know if you'd like me to commit the change.

usr.sbin/bhyve/pci_nvme.c
553–557	I think the comment should also clarify that the value is zero-indexed, otherwise this line looks wrong.

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

update comment as suggested.

This revision now requires review to proceed.Nov 29 2021, 9:59 PM

Harbormaster completed remote builds in B43032: Diff 99182.Nov 29 2021, 9:59 PM

Yes, please commit this for me. thank you.

wanpengqian_gmail.com edited the summary of this revision. (Show Details)Dec 4 2021, 12:51 AM

wanpengqian_gmail.com removed a reviewer: grehan.Jan 12 2022, 6:19 AM

Can we merge this patch to HEAD?

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

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

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

This revision is now accepted and ready to land.Aug 14 2022, 3:51 PM

Closed by commit rGeae0210cdd2e: bhyve: Fix Number of Power States Supported value (authored by wanpengqian_gmail.com, committed by chuck). · Explain WhyAug 14 2022, 6:18 PM

This revision was automatically updated to reflect the committed changes.

chuck added a commit: rGeae0210cdd2e: bhyve: Fix Number of Power States Supported value.