[PATCH V1 1/2] accel/amdxdna: Add zero check for pad in ioctl input structures

[PATCH V1 1/2] accel/amdxdna: Add zero check for pad in ioctl input structures

[Lizhi Hou]

For input ioctl structures, it is better to check if the pad is zero.
Thus, the pad bytes might be usable in the future.

Suggested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_ctx.c     |  3 +++
 drivers/accel/amdxdna/aie2_message.c |  3 +++
 drivers/accel/amdxdna/amdxdna_ctx.c  |  6 ++++++
 drivers/accel/amdxdna/amdxdna_gem.c  |  2 +-
 include/uapi/drm/amdxdna_accel.h     | 10 +++++-----
 5 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index cdeead75c6f5..9facf45818f9 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -690,6 +690,9 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
 	int ret;
 
 	XDNA_DBG(xdna, "Config %d CU to %s", config->num_cus, hwctx->name);
+	if (XDNA_MBZ_DBG(xdna, config->pad, sizeof(config->pad)))
+		return -EINVAL;
+
 	if (hwctx->status != HWCTX_STAT_INIT) {
 		XDNA_ERR(xdna, "Not support re-config CU");
 		return -EINVAL;
diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
index b2ca78cfd0a7..9e2c9a44f76a 100644
--- a/drivers/accel/amdxdna/aie2_message.c
+++ b/drivers/accel/amdxdna/aie2_message.c
@@ -395,6 +395,9 @@ int aie2_config_cu(struct amdxdna_hwctx *hwctx)
 	for (i = 0; i < hwctx->cus->num_cus; i++) {
 		struct amdxdna_cu_config *cu = &hwctx->cus->cu_configs[i];
 
+		if (XDNA_MBZ_DBG(xdna, cu->pad, sizeof(cu->pad)))
+			return -EINVAL;
+
 		gobj = drm_gem_object_lookup(hwctx->client->filp, cu->cu_bo);
 		if (!gobj) {
 			XDNA_ERR(xdna, "Lookup GEM object failed");
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 324f35c43f6c..d11b1c83d9c3 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -243,6 +243,9 @@ int amdxdna_drm_destroy_hwctx_ioctl(struct drm_device *dev, void *data, struct d
 	struct amdxdna_hwctx *hwctx;
 	int ret = 0, idx;
 
+	if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad)))
+		return -EINVAL;
+
 	if (!drm_dev_enter(dev, &idx))
 		return -ENODEV;
 
@@ -277,6 +280,9 @@ int amdxdna_drm_config_hwctx_ioctl(struct drm_device *dev, void *data, struct dr
 	void *buf;
 	u64 val;
 
+	if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad)))
+		return -EINVAL;
+
 	if (!xdna->dev_info->ops->hwctx_config)
 		return -EOPNOTSUPP;
 
diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 4dfeca306d98..606433d73236 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -552,7 +552,7 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm
 	struct drm_gem_object *gobj;
 	int ret = 0;
 
-	if (args->ext || args->ext_flags)
+	if (args->ext || args->ext_flags || args->pad)
 		return -EINVAL;
 
 	gobj = drm_gem_object_lookup(filp, args->handle);
diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h
index e4edb52bc27b..92eff83fac1f 100644
--- a/include/uapi/drm/amdxdna_accel.h
+++ b/include/uapi/drm/amdxdna_accel.h
@@ -87,7 +87,7 @@ struct amdxdna_drm_create_hwctx {
 /**
  * struct amdxdna_drm_destroy_hwctx - Destroy hardware context.
  * @handle: Hardware context handle.
- * @pad: Structure padding.
+ * @pad: MBZ.
  */
 struct amdxdna_drm_destroy_hwctx {
 	__u32 handle;
@@ -98,7 +98,7 @@ struct amdxdna_drm_destroy_hwctx {
  * struct amdxdna_cu_config - configuration for one CU
  * @cu_bo: CU configuration buffer bo handle.
  * @cu_func: Function of a CU.
- * @pad: Structure padding.
+ * @pad: MBZ.
  */
 struct amdxdna_cu_config {
 	__u32 cu_bo;
@@ -109,7 +109,7 @@ struct amdxdna_cu_config {
 /**
  * struct amdxdna_hwctx_param_config_cu - configuration for CUs in hardware context
  * @num_cus: Number of CUs to configure.
- * @pad: Structure padding.
+ * @pad: MBZ.
  * @cu_configs: Array of CU configurations of struct amdxdna_cu_config.
  */
 struct amdxdna_hwctx_param_config_cu {
@@ -133,7 +133,7 @@ enum amdxdna_drm_config_hwctx_param {
  * @param_val: A structure specified by the param_type struct member.
  * @param_val_size: Size of the parameter buffer pointed to by the param_val.
  *		    If param_val is not a pointer, driver can ignore this.
- * @pad: Structure padding.
+ * @pad: MBZ.
  *
  * Note: if the param_val is a pointer pointing to a buffer, the maximum size
  * of the buffer is 4KiB(PAGE_SIZE).
@@ -175,7 +175,7 @@ struct amdxdna_drm_create_bo {
  * @ext: MBZ.
  * @ext_flags: MBZ.
  * @handle: DRM buffer object handle.
- * @pad: Structure padding.
+ * @pad: MBZ.
  * @map_offset: Returned DRM fake offset for mmap().
  * @vaddr: Returned user VA of buffer. 0 in case user needs mmap().
  * @xdna_addr: Returned XDNA device virtual address.
-- 
2.34.1

[PATCH V1 2/2] accel/amdxdna: Remove DRM_AMDXDNA_HWCTX_CONFIG_NUM

[Lizhi Hou]

Defining a number of enum elements in uapi header is meaningless. It will
not be used as expected and can potentially lead to incompatible issue
between user space application and driver.

Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 include/uapi/drm/amdxdna_accel.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h
index 92eff83fac1f..a706ead39082 100644
--- a/include/uapi/drm/amdxdna_accel.h
+++ b/include/uapi/drm/amdxdna_accel.h
@@ -122,7 +122,6 @@ enum amdxdna_drm_config_hwctx_param {
 	DRM_AMDXDNA_HWCTX_CONFIG_CU,
 	DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF,
 	DRM_AMDXDNA_HWCTX_REMOVE_DBG_BUF,
-	DRM_AMDXDNA_HWCTX_CONFIG_NUM
 };
 
 /**
-- 
2.34.1

Re: [PATCH V1 2/2] accel/amdxdna: Remove DRM_AMDXDNA_HWCTX_CONFIG_NUM

[Mario Limonciello]

On 12/17/2024 10:54, Lizhi Hou wrote:
> Defining a number of enum elements in uapi header is meaningless. It will
> not be used as expected and can potentially lead to incompatible issue
> between user space application and driver.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>

It's a great point.  Making this change now before the uAPI is stable 
will allow you to add new changes later.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   include/uapi/drm/amdxdna_accel.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h
> index 92eff83fac1f..a706ead39082 100644
> --- a/include/uapi/drm/amdxdna_accel.h
> +++ b/include/uapi/drm/amdxdna_accel.h
> @@ -122,7 +122,6 @@ enum amdxdna_drm_config_hwctx_param {
>   	DRM_AMDXDNA_HWCTX_CONFIG_CU,
>   	DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF,
>   	DRM_AMDXDNA_HWCTX_REMOVE_DBG_BUF,
> -	DRM_AMDXDNA_HWCTX_CONFIG_NUM
>   };
>   
>   /**

Re: [PATCH V1 1/2] accel/amdxdna: Add zero check for pad in ioctl input structures

[Mario Limonciello]

On 12/17/2024 10:54, Lizhi Hou wrote:
> For input ioctl structures, it is better to check if the pad is zero.
> Thus, the pad bytes might be usable in the future.
> 
> Suggested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/accel/amdxdna/aie2_ctx.c     |  3 +++
>   drivers/accel/amdxdna/aie2_message.c |  3 +++
>   drivers/accel/amdxdna/amdxdna_ctx.c  |  6 ++++++
>   drivers/accel/amdxdna/amdxdna_gem.c  |  2 +-
>   include/uapi/drm/amdxdna_accel.h     | 10 +++++-----
>   5 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index cdeead75c6f5..9facf45818f9 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -690,6 +690,9 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
>   	int ret;
>   
>   	XDNA_DBG(xdna, "Config %d CU to %s", config->num_cus, hwctx->name);
> +	if (XDNA_MBZ_DBG(xdna, config->pad, sizeof(config->pad)))
> +		return -EINVAL;
> +
>   	if (hwctx->status != HWCTX_STAT_INIT) {
>   		XDNA_ERR(xdna, "Not support re-config CU");
>   		return -EINVAL;
> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
> index b2ca78cfd0a7..9e2c9a44f76a 100644
> --- a/drivers/accel/amdxdna/aie2_message.c
> +++ b/drivers/accel/amdxdna/aie2_message.c
> @@ -395,6 +395,9 @@ int aie2_config_cu(struct amdxdna_hwctx *hwctx)
>   	for (i = 0; i < hwctx->cus->num_cus; i++) {
>   		struct amdxdna_cu_config *cu = &hwctx->cus->cu_configs[i];
>   
> +		if (XDNA_MBZ_DBG(xdna, cu->pad, sizeof(cu->pad)))
> +			return -EINVAL;
> +
>   		gobj = drm_gem_object_lookup(hwctx->client->filp, cu->cu_bo);
>   		if (!gobj) {
>   			XDNA_ERR(xdna, "Lookup GEM object failed");
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 324f35c43f6c..d11b1c83d9c3 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -243,6 +243,9 @@ int amdxdna_drm_destroy_hwctx_ioctl(struct drm_device *dev, void *data, struct d
>   	struct amdxdna_hwctx *hwctx;
>   	int ret = 0, idx;
>   
> +	if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad)))
> +		return -EINVAL;
> +
>   	if (!drm_dev_enter(dev, &idx))
>   		return -ENODEV;
>   
> @@ -277,6 +280,9 @@ int amdxdna_drm_config_hwctx_ioctl(struct drm_device *dev, void *data, struct dr
>   	void *buf;
>   	u64 val;
>   
> +	if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad)))
> +		return -EINVAL;
> +
>   	if (!xdna->dev_info->ops->hwctx_config)
>   		return -EOPNOTSUPP;
>   
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 4dfeca306d98..606433d73236 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -552,7 +552,7 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm
>   	struct drm_gem_object *gobj;
>   	int ret = 0;
>   
> -	if (args->ext || args->ext_flags)
> +	if (args->ext || args->ext_flags || args->pad)
>   		return -EINVAL;
>   
>   	gobj = drm_gem_object_lookup(filp, args->handle);
> diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h
> index e4edb52bc27b..92eff83fac1f 100644
> --- a/include/uapi/drm/amdxdna_accel.h
> +++ b/include/uapi/drm/amdxdna_accel.h
> @@ -87,7 +87,7 @@ struct amdxdna_drm_create_hwctx {
>   /**
>    * struct amdxdna_drm_destroy_hwctx - Destroy hardware context.
>    * @handle: Hardware context handle.
> - * @pad: Structure padding.
> + * @pad: MBZ.
>    */
>   struct amdxdna_drm_destroy_hwctx {
>   	__u32 handle;
> @@ -98,7 +98,7 @@ struct amdxdna_drm_destroy_hwctx {
>    * struct amdxdna_cu_config - configuration for one CU
>    * @cu_bo: CU configuration buffer bo handle.
>    * @cu_func: Function of a CU.
> - * @pad: Structure padding.
> + * @pad: MBZ.
>    */
>   struct amdxdna_cu_config {
>   	__u32 cu_bo;
> @@ -109,7 +109,7 @@ struct amdxdna_cu_config {
>   /**
>    * struct amdxdna_hwctx_param_config_cu - configuration for CUs in hardware context
>    * @num_cus: Number of CUs to configure.
> - * @pad: Structure padding.
> + * @pad: MBZ.
>    * @cu_configs: Array of CU configurations of struct amdxdna_cu_config.
>    */
>   struct amdxdna_hwctx_param_config_cu {
> @@ -133,7 +133,7 @@ enum amdxdna_drm_config_hwctx_param {
>    * @param_val: A structure specified by the param_type struct member.
>    * @param_val_size: Size of the parameter buffer pointed to by the param_val.
>    *		    If param_val is not a pointer, driver can ignore this.
> - * @pad: Structure padding.
> + * @pad: MBZ.
>    *
>    * Note: if the param_val is a pointer pointing to a buffer, the maximum size
>    * of the buffer is 4KiB(PAGE_SIZE).
> @@ -175,7 +175,7 @@ struct amdxdna_drm_create_bo {
>    * @ext: MBZ.
>    * @ext_flags: MBZ.
>    * @handle: DRM buffer object handle.
> - * @pad: Structure padding.
> + * @pad: MBZ.
>    * @map_offset: Returned DRM fake offset for mmap().
>    * @vaddr: Returned user VA of buffer. 0 in case user needs mmap().
>    * @xdna_addr: Returned XDNA device virtual address.

Re: [PATCH V1 2/2] accel/amdxdna: Remove DRM_AMDXDNA_HWCTX_CONFIG_NUM

[Mario Limonciello]

On 12/17/2024 10:54, Lizhi Hou wrote:
> Defining a number of enum elements in uapi header is meaningless. It will
> not be used as expected and can potentially lead to incompatible issue
> between user space application and driver.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
>   include/uapi/drm/amdxdna_accel.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h
> index 92eff83fac1f..a706ead39082 100644
> --- a/include/uapi/drm/amdxdna_accel.h
> +++ b/include/uapi/drm/amdxdna_accel.h
> @@ -122,7 +122,6 @@ enum amdxdna_drm_config_hwctx_param {
>   	DRM_AMDXDNA_HWCTX_CONFIG_CU,
>   	DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF,
>   	DRM_AMDXDNA_HWCTX_REMOVE_DBG_BUF,
> -	DRM_AMDXDNA_HWCTX_CONFIG_NUM
>   };
>   
>   /**

I've applied both patches to drm-misc-next.  Thanks!

6a8d72b80807a accel/amdxdna: Remove DRM_AMDXDNA_HWCTX_CONFIG_NUM 
  

03c318a0af96f accel/amdxdna: Add zero check for pad in ioctl input 
structures