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

[Mario Limonciello <mario.limonciello@amd.com>]

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.