linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mei: improve Denverton HSM & IFSI support
@ 2021-08-19 14:51 Lukas Bulwahn
  2021-08-19 15:07 ` Bjorn Helgaas
  0 siblings, 1 reply; 10+ messages in thread
From: Lukas Bulwahn @ 2021-08-19 14:51 UTC (permalink / raw)
  To: Tomas Winkler
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas, linux-pci,
	Ionel-Catalin Mititelu, Jiri Kosina, linux-kernel, Lukas Bulwahn

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@intel.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
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 },
 	/* QCOM QDF2xxx root ports */
 	{ PCI_VENDOR_ID_QCOM, 0x0400, pci_quirk_qcom_rp_acs },
 	{ PCI_VENDOR_ID_QCOM, 0x0401, pci_quirk_qcom_rp_acs },
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-19 14:51 [PATCH] mei: improve Denverton HSM & IFSI support Lukas Bulwahn
@ 2021-08-19 15:07 ` Bjorn Helgaas
  2021-08-19 20:10   ` Alex Williamson
  2021-08-20 13:20   ` Lukas Bulwahn
  0 siblings, 2 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2021-08-19 15:07 UTC (permalink / raw)
  To: Lukas Bulwahn
  Cc: Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas,
	linux-pci, Ionel-Catalin Mititelu, Jiri Kosina, linux-kernel,
	Alex Williamson

