Agreed on all comments - will fix them all. Thanks Daniele, Nikula.
...alan
On Wed, 2022-11-02 at 11:35 +0200, Jani Nikula wrote:
> On Tue, 01 Nov 2022, "Ceraolo Spurio, Daniele"
> <daniele.ceraolospu...@intel.com> wrote:
> > On 10/24/2022 11:40 AM, Alan Previn wrote:
> > > Previously, we only used PXP FW interface version-42 structures for
> > > PXP arbitration session on ADL/TGL products and version-43 for HuC
> > > authentication on DG2. That worked fine despite not differentiating such
> > > versioning of the PXP firmware interaction structures. This was okay
> > > back then because the only commands used via version 42 was not
> > > used via version 43 and vice versa.
> > >
> > > With MTL, we'll need both these versions side by side for the same
> > > commands (PXP-session) with the older platform feature support. That
> > > said, let's create separate files to define the structures and definitions
> > > for both version-42 and 43 of PXP FW interfaces.
> > >
> > > Signed-off-by: Alan Previn <alan.previn.teres.ale...@intel.com>
> > > ---
> > > .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 39 +++++++++++++
> > > .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 45 +++++++++++++++
> > > .../i915/pxp/intel_pxp_cmd_interface_cmn.h | 27 +++++++++
> > > drivers/gpu/drm/i915/pxp/intel_pxp_huc.c | 20 +++----
> > > drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 12 ++--
> > > .../drm/i915/pxp/intel_pxp_tee_interface.h | 57 -------------------
> > > 6 files changed, 127 insertions(+), 73 deletions(-)
> > > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > > create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> > > create mode 100644
> > > drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > > delete mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee_interface.h
> > >
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > > new file mode 100644
> > > index 000000000000..501012d3084d
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > > @@ -0,0 +1,39 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef __INTEL_PXP_FW_INTERFACE_42_H__
> > > +#define __INTEL_PXP_FW_INTERFACE_42_H__
> > > +
> > > +#include <linux/types.h>
> > > +#include "intel_pxp_cmd_interface_cmn.h"
> > > +
> > > +/* PXP API Version 42 Core Definitions */
> > > +#define PXP42_APIVER 0x00040002
> >
> > Is it worth having a unified macro for this instead of 2 separate
> > defines for 42 and 43? e.g
> >
> > #define PXP_APIVER(x, y) (x << 16 | y)
> >
> > And then use PXP_APIVER(4, 2) or PXP_APIVER(4, 3). Just a suggestion,
> > not a blocker.
> >
> > > +
> > > +/* PXP-Cmd-Op definitions */
> > > +#define PXP42_CMDID_INIT_SESSION 0x1e
> >
> > This might be better off closer to the matching structure. Not a blocker.
> >
> > > +
> > > +/* PXP-In/Out-Cmd-Header */
> > > +struct pxp42_cmd_header {
> > > + struct pxpcmn_cmd_header header;
> > > + u32 status;
> > > + /* Length of the message (excluding the header) */
> > > + u32 buffer_len;
> > > +} __packed;
> >
> > The PXP specs indicate that the header is common between v42 and v43,
> > with one field being considered a union, so we can just define it as
> > fully shared in the common file. Something like:
>
> Agreed. Using separate structs is going to lead to trouble with any
> shared code.
>
> BR,
> Jani.
>
>
> >
> > struct pxp_cmd_header {
> > u32 api_version;
> > u32 command_id;
> > union {
> > u32 status; /* out */
> > u32 stream id; /* in */
> > }
> > u32 buffer_len;
> > }
> >
> >
> >
> > > +
> > > +/* PXP-Input-Packet: Create-Arb-Session */
> > > +#define PXP42_INIT_SESSION_PROTECTION_ARB 0x2
> >
> > I was a bit confused by the comment above the define, took me a moment
> > to understand that the define is not of the command ID matching the
> > packed, but one of the possible values of one of the fields. Maybe move
> > it inside the structure and below the matching variable like we usually do?
> >
> > > +struct pxp42_create_arb_in {
> > > + struct pxp42_cmd_header header;
> > > + u32 protection_mode;
> > > + u32 session_id;
> > > +} __packed;
> > > +
> > > +/* PXP-Output-Packet: Create-Arb-Session */
> > > +struct pxp42_create_arb_out {
> > > + struct pxp42_cmd_header header;
> > > +} __packed;
> > > +
> > > +#endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> > > new file mode 100644
> > > index 000000000000..d7d93876bbef
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> > > @@ -0,0 +1,45 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright(c) 2022, Intel Corporation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef __INTEL_PXP_FW_INTERFACE_43_H__
> > > +#define __INTEL_PXP_FW_INTERFACE_43_H__
> > > +
> > > +#include <linux/types.h>
> > > +#include "intel_pxp_cmd_interface_cmn.h"
> > > +
> > > +/* PXP API Version 43 Core Definitions */
> > > +#define PXP43_APIVER 0x00040003
> > > +#define PXP43_MAX_HECI_IN_SIZE (32 * 1024)
> > > +#define PXP43_MAX_HECI_OUT_SIZE (32 * 1024)
> >
> > Those MAX_HECI defines are unused
> >
> > Daniele
> >
> > > +
> > > +/* PXP-Cmd-Op definitions */
> > > +#define PXP43_CMDID_START_HUC_AUTH 0x0000003A
> > > +
> > > +/* PXP-In/Out-Cmd-Header */
> > > +struct pxp43_cmd_header {
> > > + struct pxpcmn_cmd_header header;
> > > + u32 in_out_data;
> > > + /* Length of the message (excluding the header) */
> > > + u32 buffer_len;
> > > +} __packed;
> >
> > This is unused (but anyway superseded by previous comment about the
> > unified header)
> >
> > > +
> > > +/* PXP-Input-Packet: HUC-Authentication */
> > > +struct pxp43_start_huc_auth_in {
> > > + struct pxpcmn_cmd_header header;
> > > + u32 status;
> > > + /* Length of the message (excluding the header) */
> > > + u32 buffer_len;
> > > + __le64 huc_base_address;
> > > +} __packed;
> > > +
> > > +/* PXP-Output-Packet: HUC-Authentication */
> > > +struct pxp43_start_huc_auth_out {
> > > + struct pxpcmn_cmd_header header;
> > > + u32 status;
> > > + /* Length of the message (excluding the header) */
> > > + u32 buffer_len;
> > > +} __packed;
> > > +
> > > +#endif /* __INTEL_PXP_FW_INTERFACE_43_H__ */
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > > new file mode 100644
> > > index 000000000000..5c301ddc55e2
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > > @@ -0,0 +1,27 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright(c) 2022, Intel Corporation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef __INTEL_PXP_FW_INTERFACE_CMN_H__
> > > +#define __INTEL_PXP_FW_INTERFACE_CMN_H__
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/*
> > > + * there are a lot of status codes for PXP, but we only define the
> > > cross-API
> > > + * common ones that we actually can handle in the kernel driver. Other
> > > failure
> > > + * codes should be printed to error msg for debug.
> > > + */
> > > +enum pxp_status {
> > > + PXP_STATUS_SUCCESS = 0x0,
> > > + PXP_STATUS_OP_NOT_PERMITTED = 0x4013
> > > +};
> > > +
> > > +/* Common PXP FW message header */
> > > +struct pxpcmn_cmd_header {
> > > + u32 api_version;
> > > + u32 command_id;
> > > +} __packed;
> > > +
> > > +#endif /* __INTEL_PXP_FW_INTERFACE_CMN_H__ */
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c
> > > index 7ec36d94e758..ea8389d54963 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c
> > > @@ -13,14 +13,14 @@
> > > #include "intel_pxp_huc.h"
> > > #include "intel_pxp_tee.h"
> > > #include "intel_pxp_types.h"
> > > -#include "intel_pxp_tee_interface.h"
> > > +#include "intel_pxp_cmd_interface_43.h"
> > >
> > > int intel_pxp_huc_load_and_auth(struct intel_pxp *pxp)
> > > {
> > > struct intel_gt *gt = pxp_to_gt(pxp);
> > > struct intel_huc *huc = >->uc.huc;
> > > - struct pxp_tee_start_huc_auth_in huc_in = {0};
> > > - struct pxp_tee_start_huc_auth_out huc_out = {0};
> > > + struct pxp43_start_huc_auth_in huc_in = {0};
> > > + struct pxp43_start_huc_auth_out huc_out = {0};
> > > dma_addr_t huc_phys_addr;
> > > u8 client_id = 0;
> > > u8 fence_id = 0;
> > > @@ -32,10 +32,10 @@ int intel_pxp_huc_load_and_auth(struct intel_pxp *pxp)
> > > huc_phys_addr = i915_gem_object_get_dma_address(huc->fw.obj, 0);
> > >
> > > /* write the PXP message into the lmem (the sg list) */
> > > - huc_in.header.api_version = PXP_TEE_43_APIVER;
> > > - huc_in.header.command_id = PXP_TEE_43_START_HUC_AUTH;
> > > - huc_in.header.status = 0;
> > > - huc_in.header.buffer_len = sizeof(huc_in.huc_base_address);
> > > + huc_in.header.api_version = PXP43_APIVER;
> > > + huc_in.header.command_id = PXP43_CMDID_START_HUC_AUTH;
> > > + huc_in.status = 0;
> > > + huc_in.buffer_len = sizeof(huc_in.huc_base_address);
> > > huc_in.huc_base_address = huc_phys_addr;
> > >
> > > err = intel_pxp_tee_stream_message(pxp, client_id, fence_id,
> > > @@ -57,11 +57,11 @@ int intel_pxp_huc_load_and_auth(struct intel_pxp *pxp)
> > > * returned with HuC not loaded we'll still catch it when we
> > > check the
> > > * authentication bit later.
> > > */
> > > - if (huc_out.header.status != PXP_STATUS_SUCCESS &&
> > > - huc_out.header.status != PXP_STATUS_OP_NOT_PERMITTED) {
> > > + if (huc_out.status != PXP_STATUS_SUCCESS &&
> > > + huc_out.status != PXP_STATUS_OP_NOT_PERMITTED) {
> > > drm_err(>->i915->drm,
> > > "HuC load failed with GSC error = 0x%x\n",
> > > - huc_out.header.status);
> > > + huc_out.status);
> > > return -EPROTO;
> > > }
> > >
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > index 052fd2f9a583..7226becc0a82 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > @@ -14,7 +14,7 @@
> > > #include "intel_pxp.h"
> > > #include "intel_pxp_session.h"
> > > #include "intel_pxp_tee.h"
> > > -#include "intel_pxp_tee_interface.h"
> > > +#include "intel_pxp_cmd_interface_42.h"
> > > #include "intel_pxp_huc.h"
> > >
> > > static inline struct intel_pxp *i915_dev_to_pxp(struct device
> > > *i915_kdev)
> > > @@ -286,14 +286,14 @@ int intel_pxp_tee_cmd_create_arb_session(struct
> > > intel_pxp *pxp,
> > > int arb_session_id)
> > > {
> > > struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> > > - struct pxp_tee_create_arb_in msg_in = {0};
> > > - struct pxp_tee_create_arb_out msg_out = {0};
> > > + struct pxp42_create_arb_in msg_in = {0};
> > > + struct pxp42_create_arb_out msg_out = {0};
> > > int ret;
> > >
> > > - msg_in.header.api_version = PXP_TEE_APIVER;
> > > - msg_in.header.command_id = PXP_TEE_ARB_CMDID;
> > > + msg_in.header.header.api_version = PXP42_APIVER;
> > > + msg_in.header.header.command_id = PXP42_CMDID_INIT_SESSION;
> > > msg_in.header.buffer_len = sizeof(msg_in) -
> > > sizeof(msg_in.header);
> > > - msg_in.protection_mode = PXP_TEE_ARB_PROTECTION_MODE;
> > > + msg_in.protection_mode = PXP42_INIT_SESSION_PROTECTION_ARB;
> > > msg_in.session_id = arb_session_id;
> > >
> > > ret = intel_pxp_tee_io_message(pxp,
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee_interface.h
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_tee_interface.h
> > > deleted file mode 100644
> > > index 7edc1760f142..000000000000
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee_interface.h
> > > +++ /dev/null
> > > @@ -1,57 +0,0 @@
> > > -/* SPDX-License-Identifier: MIT */
> > > -/*
> > > - * Copyright(c) 2020-2022, Intel Corporation. All rights reserved.
> > > - */
> > > -
> > > -#ifndef __INTEL_PXP_TEE_INTERFACE_H__
> > > -#define __INTEL_PXP_TEE_INTERFACE_H__
> > > -
> > > -#include <linux/types.h>
> > > -
> > > -#define PXP_TEE_APIVER 0x40002
> > > -#define PXP_TEE_43_APIVER 0x00040003
> > > -#define PXP_TEE_ARB_CMDID 0x1e
> > > -#define PXP_TEE_ARB_PROTECTION_MODE 0x2
> > > -#define PXP_TEE_43_START_HUC_AUTH 0x0000003A
> > > -
> > > -/*
> > > - * there are a lot of status codes for PXP, but we only define the ones
> > > we
> > > - * actually can handle in the driver. other failure codes will be
> > > printed to
> > > - * error msg for debug.
> > > - */
> > > -enum pxp_status {
> > > - PXP_STATUS_SUCCESS = 0x0,
> > > - PXP_STATUS_OP_NOT_PERMITTED = 0x4013
> > > -};
> > > -
> > > -/* PXP TEE message header */
> > > -struct pxp_tee_cmd_header {
> > > - u32 api_version;
> > > - u32 command_id;
> > > - u32 status;
> > > - /* Length of the message (excluding the header) */
> > > - u32 buffer_len;
> > > -} __packed;
> > > -
> > > -/* PXP TEE message input to create a arbitrary session */
> > > -struct pxp_tee_create_arb_in {
> > > - struct pxp_tee_cmd_header header;
> > > - u32 protection_mode;
> > > - u32 session_id;
> > > -} __packed;
> > > -
> > > -/* PXP TEE message output to create a arbitrary session */
> > > -struct pxp_tee_create_arb_out {
> > > - struct pxp_tee_cmd_header header;
> > > -} __packed;
> > > -
> > > -struct pxp_tee_start_huc_auth_in {
> > > - struct pxp_tee_cmd_header header;
> > > - __le64 huc_base_address;
> > > -};
> > > -
> > > -struct pxp_tee_start_huc_auth_out {
> > > - struct pxp_tee_cmd_header header;
> > > -};
> > > -
> > > -#endif /* __INTEL_PXP_TEE_INTERFACE_H__ */
> > >
> > > base-commit: 92b40b6e1d54d68a766c1545b9ace3e2eccad94a
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center
