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

[Lizhi Hou <lizhi.hou@amd.com>]

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.
>