[+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@intel.com>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> 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.

>  	/* QCOM QDF2xxx root ports */
>  	{ PCI_VENDOR_ID_QCOM, 0x0400, pci_quirk_qcom_rp_acs },
>  	{ PCI_VENDOR_ID_QCOM, 0x0401, pci_quirk_qcom_rp_acs },
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-19 15:07 ` Bjorn Helgaas
@ 2021-08-19 20:10   ` Alex Williamson
  2021-08-20  8:28     ` Lukas Bulwahn
  2021-08-20 13:20   ` Lukas Bulwahn
  1 sibling, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2021-08-19 20:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Lukas Bulwahn, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman,
	Bjorn Helgaas, linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	linux-kernel

On Thu, 19 Aug 2021 10:07:03 -0500
Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> > 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-19 20:10   ` Alex Williamson
@ 2021-08-20  8:28     ` Lukas Bulwahn
  2021-08-20 13:14       ` Winkler, Tomas
  2021-08-20 15:45       ` Alex Williamson
  0 siblings, 2 replies; 10+ messages in thread
From: Lukas Bulwahn @ 2021-08-20  8:28 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Bjorn Helgaas, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman,
	Bjorn Helgaas, linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	Linux Kernel Mailing List

On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Thu, 19 Aug 2021 10:07:03 -0500
> Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > ---
> > > 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.

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-20  8:28     ` Lukas Bulwahn
@ 2021-08-20 13:14       ` Winkler, Tomas
  2021-08-20 15:45       ` Alex Williamson
  1 sibling, 0 replies; 10+ messages in thread
From: Winkler, Tomas @ 2021-08-20 13:14 UTC (permalink / raw)
  To: Lukas Bulwahn, Alex Williamson
  Cc: Bjorn Helgaas, Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas,
	linux-pci, Mititelu, Ionel-catalin, Jiri Kosina,
	Linux Kernel Mailing List


> 
> On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Thu, 19 Aug 2021 10:07:03 -0500
> > Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > ---
> > > > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-19 15:07 ` Bjorn Helgaas
  2021-08-19 20:10   ` Alex Williamson
@ 2021-08-20 13:20   ` Lukas Bulwahn
  1 sibling, 0 replies; 10+ messages in thread
From: Lukas Bulwahn @ 2021-08-20 13:20 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas,
	linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	Linux Kernel Mailing List, Alex Williamson

On Thu, Aug 19, 2021 at 5:07 PM Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> > 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.
>

Bjorn, I will happily split this into two patches and follow the
general conventions as soon as we have somebody at Intel to confirm on
this email thread that the proposal basically makes sense or if this
is actually flawed and why (although it was initially proposed by
somebody at Intel in another off-list discussion).

Lukas

> >       /* QCOM QDF2xxx root ports */
> >       { PCI_VENDOR_ID_QCOM, 0x0400, pci_quirk_qcom_rp_acs },
> >       { PCI_VENDOR_ID_QCOM, 0x0401, pci_quirk_qcom_rp_acs },
> > --
> > 2.26.2
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-20  8:28     ` Lukas Bulwahn
  2021-08-20 13:14       ` Winkler, Tomas
@ 2021-08-20 15:45       ` Alex Williamson
  2021-08-20 16:25         ` Lukas Bulwahn
  1 sibling, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2021-08-20 15:45 UTC (permalink / raw)
  To: Lukas Bulwahn
  Cc: Bjorn Helgaas, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman,
	Bjorn Helgaas, linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	Linux Kernel Mailing List

On Fri, 20 Aug 2021 10:28:21 +0200
Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:

> On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Thu, 19 Aug 2021 10:07:03 -0500
> > Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > ---
> > > > 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-20 15:45       ` Alex Williamson
@ 2021-08-20 16:25         ` Lukas Bulwahn
  2021-08-20 17:04           ` Alex Williamson
  0 siblings, 1 reply; 10+ messages in thread
From: Lukas Bulwahn @ 2021-08-20 16:25 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Bjorn Helgaas, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman,
	Bjorn Helgaas, linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	Linux Kernel Mailing List

On Fri, Aug 20, 2021 at 5:45 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Fri, 20 Aug 2021 10:28:21 +0200
> Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> > On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Thu, 19 Aug 2021 10:07:03 -0500
> > > Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > > ---
> > > > > 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.
>

Well, IMHO, my observation of what Intel provided to me clearly
contradicts that statement. It seems that Intel did provide an
operating system, driver and applications for the IE, and suggested
modifying/extending the kernel sources for that purpose beyond what
was already existing in the kernel tree, which already suggests by
itself that Intel has a specific driver and application for the IE in
mind.

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

I agree with that mystery, but I do not know if I should rather trust
the Intel documentation you cite or simply the bits and pieces that
already landed in the kernel tree here for the Denverton IE.

Am I right that we are basically stuck here without any further
explanation by somebody from Intel?

Do I also get it right that:

If we would trust the Intel documentation, we would not really see the
purpose of the existing line
MEI_PCI_DEVICE(MEI_DEV_ID_DNV_IE, MEI_ME_PCH8_CFG) in
drivers/misc/mei/pci-me.c, added with commit f7ee8ead151f ("mei: me:
add denverton innovation engine device IDs"), because that also
depends on the existence of a specific system-builder code?

Thanks for all your explanations and pointers, Alex.

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-20 16:25         ` Lukas Bulwahn
@ 2021-08-20 17:04           ` Alex Williamson
  2021-08-23  6:54             ` Lukas Bulwahn
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2021-08-20 17:04 UTC (permalink / raw)
  To: Lukas Bulwahn
  Cc: Bjorn Helgaas, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman,
	Bjorn Helgaas, linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	Linux Kernel Mailing List

On Fri, 20 Aug 2021 18:25:04 +0200
Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:

> On Fri, Aug 20, 2021 at 5:45 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Fri, 20 Aug 2021 10:28:21 +0200
> > Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >  
> > > On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson
> > > <alex.williamson@redhat.com> wrote:  
> > > >
> > > > On Thu, 19 Aug 2021 10:07:03 -0500
> > > > Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > > > ---
> > > > > > 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.
> >  
> 
> Well, IMHO, my observation of what Intel provided to me clearly
> contradicts that statement. It seems that Intel did provide an
> operating system, driver and applications for the IE, and suggested
> modifying/extending the kernel sources for that purpose beyond what
> was already existing in the kernel tree, which already suggests by
> itself that Intel has a specific driver and application for the IE in
> mind.

But in your case is Intel both the SoC vendor and system builder?  It's
specifically noted below that Intel does not provide a complete IE FW
solution to 3rd parties, regardless of any standardization that might
(or might not) exist among Intel developed solutions based on this SoC.
This doesn't contradict the datasheet.
 
> > 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, 
> 
> I agree with that mystery, but I do not know if I should rather trust
> the Intel documentation you cite or simply the bits and pieces that
> already landed in the kernel tree here for the Denverton IE.
> 
> Am I right that we are basically stuck here without any further
> explanation by somebody from Intel?
> 
> Do I also get it right that:
> 
> If we would trust the Intel documentation, we would not really see the
> purpose of the existing line
> MEI_PCI_DEVICE(MEI_DEV_ID_DNV_IE, MEI_ME_PCH8_CFG) in
> drivers/misc/mei/pci-me.c, added with commit f7ee8ead151f ("mei: me:
> add denverton innovation engine device IDs"), because that also
> depends on the existence of a specific system-builder code?

The existing entry was added by Tomas in commit f7ee8ead151f ("mei: me:
add denverton innovation engine device IDs") which claims IE is an
ME-like device which provides HW security offload.  I expect there is
the ability to provide such an offload, but I'm afraid this was added
relative to a specific implementation of IE that we really can't
determine by the device ID alone according to the datasheet.

I don't know the MEI code, does it further probe for a compatible
software interface on these device IDs or are we likely to run into the
weeds?

I think we're stuck without some public comment from Intel.  I don't
necessarily have high confidence in the existing entry.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mei: improve Denverton HSM & IFSI support
  2021-08-20 17:04           ` Alex Williamson
@ 2021-08-23  6:54             ` Lukas Bulwahn
  0 siblings, 0 replies; 10+ messages in thread
From: Lukas Bulwahn @ 2021-08-23  6:54 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Bjorn Helgaas, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman,
	Bjorn Helgaas, linux-pci, Ionel-Catalin Mititelu, Jiri Kosina,
	Linux Kernel Mailing List

On Fri, Aug 20, 2021 at 7:04 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Fri, 20 Aug 2021 18:25:04 +0200
> Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> > On Fri, Aug 20, 2021 at 5:45 PM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Fri, 20 Aug 2021 10:28:21 +0200
> > > Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > >
> > > > On Thu, Aug 19, 2021 at 10:10 PM Alex Williamson
> > > > <alex.williamson@redhat.com> wrote:
> > > > >
> > > > > On Thu, 19 Aug 2021 10:07:03 -0500
> > > > > Bjorn Helgaas <helgaas@kernel.org> 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@intel.com>
> > > > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > > > > ---
> > > > > > > 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.
> > >
> >
> > Well, IMHO, my observation of what Intel provided to me clearly
> > contradicts that statement. It seems that Intel did provide an
> > operating system, driver and applications for the IE, and suggested
> > modifying/extending the kernel sources for that purpose beyond what
> > was already existing in the kernel tree, which already suggests by
> > itself that Intel has a specific driver and application for the IE in
> > mind.
>
> But in your case is Intel both the SoC vendor and system builder?  It's
> specifically noted below that Intel does not provide a complete IE FW
> solution to 3rd parties, regardless of any standardization that might
> (or might not) exist among Intel developed solutions based on this SoC.
> This doesn't contradict the datasheet.
>

I guess you are right: in my case, Intel is both a SoC vendor and
system builder. Unfortunately, I do not have much more information
from Intel on the technical specifics of what they did as system
builder that is not covered by the Intel Denverton C3000 data sheet,
though.

> > > 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,
> >
> > I agree with that mystery, but I do not know if I should rather trust
> > the Intel documentation you cite or simply the bits and pieces that
> > already landed in the kernel tree here for the Denverton IE.
> >
> > Am I right that we are basically stuck here without any further
> > explanation by somebody from Intel?
> >
> > Do I also get it right that:
> >
> > If we would trust the Intel documentation, we would not really see the
> > purpose of the existing line
> > MEI_PCI_DEVICE(MEI_DEV_ID_DNV_IE, MEI_ME_PCH8_CFG) in
> > drivers/misc/mei/pci-me.c, added with commit f7ee8ead151f ("mei: me:
> > add denverton innovation engine device IDs"), because that also
> > depends on the existence of a specific system-builder code?
>
> The existing entry was added by Tomas in commit f7ee8ead151f ("mei: me:
> add denverton innovation engine device IDs") which claims IE is an
> ME-like device which provides HW security offload.  I expect there is
> the ability to provide such an offload, but I'm afraid this was added
> relative to a specific implementation of IE that we really can't
> determine by the device ID alone according to the datasheet.
>
> I don't know the MEI code, does it further probe for a compatible
> software interface on these device IDs or are we likely to run into the
> weeds?
>
> I think we're stuck without some public comment from Intel.  I don't
> necessarily have high confidence in the existing entry.  Thanks,
>

Agree. All in all, we need some comments from Intel to understand and
confirm the existing entry and the proposed additions here.

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-23  6:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 14:51 [PATCH] mei: improve Denverton HSM & IFSI support Lukas Bulwahn
2021-08-19 15:07 ` Bjorn Helgaas
2021-08-19 20:10   ` Alex Williamson
2021-08-20  8:28     ` Lukas Bulwahn
2021-08-20 13:14       ` Winkler, Tomas
2021-08-20 15:45       ` Alex Williamson
2021-08-20 16:25         ` Lukas Bulwahn
2021-08-20 17:04           ` Alex Williamson
2021-08-23  6:54             ` Lukas Bulwahn
2021-08-20 13:20   ` Lukas Bulwahn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).