RE: [PATCH] mei: improve Denverton HSM & IFSI support

[Winkler, Tomas]

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

I'm not familiar with this firmware how do I know this firmware supports correct HECI protocol?  I need someone from Intel to confirm.
Thanks 
Tomas
> 
> Lukas