Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

Antoine, Peter
2015-04-28T13:29:21Z

On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
> > > > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index
> > > > > 62c40777..367e42f 100644
> > > > > --- a/include/drm/drmP.h
> > > > > +++ b/include/drm/drmP.h
> > > > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
> > > > >
> > > > > /* driver capabilities and requirements mask */
> > > > > -#define DRIVER_USE_AGP 0x1
> > > > > -#define DRIVER_PCI_DMA 0x8
> > > > > -#define DRIVER_SG 0x10
> > > > > -#define DRIVER_HAVE_DMA 0x20
> > > > > -#define DRIVER_HAVE_IRQ 0x40
> > > > > -#define DRIVER_IRQ_SHARED 0x80
> > > > > -#define DRIVER_GEM 0x1000
> > > > > -#define DRIVER_MODESET 0x2000
> > > > > -#define DRIVER_PRIME 0x4000
> > > > > -#define DRIVER_RENDER 0x8000
> > > > > -#define DRIVER_ATOMIC 0x10000
> > > > > +#define DRIVER_USE_AGP 0x1
> > > > > +#define DRIVER_PCI_DMA 0x8
> > > > > +#define DRIVER_SG 0x10
> > > > > +#define DRIVER_HAVE_DMA 0x20
> > > > > +#define DRIVER_HAVE_IRQ 0x40
> > > > > +#define DRIVER_IRQ_SHARED 0x80
> > > > > +#define DRIVER_GEM 0x1000
> > > > > +#define DRIVER_MODESET 0x2000
> > > > > +#define DRIVER_PRIME 0x4000
> > > > > +#define DRIVER_RENDER 0x8000
> > > > > +#define DRIVER_ATOMIC 0x10000
> > > > > +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
> > > >
> > > > Why is there KMS in the name?
> > > >
> > > > By suggestion of Daniel.
> > > >
> > > > I was thinking just checking for GEM, but I think there was some
> > > > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > > > now dead as far as i915 is concerned, so in theory it should be fine.
> > > > But I'm not sure if some other driver might have the same baggage.
> > > >
> > > > Other drivers have the same baggage.
> > > >
> > > > I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> > > >
> > > > Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
> > >
> > > Reference?
> >
> > From the next commit [5/5] as it is the one that actually turns off the
> > functions that were turned off before.
> >
> > These changes are based on the two patches:
> > commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> > Author: Dave Airlie <airlied@redhat.com>
> >
> > And the commit that the above patch reverts:
> > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Looking at ancient libdrm sources makes me think nouveau just used to
> create and destroy the context, but not actually use it for anything.
> So nopping out the ioctls should be good enough AFAICS. Or am I missing
> something?
>

An old version of libdrm that still requires support needs them, it's
the reason that David Airlie reverted the patch that Daniel did to
remove the functions. Do they still need support, I don't know? David
Airlie is on the cc list.

A discussion was had and this was the way that it was suggested it be
done. This seems a good half-way house, the actual security holes that
have been found have been fixed and the functions (that seem to have a
lot more security issues in them) are turned off for the drivers that
don't use them, and if a driver does require them, it will be a one line
change to reintroduce them. Are we carrying code we don't need to
support, probably.

Peter.