Page MenuHomeFreeBSD

intel: add hfsts debug-probe driver for Intel ME/HECI firmware status
Needs ReviewPublic

Authored by seuros on Sat, Aug 15, 8:12 PM.
Referenced Files
F170961719: D58863.id185133.diff
Mon, Sep 7, 9:27 PM
F170885135: D58863.id184553.diff
Mon, Sep 7, 7:35 AM
F170864432: D58863.id185127.diff
Mon, Sep 7, 3:09 AM
Unknown Object (File)
Sun, Sep 6, 9:27 PM
Unknown Object (File)
Sun, Sep 6, 9:26 PM
Unknown Object (File)
Sun, Sep 6, 9:08 PM
Unknown Object (File)
Sun, Sep 6, 10:18 AM
Unknown Object (File)
Sun, Sep 6, 5:36 AM

Details

Summary

Add an amd64-only, Intel-only diagnostic driver that reads ME Host
Firmware Status registers from PCI configuration space without mapping the messaging BAR.

Select the HFS register count per device generation, decode sparse
HFS1 state, mode, and error fields, and expose raw and summarized
status via sysctl. Register an ISA-side hfstsfd probe for supported
RCBA/FD2 generations so firmware-hidden HECI functions can be diagnosed.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 75741
Build 72624: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
adrian added inline comments.
sys/dev/mei/mei_pci.c
34 ↗(On Diff #184110)

Do you mind moving register definitions into mei_reg.h ? the strings are fine in the .c file as-is.

444 ↗(On Diff #184110)

should this be a separate source file?

share/man/man4/mei.4
66 ↗(On Diff #184111)

This is probably more appropriate for the commit message not the man page

73 ↗(On Diff #184111)

This kind of detail is probably more appropriate for a block comment inside the source code. The man page should just list what PCHs are supported.

sys/dev/mei/mei_fd.c
36 ↗(On Diff #184111)

If you define the PCI IDs locally here I think this include can be dropped?

77 ↗(On Diff #184111)

PPT16–PPT31 can be covered by this code

105 ↗(On Diff #184111)

I think this is inverted, and we shouldn't do BUS_ADD_CHILD if it is missing?

112 ↗(On Diff #184111)

I think this function should reject when the me isn't found

141 ↗(On Diff #184111)

Use device_get_nameunit(isab) as a param instead of coding isa0 directly

172 ↗(On Diff #184111)

This could be tightened, you only need space for the read4

186 ↗(On Diff #184111)

This should probably be boot verbose

sys/dev/mei/mei_pci.c
339 ↗(On Diff #184111)

These all capture snapshots of hfs during attach, I'd make these live sysctl handlers and re-read config space on depand

369 ↗(On Diff #184111)

I would call this D0i3_supported. It also isn't a power state (applicable to man page too)

Previously tested in every generation till Raptor Lake.

Tested now in Meteor Lake , Lunar Lake and Arrow Lake.

Missing is Panther Lake, and few Laptop architectures.

Renamed the driver. Since this is a probe only driver, it need it own name.
The mei driver will confuse both devs and other driver that expect the mei
name to be the full implmentation. The i915 driver talk to the ME to get HDCP support.

seuros retitled this revision from mei: add debug-probe driver for Intel ME/HECI firmware status to intel: add hfsts debug-probe driver for Intel ME/HECI firmware status.Wed, Aug 19, 1:56 PM
seuros edited the summary of this revision. (Show Details)

This is close, a couple inline nits still to address. I think the sysctls should be live queries, not just display attach time cached values. I'm not sure if the rename is the right direction; this claims the mei attachment exclusively so maybe just call it that the limited scope is in the man page. If someone wants to wire up extended functionality in the future this is a good base to do so?

sys/dev/intel/intel_hfsts.c
58–98
  • CWS 6: Platform Disable Wait
  • CWS 7: Operation State Transition
  • CWS 8: Invalid CPU
  • Operation state 6: Bring up

hfsts_state_summary() classifies valid CWS values 6–8 as Absent

See https://github.com/coreboot/coreboot/blob/main/util/intelmetool/me.h for values

369

Still needs to be renamed d0i3_supported an man page updated accordingly

I'm not sure if the rename is the right direction; this claims the mei attachment exclusively so maybe just call it that the limited scope is in the man page. If someone wants to wire up extended functionality in the future this is a good base to do so?

This driver probes and attaches to the MEI/HECI PCI function no matter of whether the ME is actually alive, it only decodes HFS1-6 out of PCI config space, so it attaches even when the ME firmware is disabled (HAP bit set, neutered coreboot boards, X200-class Libreboot machines where the ME region is yanked).

A live-protocol mei(4) driver needs the ME to actually answer the handshake over the messaging BAR to attach at all.

So the two have different attach preconditions, not just different feature scope.

I renamed it because I want the mei name reserved for that real driver when it's written, which we will need for multi-GPU support,HDCP and lot other feauture/capabilities.

That's a much bigger driver: Linux's drivers/misc/mei is 20k+ LOC even before adding board-specific quirks.

If Intel or some company decides to sponsor someone or even author it, the mei namespace will be available.

I might later try to take a stab at mei(4). I currently have all the hardware to test across multiple generation.

PS: I'm fine in renaming it to something else like intel_fwstatus or me_status.. ect..

adress comments. clean copy/pasted code while prototyping.

This is substantively fine, I would fix up these 6 new suggestions and then see if @adrian has any desire for the naming.

share/man/man4/intel_hfsts.4
198

I think the driver comes up as dev.hfsts? make sure whatever it ends up as is matched everywhere.

sys/conf/files.amd64
222

should be isa pci now

sys/dev/intel/intel_hfsts.c
457

If you want the contract where a later driver can overcome this, BUS_PROBE_GENERIC

495

should probably be boot verbose

505

An stance could be made either way for boot verbose, but this might be the substantive one to leave exposed everywhere as you have it so I think this is good.

sys/dev/intel/intel_hfsts_isa.c
122

We should BUS_ADD_CHILD if the pci device already populated hfsts_devices?

This revision is now accepted and ready to land.Fri, Aug 21, 5:04 AM
sys/dev/intel/intel_hfsts_isa.c
122

Meant to say: should not

Shall we simply name it hfsts ? It loads fine on my MacBook13,1

share/man/man4/intel_hfsts.4
198

I think the driver comes up as dev.hfsts? make sure whatever it ends up as is matched everywhere.

> sysctl dev.hfsts.0
dev.hfsts.0.summary: Normal
dev.hfsts.0.error: None
dev.hfsts.0.opmode_str: Normal
dev.hfsts.0.opstate: M0 (UMA)
dev.hfsts.0.state: Normal
dev.hfsts.0.update_in_progress: 0
dev.hfsts.0.mfg_mode: 0
dev.hfsts.0.fw_sku: 1
dev.hfsts.0.pm_event: 11
dev.hfsts.0.opmode: 0
dev.hfsts.0.d0i3_supported: 1
dev.hfsts.0.hfs6: 1073741824
dev.hfsts.0.hfs5: 0
dev.hfsts.0.hfs4: 16384
dev.hfsts.0.hfs3: 16
dev.hfsts.0.hfs2: 185271558
dev.hfsts.0.hfs1: 2415919685
dev.hfsts.0.%iommu: rid=0xb0
dev.hfsts.0.%parent: pci0
dev.hfsts.0.%pnpinfo: vendor=0x8086 device=0x9d3a subvendor=0x8086 subdevice=0x7270 class=0x078000
dev.hfsts.0.%location: slot=22 function=0 dbsf=pci0:0:22:0 handle=\_SB_.PCI0.IMEI
dev.hfsts.0.%driver: hfsts
dev.hfsts.0.%desc: Sunrise Point MEI
This revision now requires review to proceed.Wed, Aug 26, 2:15 AM
This revision is now accepted and ready to land.Wed, Aug 26, 6:25 AM

Some followups: Add NOTES entry on commit. Later consider MODULE_PNP_INFO. LGTM.

This revision now requires review to proceed.Wed, Aug 26, 10:09 AM
This revision is now accepted and ready to land.Wed, Aug 26, 10:20 AM

@ziaee Your signoff on the man page. Then we can land this.

I think hfstsfd(4) manpage should be a symbolic link to hfsts(4).

I find it a bit confusing to understand why I don't have dev.hfstsfd.0 on my machine. The manpage does not explain this very well ("modeled after ichwd_identify()" is not a good explanation to the user).

The source code, however, does this nicely:

/* MEI enumerated on PCI, so it is not firmware-hidden. */
if (hfsts_pci_present())
    return;

So maybe something like this:

If Host Firmware Status registers are available on the PCI bus, the `hfstsfd` driver is not needed and will not be loaded.

Do I understand it right that, if hfstsfd driver attaches, dev.hfsts.0 will NOT be available?

Another question, will other Management Engine functions be available (not supported by this read-only driver) if hfsts is disabled by the firmware?

I'm waiting for @ziaee's review before i reword it, but i agree with you

On dev.hfstsfd.0, yes, they are mutually exclusive.
If MEI is visible on PCI, hfsts_fd_identify() returns before adding the child, and hfsts attaches normally.
So you get either dev.hfsts.0 or dev.hfstsfd.0, never together.

For your MacBook, there are two reasons hfstsfd does not attach. First, MEI is already visible.
Second, hfstsfd only supports Ibex Peak through Wildcat Point, where I know the RCBA+FD2 layout.
Sunrise Point uses the newer P2SB/PCR mechanism, that i did not dig into.
The PCH list in the man page implies this, but I will make it clearer.

For your last question: no. If firmware hides HECI with Function Disable because it neutered, defective or HAP disabled,
the PCI function does not exist from the host's point of view. A full mei(4) would not even attach.

It could use a HARDWARE section too, but we can always add that later.

share/man/man4/hfsts.4
1–25 ↗(On Diff #185133)
27 ↗(On Diff #185133)

This interface is only for amd64 architecture, right?

32 ↗(On Diff #185133)

Nm and Nd are always together, so there is no need to repeat hfsts for a second time.

34–48 ↗(On Diff #185133)
52–53 ↗(On Diff #185133)
54–56 ↗(On Diff #185133)

I would get rid of this sentence because only Intel amd64 platforms have Intel Management Engine. Like saying that the floppy disk driver is only supporting floppy disks.

Also, I would prefer to see a brief introductory sentence here explaining what hfstsd is. That should address saper's concern?

Usually when we have $DRIVER and $DRIVERd, driverd is a daemon, so if that's what it is it needs a separate page in section 8. it doesnt seem like that's what it is, so hstsd is probably not the best name for it? ichwd is different because wd is watchdog. maybe hfstsisa or whatever you want?

75 ↗(On Diff #185133)

Dq takes the rest of the line as an argument

85 ↗(On Diff #185133)
122 ↗(On Diff #185133)

Right?

125 ↗(On Diff #185133)
172 ↗(On Diff #185133)
197 ↗(On Diff #185133)

Dl takes the rest of the line as an argument, so no need to quote here.

202 ↗(On Diff #185133)

Ditto

Also, sorry for the delay, and thanks so much for waiting for me and pinging me again! I actually didn't see the first one.

Just whitespace nitpicks.

sys/dev/intel/hfsts.c
33–56 ↗(On Diff #185133)
69–70 ↗(On Diff #185133)
79 ↗(On Diff #185133)
85 ↗(On Diff #185133)
89–125 ↗(On Diff #185133)
sys/dev/intel/hfsts_isa.c
161 ↗(On Diff #185133)

The more I think about it I somehow think maybe the FD register check should end up in the isab(4) driver.

Maybe we should also think how to collect common knowledge in some ich.c/ich,.h files so that all the ich* drivers can share that and get rid of /sys/dev/intel - but this is not for this PR :)

Regarding the documentation, I understand from my reading of the code that all the play about having the MEI functions enabled/disabled is done in the PCI-ISA bridge component register, but this has nothing to do with the classic ISA or LPC bus. Am I right? In that case we shall not mention "ISA" or "LPC" in the documentation unless some classic ISA resources such as I/O ports are accessed. We should at most mention "PCI-ISA bridge function" or isab(4). If this is correct, "hfstsd" code could live in the isab(4) driver.

Not sure if all this makes any sense, I just try to grasp the code.

seuros marked 3 inline comments as done.

reformat man page

This revision now requires review to proceed.Sun, Sep 6, 7:25 PM