[Intel-xe] [PATCH] drm/xe: Make explicit that exec uAPI expects canonical addresses

[Oded Gabbay]

On Mon, Jul 3, 2023 at 10:01 PM José Roberto de Souza
<jose.souza at intel.com> wrote:
>
> The batch buffer address in exec uAPI is used when emitting
> MI_BATCH_BUFFER_START that expect canonical addresses in future
> platforms, for current ones the bits above 57 for PVC and 47 for
> other platforms are ignored.
>
> So the safest approach is to require canonical address for all
> platforms supported by Xe to avoid uAPI breaks.
What do you mean by uAPI break ?
If the user gives a wrong address, won't the device simply issue a
page fault and/or a different error event that will cause the job to
terminate ?
Are you explicitly checking all addresses that are given by the user ?
I don't understand why this check warrants a dedicated check in the driver.
imo, you should never validate user input if it will just cause the
gpu to issue a page fault and/or an error interrupt

Oded

>
> BSpec: 60223 59475 45718
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c | 6 ++++++
>  drivers/gpu/drm/xe/xe_device.h | 2 ++
>  drivers/gpu/drm/xe/xe_exec.c   | 8 ++++++++
>  include/uapi/drm/xe_drm.h      | 4 ++--
>  4 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 07ae208af809d..887aed80a20e9 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -437,3 +437,9 @@ void xe_device_mem_access_put(struct xe_device *xe)
>
>         XE_WARN_ON(ref < 0);
>  }
> +
> +u64 xe_device_canonical_addr(struct xe_device *xe, u64 address)
> +{
> +       const int high_address_bit = xe->info.dma_mask_size > 47 ? 57 : 47;
> +       return sign_extend64(address, high_address_bit);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> index 779f71d066e6e..4df4f52f946ea 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -140,6 +140,8 @@ static inline struct xe_force_wake * gt_to_fw(struct xe_gt *gt)
>  void xe_device_mem_access_get(struct xe_device *xe);
>  void xe_device_mem_access_put(struct xe_device *xe);
>
> +u64 xe_device_canonical_addr(struct xe_device *xe, u64 address);
> +
>  static inline bool xe_device_mem_access_ongoing(struct xe_device *xe)
>  {
>         return atomic_read(&xe->mem_access.ref);
> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> index c52edff9a3584..ba6e82112e4be 100644
> --- a/drivers/gpu/drm/xe/xe_exec.c
> +++ b/drivers/gpu/drm/xe/xe_exec.c
> @@ -231,6 +231,14 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>                 }
>         }
>
> +       for (i = 0; i < engine->width; i++) {
> +               const u64 canonical_addr = xe_device_canonical_addr(xe, addresses[i]);
> +               if (XE_IOCTL_ERR(xe, addresses[i] != canonical_addr)) {
> +                       err = -EINVAL;
> +                       goto err_syncs;
> +               }
> +       }
> +
>         /*
>          * We can't install a job into the VM dma-resv shared slot before an
>          * async VM bind passed in as a fence without the risk of deadlocking as
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index e890b131af918..72dc2161fd232 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -811,8 +811,8 @@ struct drm_xe_exec {
>         __u64 syncs;
>
>         /**
> -        * @address: address of batch buffer if num_batch_buffer == 1 or an
> -        * array of batch buffer addresses
> +        * @address: canonical address of batch buffer if num_batch_buffer == 1
> +        * or an array of batch buffer canonical addresses
>          */
>         __u64 address;
>
> --
> 2.41.0
>