Re: [Intel-xe] [PATCH v2 3/4] fixup! drm/xe: Introduce a new DRM driver for Intel GPUs

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Tue, 2023-08-15 at 11:40 -0700, Umesh Nerlige Ramappa wrote:
> Hi Matt, Rodrigo,
> 
> I have a uAPI change here that breaks UMDs. How do I land this and
> what 
> UMDs should I inform for updating xe_drm.h?

We need to ensure that we are in sync with all the UMD contacts
(as listed in our internal gfx-linux page)

Cc: Jose Souza <jose.souza@intel.com>
Cc: Filip Hazubski <filip.hazubski@intel.com>
Cc: Carl Zhang <carl.zhang@intel.com>
Cc: Effie Yu <effie.yu@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@intel.com>

Also, ensure that all the components have their implementation
ready to be able to push all simultaneously in the least disruptive
way.

Needless to say, but maybe worth, that we are only doing this
because we are out of tree yet. We won't be able to break it
again soon when we are in tree.

> 
> Thanks,
> Umesh
> 
> On Tue, Aug 15, 2023 at 07:03:42AM -0700, Souza, Jose wrote:
> > On Mon, 2023-08-14 at 15:37 -0700, Umesh Nerlige Ramappa wrote:
> > > struct drm_xe_engine_class_instance might get padded for 64-bit
> > > alignment based on compiler used. Since engine information is
> > > kmalloced
> > > in the query and drm_xe_engine_class_instance may be padded, it
> > > could
> > > potentially leak some kernel memory to user.
> > > 
> > > Add a rsvd field to struct drm_xe_engine_class_instance to make
> > > it
> > > 64-bit aligned and zero out the field before returning to user.
> > > 
> > > Signed-off-by: Umesh Nerlige Ramappa
> > > <umesh.nerlige.ramappa@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 7 +++++--
> > >  include/uapi/drm/xe_drm.h     | 1 +
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c
> > > b/drivers/gpu/drm/xe/xe_query.c
> > > index 99a4800c7c53..b9d565264ceb 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -65,7 +65,7 @@ static int query_engines(struct xe_device *xe,
> > >                 return -EINVAL;
> > >         }
> > > 
> > > -       hw_engine_info = kmalloc(size, GFP_KERNEL);
> > > +       hw_engine_info = kzalloc(size, GFP_KERNEL);
> > >         if (!hw_engine_info)
> > >                 return -ENOMEM;
> > > 
> > > @@ -78,7 +78,10 @@ static int query_engines(struct xe_device *xe,
> > >                                 xe_to_user_engine_class[hwe-
> > > >class];
> > >                         hw_engine_info[i].engine_instance =
> > >                                 hwe->logical_instance;
> > > -                       hw_engine_info[i++].gt_id = gt->info.id;
> > > +                       hw_engine_info[i].gt_id = gt->info.id;
> > > +                       hw_engine_info[i].rsvd = 0;
> > > +
> > > +                       i++;
> > >                 }
> > > 
> > >         if (copy_to_user(query_ptr, hw_engine_info, size)) {
> > > diff --git a/include/uapi/drm/xe_drm.h
> > > b/include/uapi/drm/xe_drm.h
> > > index 86f16d50e9cc..53cd57342620 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -753,6 +753,7 @@ struct drm_xe_engine_class_instance {
> > > 
> > >         __u16 engine_instance;
> > >         __u16 gt_id;
> > > +       __u16 rsvd;
> > 
> > I don't think this change should be fixup, as this breaks the uAPI.
> > Please ping the Rodrigo, Matt Brost... about what we should do
> > here.
> > But at least you need to sync with other UMD contacts about landing
> > this change at the same time UMDs update their xe_drm.h.
> > 
> > >  };
> > > 
> > >  struct drm_xe_exec_queue_create {
> >