On Tue, 01 Nov 2022, "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx> 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.alexis@xxxxxxxxx>
>> ---
>> .../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
