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

[Souza, Jose]

On Tue, 2023-07-04 at 11:23 +0300, Oded Gabbay wrote:
> 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 Xe start to expect canonical addresses but a public released UMD sets regular addresses, that will break the uAPI.
We are allowed to change the behavior starting from platform X that are still under "force probe" but better have it for all platforms from start.

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

But if we don't check the issue could go silent as current platforms ignore the bits above 47/57.

> Are you explicitly checking all addresses that are given by the user ?

The starting batch buffer address, yes.

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