| Message ID | 20190301140404.26690-14-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/38] drm/i915/execlists: Suppress redundant preemption | expand |
On 01/03/2019 14:03, Chris Wilson wrote: > An idea for extending uABI inspired by Vulkan's extension chains. > Instead of expanding the data struct for each ioctl every time we need > to add a new feature, define an extension chain instead. As we add > optional interfaces to control the ioctl, we define a new extension > struct that can be linked into the ioctl data only when required by the > user. The key advantage being able to ignore large control structs for > optional interfaces/extensions, while being able to process them in a > consistent manner. > > In comparison to other extensible ioctls, the key difference is the > use of a linked chain of extension structs vs an array of tagged > pointers. For example, > > struct drm_amdgpu_cs_chunk { > __u32 chunk_id; > __u32 length_dw; > __u64 chunk_data; > }; > > struct drm_amdgpu_cs_in { > __u32 ctx_id; > __u32 bo_list_handle; > __u32 num_chunks; > __u32 _pad; > __u64 chunks; > }; > > allows userspace to pass in array of pointers to extension structs, but > must therefore keep constructing that array along side the command stream. > In dynamic situations like that, a linked list is preferred and does not > similar from extra cache line misses as the extension structs themselves > must still be loaded separate to the chunks array. > > v2: Apply the tail call optimisation directly to nip the worry of stack > overflow in the bud. > v3: Defend against recursion. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/i915_user_extensions.c | 43 +++++++++++++++++++++ > drivers/gpu/drm/i915/i915_user_extensions.h | 20 ++++++++++ > drivers/gpu/drm/i915/i915_utils.h | 7 ++++ > include/uapi/drm/i915_drm.h | 20 ++++++++++ > 5 files changed, 91 insertions(+) > create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.c > create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index a1d834068765..89105b1aaf12 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -46,6 +46,7 @@ i915-y := i915_drv.o \ > i915_sw_fence.o \ > i915_syncmap.o \ > i915_sysfs.o \ > + i915_user_extensions.o \ > intel_csr.o \ > intel_device_info.o \ > intel_pm.o \ > diff --git a/drivers/gpu/drm/i915/i915_user_extensions.c b/drivers/gpu/drm/i915/i915_user_extensions.c > new file mode 100644 > index 000000000000..879b4094b2d7 > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_user_extensions.c > @@ -0,0 +1,43 @@ > +/* > + * SPDX-License-Identifier: MIT > + * > + * Copyright © 2018 Intel Corporation > + */ > + > +#include <linux/sched/signal.h> > +#include <linux/uaccess.h> > +#include <uapi/drm/i915_drm.h> > + > +#include "i915_user_extensions.h" > + > +int i915_user_extensions(struct i915_user_extension __user *ext, > + const i915_user_extension_fn *tbl, > + unsigned long count, > + void *data) > +{ > + unsigned int stackdepth = 512; > + > + while (ext) { > + int err; > + u64 x; > + > + if (!stackdepth--) /* recursion vs useful flexibility */ > + return -EINVAL; I don't get this. What stack? Did you mean "static unsigned int stackdepth" in case someone puts i915_user_extension into the extension table? Or just a limit on number of chained extensions? But you are not processing the recursively here. Regards, Tvrtko > + > + if (get_user(x, &ext->name)) > + return -EFAULT; > + > + err = -EINVAL; > + if (x < count && tbl[x]) > + err = tbl[x](ext, data); > + if (err) > + return err; > + > + if (get_user(x, &ext->next_extension)) > + return -EFAULT; > + > + ext = u64_to_user_ptr(x); > + } > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/i915_user_extensions.h b/drivers/gpu/drm/i915/i915_user_extensions.h > new file mode 100644 > index 000000000000..313a510b068a > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_user_extensions.h > @@ -0,0 +1,20 @@ > +/* > + * SPDX-License-Identifier: MIT > + * > + * Copyright © 2018 Intel Corporation > + */ > + > +#ifndef I915_USER_EXTENSIONS_H > +#define I915_USER_EXTENSIONS_H > + > +struct i915_user_extension; > + > +typedef int (*i915_user_extension_fn)(struct i915_user_extension __user *ext, > + void *data); > + > +int i915_user_extensions(struct i915_user_extension __user *ext, > + const i915_user_extension_fn *tbl, > + unsigned long count, > + void *data); > + > +#endif /* I915_USER_EXTENSIONS_H */ > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > index 9726df37c4c4..fcc751aa1ea8 100644 > --- a/drivers/gpu/drm/i915/i915_utils.h > +++ b/drivers/gpu/drm/i915/i915_utils.h > @@ -105,6 +105,13 @@ > __T; \ > }) > > +#define container_of_user(ptr, type, member) ({ \ > + void __user *__mptr = (void __user *)(ptr); \ > + BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \ > + !__same_type(*(ptr), void), \ > + "pointer type mismatch in container_of()"); \ > + ((type __user *)(__mptr - offsetof(type, member))); }) > + > static inline u64 ptr_to_u64(const void *ptr) > { > return (uintptr_t)ptr; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index b10eea3f6d24..f4fa0825722a 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -62,6 +62,26 @@ extern "C" { > #define I915_ERROR_UEVENT "ERROR" > #define I915_RESET_UEVENT "RESET" > > +/* > + * i915_user_extension: Base class for defining a chain of extensions > + * > + * Many interfaces need to grow over time. In most cases we can simply > + * extend the struct and have userspace pass in more data. Another option, > + * as demonstrated by Vulkan's approach to providing extensions for forward > + * and backward compatibility, is to use a list of optional structs to > + * provide those extra details. > + * > + * The key advantage to using an extension chain is that it allows us to > + * redefine the interface more easily than an ever growing struct of > + * increasing complexity, and for large parts of that interface to be > + * entirely optional. The downside is more pointer chasing; chasing across > + * the __user boundary with pointers encapsulated inside u64. > + */ > +struct i915_user_extension { > + __u64 next_extension; > + __u64 name; > +}; > + > /* > * MOCS indexes used for GPU surfaces, defining the cacheability of the > * surface data and the coherency for this data wrt. CPU vs. GPU accesses. >
Quoting Tvrtko Ursulin (2019-03-01 15:39:13) > > On 01/03/2019 14:03, Chris Wilson wrote: > > +int i915_user_extensions(struct i915_user_extension __user *ext, > > + const i915_user_extension_fn *tbl, > > + unsigned long count, > > + void *data) > > +{ > > + unsigned int stackdepth = 512; > > + > > + while (ext) { > > + int err; > > + u64 x; > > + > > + if (!stackdepth--) /* recursion vs useful flexibility */ > > + return -EINVAL; > > I don't get this. What stack? Did you mean "static unsigned int > stackdepth" in case someone puts i915_user_extension into the extension > table? Or just a limit on number of chained extensions? But you are not > processing the recursively here. It's iterative recursion :) I still think of this loop in terms of its simple tail recursion. And if we need to individual levels for unwind, that is a manual stack. -Chris
On 01/03/2019 18:57, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-01 15:39:13) >> >> On 01/03/2019 14:03, Chris Wilson wrote: >>> +int i915_user_extensions(struct i915_user_extension __user *ext, >>> + const i915_user_extension_fn *tbl, >>> + unsigned long count, >>> + void *data) >>> +{ >>> + unsigned int stackdepth = 512; >>> + >>> + while (ext) { >>> + int err; >>> + u64 x; >>> + >>> + if (!stackdepth--) /* recursion vs useful flexibility */ >>> + return -EINVAL; >> >> I don't get this. What stack? Did you mean "static unsigned int >> stackdepth" in case someone puts i915_user_extension into the extension >> table? Or just a limit on number of chained extensions? But you are not >> processing the recursively here. > > It's iterative recursion :) > > I still think of this loop in terms of its simple tail recursion. > > And if we need to individual levels for unwind, that is a manual stack. Okay.. One thought I had - if unwind is too unwieldy, could we mandate each user extension user has an additional output field in the embedding struct which would hold the last processed extension id? This way the state of the object would be less undefined after failure. Userspace would be able to tell how far in the extension chain i915 managed to get to. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-03-04 08:54:10) > > On 01/03/2019 18:57, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-03-01 15:39:13) > >> > >> On 01/03/2019 14:03, Chris Wilson wrote: > >>> +int i915_user_extensions(struct i915_user_extension __user *ext, > >>> + const i915_user_extension_fn *tbl, > >>> + unsigned long count, > >>> + void *data) > >>> +{ > >>> + unsigned int stackdepth = 512; > >>> + > >>> + while (ext) { > >>> + int err; > >>> + u64 x; > >>> + > >>> + if (!stackdepth--) /* recursion vs useful flexibility */ > >>> + return -EINVAL; > >> > >> I don't get this. What stack? Did you mean "static unsigned int > >> stackdepth" in case someone puts i915_user_extension into the extension > >> table? Or just a limit on number of chained extensions? But you are not > >> processing the recursively here. > > > > It's iterative recursion :) > > > > I still think of this loop in terms of its simple tail recursion. > > > > And if we need to individual levels for unwind, that is a manual stack. > Okay.. > > One thought I had - if unwind is too unwieldy, could we mandate each > user extension user has an additional output field in the embedding > struct which would hold the last processed extension id? id? I guess we could do depth, though I guess pointer is more practical. I don't think you mean ext.name as that we expect to repeat a few times. > This way the state of the object would be less undefined after failure. > Userspace would be able to tell how far in the extension chain i915 > managed to get to. But I'm not convinced how practical that is. The answer is pretty much always -EINVAL (anything else is much less ambiguous) and even then you have no idea which field tripped EINVAL or why? -Chris
On 04/03/2019 09:04, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-04 08:54:10) >> >> On 01/03/2019 18:57, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2019-03-01 15:39:13) >>>> >>>> On 01/03/2019 14:03, Chris Wilson wrote: >>>>> +int i915_user_extensions(struct i915_user_extension __user *ext, >>>>> + const i915_user_extension_fn *tbl, >>>>> + unsigned long count, >>>>> + void *data) >>>>> +{ >>>>> + unsigned int stackdepth = 512; >>>>> + >>>>> + while (ext) { >>>>> + int err; >>>>> + u64 x; >>>>> + >>>>> + if (!stackdepth--) /* recursion vs useful flexibility */ >>>>> + return -EINVAL; >>>> >>>> I don't get this. What stack? Did you mean "static unsigned int >>>> stackdepth" in case someone puts i915_user_extension into the extension >>>> table? Or just a limit on number of chained extensions? But you are not >>>> processing the recursively here. >>> >>> It's iterative recursion :) >>> >>> I still think of this loop in terms of its simple tail recursion. >>> >>> And if we need to individual levels for unwind, that is a manual stack. >> Okay.. >> >> One thought I had - if unwind is too unwieldy, could we mandate each >> user extension user has an additional output field in the embedding >> struct which would hold the last processed extension id? > > id? I guess we could do depth, though I guess pointer is more practical. > I don't think you mean ext.name as that we expect to repeat a few times. Pointer yes, I started with a pointer but then for some reason wrote id, where I meant name. >> This way the state of the object would be less undefined after failure. >> Userspace would be able to tell how far in the extension chain i915 >> managed to get to. > > But I'm not convinced how practical that is. The answer is pretty much > always -EINVAL (anything else is much less ambiguous) and even then you > have no idea which field tripped EINVAL or why? Which field tripped -EINVAL is why is always true. I am not 100% convinced myself, just worry about usability. But I don't have an usecase at the moment which would be practically helped with knowing which extension failed. In all cases I can think of it's a programming error ie. should be caught during development and not happen in production. Regards, Tvrtko
On 04/03/2019 09:35, Tvrtko Ursulin wrote: > > On 04/03/2019 09:04, Chris Wilson wrote: >> Quoting Tvrtko Ursulin (2019-03-04 08:54:10) >>> >>> On 01/03/2019 18:57, Chris Wilson wrote: >>>> Quoting Tvrtko Ursulin (2019-03-01 15:39:13) >>>>> >>>>> On 01/03/2019 14:03, Chris Wilson wrote: >>>>>> +int i915_user_extensions(struct i915_user_extension __user *ext, >>>>>> + const i915_user_extension_fn *tbl, >>>>>> + unsigned long count, >>>>>> + void *data) >>>>>> +{ >>>>>> + unsigned int stackdepth = 512; >>>>>> + >>>>>> + while (ext) { >>>>>> + int err; >>>>>> + u64 x; >>>>>> + >>>>>> + if (!stackdepth--) /* recursion vs useful >>>>>> flexibility */ >>>>>> + return -EINVAL; >>>>> >>>>> I don't get this. What stack? Did you mean "static unsigned int >>>>> stackdepth" in case someone puts i915_user_extension into the >>>>> extension >>>>> table? Or just a limit on number of chained extensions? But you are >>>>> not >>>>> processing the recursively here. >>>> >>>> It's iterative recursion :) >>>> >>>> I still think of this loop in terms of its simple tail recursion. >>>> >>>> And if we need to individual levels for unwind, that is a manual stack. >>> Okay.. >>> >>> One thought I had - if unwind is too unwieldy, could we mandate each >>> user extension user has an additional output field in the embedding >>> struct which would hold the last processed extension id? >> >> id? I guess we could do depth, though I guess pointer is more practical. >> I don't think you mean ext.name as that we expect to repeat a few times. > > Pointer yes, I started with a pointer but then for some reason wrote id, > where I meant name. > >>> This way the state of the object would be less undefined after failure. >>> Userspace would be able to tell how far in the extension chain i915 >>> managed to get to. >> >> But I'm not convinced how practical that is. The answer is pretty much >> always -EINVAL (anything else is much less ambiguous) and even then you >> have no idea which field tripped EINVAL or why? > > Which field tripped -EINVAL is why is always true. > > I am not 100% convinced myself, just worry about usability. But I don't > have an usecase at the moment which would be practically helped with > knowing which extension failed. In all cases I can think of it's a > programming error ie. should be caught during development and not happen > in production. To add - if we are adding a new extensible uAPI my gut feeling is we should provision for reporting which stage failed. I think it is easy now, while later it would be much harder. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index a1d834068765..89105b1aaf12 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -46,6 +46,7 @@ i915-y := i915_drv.o \ i915_sw_fence.o \ i915_syncmap.o \ i915_sysfs.o \ + i915_user_extensions.o \ intel_csr.o \ intel_device_info.o \ intel_pm.o \ diff --git a/drivers/gpu/drm/i915/i915_user_extensions.c b/drivers/gpu/drm/i915/i915_user_extensions.c new file mode 100644 index 000000000000..879b4094b2d7 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_user_extensions.c @@ -0,0 +1,43 @@ +/* + * SPDX-License-Identifier: MIT + * + * Copyright © 2018 Intel Corporation + */ + +#include <linux/sched/signal.h> +#include <linux/uaccess.h> +#include <uapi/drm/i915_drm.h> + +#include "i915_user_extensions.h" + +int i915_user_extensions(struct i915_user_extension __user *ext, + const i915_user_extension_fn *tbl, + unsigned long count, + void *data) +{ + unsigned int stackdepth = 512; + + while (ext) { + int err; + u64 x; + + if (!stackdepth--) /* recursion vs useful flexibility */ + return -EINVAL; + + if (get_user(x, &ext->name)) + return -EFAULT; + + err = -EINVAL; + if (x < count && tbl[x]) + err = tbl[x](ext, data); + if (err) + return err; + + if (get_user(x, &ext->next_extension)) + return -EFAULT; + + ext = u64_to_user_ptr(x); + } + + return 0; +} diff --git a/drivers/gpu/drm/i915/i915_user_extensions.h b/drivers/gpu/drm/i915/i915_user_extensions.h new file mode 100644 index 000000000000..313a510b068a --- /dev/null +++ b/drivers/gpu/drm/i915/i915_user_extensions.h @@ -0,0 +1,20 @@ +/* + * SPDX-License-Identifier: MIT + * + * Copyright © 2018 Intel Corporation + */ + +#ifndef I915_USER_EXTENSIONS_H +#define I915_USER_EXTENSIONS_H + +struct i915_user_extension; + +typedef int (*i915_user_extension_fn)(struct i915_user_extension __user *ext, + void *data); + +int i915_user_extensions(struct i915_user_extension __user *ext, + const i915_user_extension_fn *tbl, + unsigned long count, + void *data); + +#endif /* I915_USER_EXTENSIONS_H */ diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index 9726df37c4c4..fcc751aa1ea8 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -105,6 +105,13 @@ __T; \ }) +#define container_of_user(ptr, type, member) ({ \ + void __user *__mptr = (void __user *)(ptr); \ + BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \ + !__same_type(*(ptr), void), \ + "pointer type mismatch in container_of()"); \ + ((type __user *)(__mptr - offsetof(type, member))); }) + static inline u64 ptr_to_u64(const void *ptr) { return (uintptr_t)ptr; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index b10eea3f6d24..f4fa0825722a 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -62,6 +62,26 @@ extern "C" { #define I915_ERROR_UEVENT "ERROR" #define I915_RESET_UEVENT "RESET" +/* + * i915_user_extension: Base class for defining a chain of extensions + * + * Many interfaces need to grow over time. In most cases we can simply + * extend the struct and have userspace pass in more data. Another option, + * as demonstrated by Vulkan's approach to providing extensions for forward + * and backward compatibility, is to use a list of optional structs to + * provide those extra details. + * + * The key advantage to using an extension chain is that it allows us to + * redefine the interface more easily than an ever growing struct of + * increasing complexity, and for large parts of that interface to be + * entirely optional. The downside is more pointer chasing; chasing across + * the __user boundary with pointers encapsulated inside u64. + */ +struct i915_user_extension { + __u64 next_extension; + __u64 name; +}; + /* * MOCS indexes used for GPU surfaces, defining the cacheability of the * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
An idea for extending uABI inspired by Vulkan's extension chains. Instead of expanding the data struct for each ioctl every time we need to add a new feature, define an extension chain instead. As we add optional interfaces to control the ioctl, we define a new extension struct that can be linked into the ioctl data only when required by the user. The key advantage being able to ignore large control structs for optional interfaces/extensions, while being able to process them in a consistent manner. In comparison to other extensible ioctls, the key difference is the use of a linked chain of extension structs vs an array of tagged pointers. For example, struct drm_amdgpu_cs_chunk { __u32 chunk_id; __u32 length_dw; __u64 chunk_data; }; struct drm_amdgpu_cs_in { __u32 ctx_id; __u32 bo_list_handle; __u32 num_chunks; __u32 _pad; __u64 chunks; }; allows userspace to pass in array of pointers to extension structs, but must therefore keep constructing that array along side the command stream. In dynamic situations like that, a linked list is preferred and does not similar from extra cache line misses as the extension structs themselves must still be loaded separate to the chunks array. v2: Apply the tail call optimisation directly to nip the worry of stack overflow in the bud. v3: Defend against recursion. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_user_extensions.c | 43 +++++++++++++++++++++ drivers/gpu/drm/i915/i915_user_extensions.h | 20 ++++++++++ drivers/gpu/drm/i915/i915_utils.h | 7 ++++ include/uapi/drm/i915_drm.h | 20 ++++++++++ 5 files changed, 91 insertions(+) create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.c create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.h