Re: [PATCH v2 08/12] drm/xe/pxp: add a query for PXP status

[John Harrison <john.c.harrison@intel.com>]

On 8/16/2024 12:00, Daniele Ceraolo Spurio wrote:
> PXP prerequisites (SW proxy and HuC auth via GSC) are completed
> asynchronously from driver load, which means that userspace can start
> submitting before we're ready to start a PXP session. Therefore, we need
> a query that userspace can use to check not only if PXP is supported by
by -> but?

> also to wait until the prerequisites are done.
>
> v2: Improve doc, do not report TYPE_NONE as supported (José)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_pxp.c   | 33 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_pxp.h   |  1 +
>   drivers/gpu/drm/xe/xe_query.c | 32 ++++++++++++++++++++++++++++++++
>   include/uapi/drm/xe_drm.h     | 35 +++++++++++++++++++++++++++++++++++
>   4 files changed, 101 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
> index acdc25c8e8a1..ca4302af4ced 100644
> --- a/drivers/gpu/drm/xe/xe_pxp.c
> +++ b/drivers/gpu/drm/xe/xe_pxp.c
> @@ -60,6 +60,39 @@ static bool pxp_prerequisites_done(const struct xe_pxp *pxp)
>   	return ready;
>   }
>   
> +/**
> + * xe_pxp_get_readiness_status - check whether PXP is ready for userspace use
> + * @pxp: the xe_pxp pointer (can be NULL if PXP is disabled)
> + *
> + * This function is used for status query from userspace, so the returned value
value -> values

> + * follow the uapi (see drm_xe_query_pxp_status)
> + *
> + * Returns: 0 if PXP is not ready yet, 1 if it is ready, an errno value if PXP
> + * is not supported/enabled or if something went wrong in the initialization of
> + * the prerequisites.
You have two independent statements regarding the return code. Would be 
better to just have the "Returns: ..." paragraph but include a statement 
that these values are as defined in the UAPI.

> + */
> +int xe_pxp_get_readiness_status(struct xe_pxp *pxp)
> +{
> +	int ret = 0;
> +
> +	if (!xe_pxp_is_enabled(pxp))
> +		return -ENODEV;
> +
> +	/* if the GSC or HuC FW are in an error state, PXP will never work */
> +	if (xe_uc_fw_status_to_error(pxp->gt->uc.huc.fw.status) ||
> +	    xe_uc_fw_status_to_error(pxp->gt->uc.gsc.fw.status))
> +		return -EIO;
> +
> +	xe_pm_runtime_get(pxp->xe);
> +
> +	/* PXP requires both HuC loaded and GSC proxy initialized */
> +	if (pxp_prerequisites_done(pxp))
> +		ret = 1;
> +
> +	xe_pm_runtime_put(pxp->xe);
> +	return ret;
> +}
> +
>   static bool pxp_session_is_in_play(struct xe_pxp *pxp, u32 id)
>   {
>   	struct xe_gt *gt = pxp->gt;
> diff --git a/drivers/gpu/drm/xe/xe_pxp.h b/drivers/gpu/drm/xe/xe_pxp.h
> index 2e0ab186072a..868813cc84b9 100644
> --- a/drivers/gpu/drm/xe/xe_pxp.h
> +++ b/drivers/gpu/drm/xe/xe_pxp.h
> @@ -14,6 +14,7 @@ struct xe_pxp;
>   
>   bool xe_pxp_is_supported(const struct xe_device *xe);
>   bool xe_pxp_is_enabled(const struct xe_pxp *pxp);
> +int xe_pxp_get_readiness_status(struct xe_pxp *pxp);
>   
>   int xe_pxp_init(struct xe_device *xe);
>   void xe_pxp_irq_handler(struct xe_device *xe, u16 iir);
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index 73ef6e4c2dc9..a1e297234972 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c
> @@ -22,6 +22,7 @@
>   #include "xe_guc_hwconfig.h"
>   #include "xe_macros.h"
>   #include "xe_mmio.h"
> +#include "xe_pxp.h"
>   #include "xe_ttm_vram_mgr.h"
>   
>   static const u16 xe_to_user_engine_class[] = {
> @@ -680,6 +681,36 @@ static int query_oa_units(struct xe_device *xe,
>   	return ret ? -EFAULT : 0;
>   }
>   
> +static int query_pxp_status(struct xe_device *xe, struct drm_xe_device_query *query)
> +{
> +	struct drm_xe_query_pxp_status __user *query_ptr = u64_to_user_ptr(query->data);
> +	size_t size = sizeof(struct drm_xe_query_pxp_status);
> +	struct drm_xe_query_pxp_status resp;
> +	int ret;
> +
> +	if (query->size == 0) {
> +		query->size = size;
> +		return 0;
> +	} else if (XE_IOCTL_DBG(xe, query->size != size)) {
Do we not allow structures to grow in future versions? In a backwards 
compatible way, that is.

> +		return -EINVAL;
> +	}
> +
> +	if (copy_from_user(&resp, query_ptr, size))
> +		return -EFAULT;
Why copy in the data from the user side only to overwrite everything in 
the structure?

> +
> +	ret = xe_pxp_get_readiness_status(xe->pxp);
> +	if (ret < 0)
> +		return ret;
> +
> +	resp.status = ret;
> +	resp.supported_session_types = BIT(DRM_XE_PXP_TYPE_HWDRM);
> +
> +	if (copy_to_user(query_ptr, &resp, size))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
>   static int (* const xe_query_funcs[])(struct xe_device *xe,
>   				      struct drm_xe_device_query *query) = {
>   	query_engines,
> @@ -691,6 +722,7 @@ static int (* const xe_query_funcs[])(struct xe_device *xe,
>   	query_engine_cycles,
>   	query_uc_fw_version,
>   	query_oa_units,
> +	query_pxp_status,
>   };
>   
>   int xe_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 5f4d08123672..9972ceb3fbfb 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -627,6 +627,39 @@ struct drm_xe_query_uc_fw_version {
>   	__u64 reserved;
>   };
>   
> +/**
> + * struct drm_xe_query_pxp_status - query if PXP is ready
> + *
> + * If PXP is enabled and no fatal error as occurred, the status will be set to
as -> has

> + * one of the following values:
> + * 0: PXP init still in progress
> + * 1: PXP init complete
> + *
> + * If PXP is not enabled or something has gone wrong, the query will be failed
> + * with one of the following error codes:
> + * -ENODEV: PXP not supported or disabled;
> + * -EIO: fatal error occurred during init, so PXP will never be enabled;
> + * -EINVAL: incorrect value provided as part of the query;
> + * -EFAULT: error copying the memory between kernel and userspace.
Currently, could also be copying from user to kernel. Although that copy 
seems unnecessary.

John.

> + *
> + * The status can only be 0 in the first few seconds after driver load. If
> + * everything works as expected, the status will transition to init complete in
> + * less than 1 second, while in case of errors the driver might take longer to
> + * start returning an error code, but it should still take less than 10 seconds.
> + *
> + * The supported session type bitmask is based on the values in
> + * enum drm_xe_pxp_session_type. TYPE_NONE is always supported and therefore
> + * is not reported in the bitmask.
> + *
> + */
> +struct drm_xe_query_pxp_status {
> +	/** @status: current PXP status */
> +	__u32 status;
> +
> +	/** @supported_session_types: bitmask of supported PXP session types */
> +	__u32 supported_session_types;
> +};
> +
>   /**
>    * struct drm_xe_device_query - Input of &DRM_IOCTL_XE_DEVICE_QUERY - main
>    * structure to query device information
> @@ -646,6 +679,7 @@ struct drm_xe_query_uc_fw_version {
>    *    attributes.
>    *  - %DRM_XE_DEVICE_QUERY_GT_TOPOLOGY
>    *  - %DRM_XE_DEVICE_QUERY_ENGINE_CYCLES
> + *  - %DRM_XE_DEVICE_QUERY_PXP_STATUS
>    *
>    * If size is set to 0, the driver fills it with the required size for
>    * the requested type of data to query. If size is equal to the required
> @@ -698,6 +732,7 @@ struct drm_xe_device_query {
>   #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
>   #define DRM_XE_DEVICE_QUERY_UC_FW_VERSION	7
>   #define DRM_XE_DEVICE_QUERY_OA_UNITS		8
> +#define DRM_XE_DEVICE_QUERY_PXP_STATUS		9
>   	/** @query: The type of data to query */
>   	__u32 query;
>