On Fri, 20 Aug 2021 10:28:21 +0200 Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> wrote: > On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson > <alex.williamson@xxxxxxxxxx> wrote: > > > > On Thu, 19 Aug 2021 10:07:03 -0500 > > Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > > [+cc Alex] > > > > > > On Thu, Aug 19, 2021 at 04:51:14PM +0200, Lukas Bulwahn wrote: > > > > The Intel Denverton chip provides HSM & IFSI. In order to access > > > > HSM & IFSI at the same time, provide two HECI hardware IDs for accessing. > > > > > > > > Suggested-by: Ionel-Catalin Mititelu <ionel-catalin.mititelu@xxxxxxxxx> > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> > > > > --- > > > > Tomas, please pick this quick helpful extension for the hardware. > > > > > > > > drivers/misc/mei/hw-me-regs.h | 3 ++- > > > > drivers/misc/mei/pci-me.c | 1 + > > > > drivers/pci/quirks.c | 3 +++ > > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/misc/mei/hw-me-regs.h b/drivers/misc/mei/hw-me-regs.h > > > > index cb34925e10f1..c1c41912bb72 100644 > > > > --- a/drivers/misc/mei/hw-me-regs.h > > > > +++ b/drivers/misc/mei/hw-me-regs.h > > > > @@ -68,7 +68,8 @@ > > > > #define MEI_DEV_ID_BXT_M 0x1A9A /* Broxton M */ > > > > #define MEI_DEV_ID_APL_I 0x5A9A /* Apollo Lake I */ > > > > > > > > -#define MEI_DEV_ID_DNV_IE 0x19E5 /* Denverton IE */ > > > > +#define MEI_DEV_ID_DNV_IE 0x19E5 /* Denverton for HECI1 - IFSI */ > > > > +#define MEI_DEV_ID_DNV_IE_2 0x19E6 /* Denverton 2 for HECI2 - HSM */ > > > > > > > > #define MEI_DEV_ID_GLK 0x319A /* Gemini Lake */ > > > > > > > > diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c > > > > index c3393b383e59..30827cd2a1c2 100644 > > > > --- a/drivers/misc/mei/pci-me.c > > > > +++ b/drivers/misc/mei/pci-me.c > > > > @@ -77,6 +77,7 @@ static const struct pci_device_id mei_me_pci_tbl[] = { > > > > {MEI_PCI_DEVICE(MEI_DEV_ID_APL_I, MEI_ME_PCH8_CFG)}, > > > > > > > > {MEI_PCI_DEVICE(MEI_DEV_ID_DNV_IE, MEI_ME_PCH8_CFG)}, > > > > + {MEI_PCI_DEVICE(MEI_DEV_ID_DNV_IE_2, MEI_ME_PCH8_SPS_CFG)}, > > > > > > > > {MEI_PCI_DEVICE(MEI_DEV_ID_GLK, MEI_ME_PCH8_CFG)}, > > > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > > index 6899d6b198af..2ab767ef8469 100644 > > > > --- a/drivers/pci/quirks.c > > > > +++ b/drivers/pci/quirks.c > > > > @@ -4842,6 +4842,9 @@ static const struct pci_dev_acs_enabled { > > > > { PCI_VENDOR_ID_INTEL, 0x15b7, pci_quirk_mf_endpoint_acs }, > > > > { PCI_VENDOR_ID_INTEL, 0x15b8, pci_quirk_mf_endpoint_acs }, > > > > { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_rciep_acs }, > > > > + /* Denverton */ > > > > + { PCI_VENDOR_ID_INTEL, 0x19e5, pci_quirk_mf_endpoint_acs }, > > > > + { PCI_VENDOR_ID_INTEL, 0x19e6, pci_quirk_mf_endpoint_acs }, > > > > > > This looks like it should be a separate patch with a commit log that > > > explains it. For example, see these: > > > > > > db2f77e2bd99 ("PCI: Add ACS quirk for Broadcom BCM57414 NIC") > > > 3247bd10a450 ("PCI: Add ACS quirk for Intel Root Complex Integrated Endpoints") > > > 299bd044a6f3 ("PCI: Add ACS quirk for Zhaoxin Root/Downstream Ports") > > > 0325837c51cb ("PCI: Add ACS quirk for Zhaoxin multi-function devices") > > > 76e67e9e0f0f ("PCI: Add ACS quirk for Amazon Annapurna Labs root ports") > > > 46b2c32df7a4 ("PCI: Add ACS quirk for iProc PAXB") > > > 01926f6b321b ("PCI: Add ACS quirk for HXT SD4800") > > > > > > It should be acked by somebody at Intel since this quirk relies on > > > behavior of the device for VM security. > > > > +1 Thanks Bjorn. I got curious and AFAICT these functions are the > > interface for the host system to communicate with "Innovation Engine" > > processors within the SoC, which seem to be available for system > > builders to innovate and differentiate system firmware features. I'm > > not sure then how we can assume a specific interface ("HSM" or "IFSI", > > whatever those are) for each function, nor of course how we can assume > > isolation between them. Thanks, > > Alex, I got a Denverton hardware with Innovation Engine and the > specific system firmware (basically delivered from Intel). To make use > of that hardware, someone at Intel suggested adding these PCI ACS > quirks. It is unclear to me if there are various different Denverton > systems out there (I only got one!) with many different system > firmware variants for the Innovation Engine or if there is just one > Denverton with IE support and with one firmware from Intel, i.e., the > one I got. > > If there is only one or two variants of the Denverton with Innovation > Engine firmware out there, then we could add this ACS quirk here > unconditionally (basically assuming that if the other firmware is > there, the IE would just do the right thing, e.g., deny any operation > for a non-existing firmware function), right? Just adding a commit > similar to the commits Bjorn pointed out above. Otherwise, we would > need to make that conditional for possible different variants, but I > would need a bit more guidance from you on which other variants exist > and how one can differentiate between them. Hi Lukas, I'm looking at the C3000 datasheet, Intel document #337018-002, where I see: 1.2.7 Innovation Engine (IE) ... For the IE, the system builder can install an embedded operating system, drivers and application they develop on their own, or purchase them from a third-party vendor. Intel does not provide operating systems, drivers or applications for the IE. 15.2.3.1 Interrupt Timer Sub System (ITSS) ... The Innovation Engine (IE) has a sideband connection to the ITSS components. 16 Power Management Controller (PMC) ... 16.2 Feature List ... • Interacts with the SoC Innovation Engine (IE) Table 16-4. Causes of SMI and SCI ... [IE can cause SMI or SCI] 16.10.1 Initiating State Changes when in the G0 (S0) Working State ... The Intel® Management Engine and Innovation Engine firmware each has a mechanism to turn off a hung system similar to the Power-Button Override by writing bits in their power-management control registers. And the apparent coup de grâce: 37 Innovation Engine The Innovation Engine (IE) is an optional, complete, embedded engine intended to enable SoC customers to provide their own custom system management. This chapter provides a brief overview of the IE. It is reserved for system-builder code, not for Intel firmware since Intel supplies IE hardware only. IE activation is not required for normal system operation. ... IE is a completely optional feature, and is disabled by default in the silicon. It can be enabled by system builders and OEMs to run signed firmware created by the system builder or a third party software vendor. IE is not like the Intel® Management Engine (Intel® ME) where Intel provides the HW plus a complete FW solution. Intel only provides IE hardware (along with collateral and tools enabling). For the HECI, I see: 37.3 Architectural Overview ... The devices exposed by the IE subsystem to the Host Root Space are: • HECI (1, 2 and 3) – These functions define the mechanism for host software and IE firmware to communicate. This device exposes three PCI functions to the host during PCI bus enumeration. The message format is OEM dependent and communication between host and IE subsystem takes place via circular buffers and control/status registers. This function supports host MSI, SMI and SCI# interrupt generation mechanisms. So I don't see how the datasheet supports that there's either any specific API defined per HECI interface or that these functions would ever be intended in a generic way for independent use of by a userspace driver or VM. Perhaps with DMI or ACPI info an HECI could be associated to a specific vendor API, by why we'd describe them as using isolated IOMMU grouping is a complete mystery to me. Thanks, Alex