On 20/11/2023 10:38, Carsten Haitzler wrote:
On 11/13/23 12:24, Carsten Haitzler wrote:
Just thought it's time to share the current state of drm purecap work:
Kernel: https://git.morello-project.org/carhai01/linux-drm-purecap/-/commit/7ea169ab...
And a less ifdeffy version:
https://git.morello-project.org/carhai01/linux-drm-purecap/-/commit/753565cb...
Definitely an improvement! A few more general comments:
* Maybe the most important issue is the discussion at [1]: AFAICT all these uapi struct changes will cause the DRM_IOCTL_* command values to change on PCuABI kernels, which will break existing arm64 binaries. The approach we went for in the MMC driver is to hardcode the struct size in the command definition, so that it remains unchanged despite the native struct growing in PCuABI. It's not great, honestly, but the only plausible alternative seems to consider different command values in compat64 (i.e. a separate top-level compat handler that considers the appropriate command value). Certainly not unprecedented, but quite a bit of extra work.
* Also mentioned in [1], there is no need to use an #ifdef when replacing __u64 with __kernel_uintptr_t. It is already defined carefully so that it is equivalent to __u64 in !PCuABI.
* The one case of unsigned long representing a pointer (drm_wait_vblank_request::signal) is more problematic, because we don't have an appropriate type to replace it. This is an issue in 32-bit, as __kernel_uintptr_t is always at least 64-bit while unsigned long is 32-bit in that case. It looks a lot like a mistake in the original uapi struct definition, but of course it's too late to fix it. Since it seems to be the only occurrence (fortunately), I don't think it's worth defining a new uapi type. Keeping the #ifdef __CHERI__ you currently have should be good enough: from a userspace perspective, we can rely on it not being defined in 32-bit, so unsigned long will still be used as intended. From a kernel perspective, we don't support 32-bit when Morello / CHERI support is enabled, so breaking compat32 is not a big concern if __CHERI__ is defined.
* I'm not sure it makes sense to name the new structs drm_*32. They are different from the existing compat structs in that they are only ever used for compat64, in other words they are irrelevant to 32-bit. Calling them compat_drm_* is not in line with the others, but I think that would actually be a good thing. The fact that the existing drm_*32 structs are not actually specific to 32-bit (we use them in compat64 too) is unfortunate but I see it as a separate issue (and quite clearly the hassle of renaming them is not justified for us).
* Neither uaddr_to_user_ptr() nor u64_to_user_ptr() is meant for compat pointers. compat_ptr() should be used instead (see also [2]). "drm: Explicitly create user pointers" uses uaddr_to_user_ptr() as a stopgap to create user pointers from a __u64 (whether in native or compat). Essentially, compat_ptr() is normal usage and meant to stay, while uaddr_to_user_ptr() is only temporary. u64_to_user_ptr() is equivalent to the latter and should eventually disappear - this is one of our goals.
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... [2] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/master/D...
DRM: https://git.morello-project.org/carhai01/drm-linux-morello-purecap/-/commit/...
Just one comment here: you can avoid quite a bit of #ifdef'ing by using uintptr_t - effectively equivalent to uintcap_t in purecap, and to unsigned long otherwise.
All the DRM tests ow pass for compat and purecap.
I'm of 2 minds in the kernel code. I could go mimic the "copy field" stuff from EBPF but it's still going to be a bit messy. DRM code calls everything "compat" "32" so I'm keeping with that naming scheme (compat for us is 64bit). I've kept the compat structs at the entry points. I could copy to a local "native" struct I guess... It'd mean I carry more local data than I actually need/use.
I don't have a strong opinion on that matter. The sort of refactoring you did, introducing a local variable for each field, seems reasonable.
Kevin