1. Patchew
  2. linux
  3. [PATCH V3 0/5] AMD NPU driver improvements
  4. View patch

[PATCH V3 5/5] accel/amdxdna: Add zero check for pad in ioctl input structures

Lizhi Hou posted 5 patches 1 year ago
[PATCH V3 5/5] accel/amdxdna: Add zero check for pad in ioctl input structures
Posted by Lizhi Hou 1 year ago 
For input structures, it is better to check if the pad is zero.
Thus, the pad bytes might be usable in the future.

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     | 11 +++++------
 5 files changed, 18 insertions(+), 7 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..a706ead39082 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 {
@@ -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
 };
 
 /**
@@ -133,7 +132,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 +174,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
Re: [PATCH V3 5/5] accel/amdxdna: Add zero check for pad in ioctl input structures
Posted by Mario Limonciello 1 year ago 
On 12/13/2024 17:29, Lizhi Hou wrote:
> For input structures, it is better to check if the pad is zero.
> Thus, the pad bytes might be usable in the future.
> 

IIRC you should pick up:
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     | 11 +++++------
>   5 files changed, 18 insertions(+), 7 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..a706ead39082 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 {
> @@ -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

This change actually meant here?  It seems like it should be it's own 
change.

>   };
>   
>   /**
> @@ -133,7 +132,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 +174,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 V3 5/5] accel/amdxdna: Add zero check for pad in ioctl input structures
Posted by Lizhi Hou 1 year ago 
On 12/16/24 12:38, Mario Limonciello wrote:
> On 12/13/2024 17:29, Lizhi Hou wrote:
>> For input structures, it is better to check if the pad is zero.
>> Thus, the pad bytes might be usable in the future.
>>
>
> IIRC you should pick up:
> Suggested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Sure. Will add it.
>
>> 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     | 11 +++++------
>>   5 files changed, 18 insertions(+), 7 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..a706ead39082 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 {
>> @@ -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
>
> This change actually meant here?  It seems like it should be it's own 
> change.

Agree. Hopefully, the patch 1 - 4 are good to go and I do not need to 
resend them. :)


Thanks,

Lizhi

>
>>   };
>>     /**
>> @@ -133,7 +132,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 +174,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 V3 5/5] accel/amdxdna: Add zero check for pad in ioctl input structures
Posted by Mario Limonciello 1 year ago 
>> This change actually meant here?  It seems like it should be it's own 
>> change.
> 
> Agree. Hopefully, the patch 1 - 4 are good to go and I do not need to 
> resend them. :)
> 
> 

Yeah those are fine at this point.  They've gotten good feedback, no 
complaints from LKP robot.  I think you can just send the fixed up patch 
5 separately.

I've just added 1-4 to drm-misc-next.  Here are the hashes.

b1dcfe620574b accel/amdxdna: Read firmware interface version from registers
f4d7b8a6bc8c9 accel/amdxdna: Enhance power management settings
a37d78470bcc8 accel/amdxdna: Replace idr api with xarray
273b5176ac178 accel/amdxdna: Add RyzenAI-npu6 support

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.