On 11/21/23 13:13, Kevin Brodsky wrote:

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.

interestingly... existing arm64 binaries haven't broken. i literally am using the debian arm64 drm package binaries... they work on my kernel. tbh i actually haven't dug into why. but i can run all the arm64 compiled userspace tests with an arm64 libdrm right from upstream debian pkgs and ... they work. :) my newly built purecap ones also work...

but if i do run into issues i can just hardcode the ioctl values indeed. i'll take that as a "solve as i find the problem".

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

this was one of my "try and touch original code as little as possible" bits - thus why my first clean version had lots of them. wanting to keep that code path as untouched as possible. i can fix this and remove some more ifdefs. :)

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

yeah - vblank was the exception. indeed probably coming from some very old code but needing to stay this way for compat reasons. 32bt compat specifically.

and indeed all i wanted is o9ur kernel 0- if compiled for some 32bit system (as a 32 or 64 bit kernel) would keep working irrespective of purecap or not.

• 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

correct - this was a call between following the 32 naming scheme that for a purecap kernel is actually 64bit - thus easy to grep for 32 and find compat handling code. it lies but it's easier to maintain the code as the drm_ioc32 code is named this way. indeed it's technically untrue. it's compat64 really... but the choice is between being consistent for the same code handling the same thing vs being technically accurate... :)

i favor consistency. you seem to favor correctness... but with the exception of changing t existing 32bit handling code naming... :)

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

yeah. this is where we differ. keeping it named "32" for existing code is simply sane for maintaining a fork - it'd add so much noise to the diffs and become hard to maintain. given this reality we have to deal with... do we name other compat handling code consistently... or do we name it accurately. that is the debate :) i err on the side of consistent-but-wrong rather than inconsistent-but-sometimes-right-sometimes-wrong :)

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

i just kept the code exactly as it was - i didn't upgrade/fix this. again one of those "try and disturb existing code as little as possible" things. i can change it.

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.

one of these "not necessary to make it work" changes but "make code nicer" changes i would probably do in a separate patch set on top tbh. as such it's kind of hard to work out how to break this up into patches and i'm thinking a first "minimum work to make it function" patch and then "this cleans it up to be prettier" set of patches.

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

pretty minimal ifdefs here - but i do need to use uintcap_t here. even the cheri guys have the same ifdef - they just forgot the unsigned log (which in my version will degrade to 32bits on 32bit systems as it becomes a ulong).

...

...

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

On 24/11/2023 22:00, Carsten Haitzler wrote:

...

...

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

interestingly... existing arm64 binaries haven't broken. i literally am using the debian arm64 drm package binaries... they work on my kernel. tbh i actually haven't dug into why. but i can run all the arm64 compiled userspace tests with an arm64 libdrm right from upstream debian pkgs and ... they work. :) my newly built purecap ones also work...

Well.. a bit of digging... it's because the drm_icotl dispatch func strips off the parts of the ioctl # that encode size etc. ... it just strips down to ioctl "number" that  drm defines.

unsigned int nr = DRM_IOCTL_NR(cmd);

that means that for:

#define DRM_IOCTL_MODE_GETRESOURCES     DRM_IOWR(0xA0, struct drm_mode_card_res)

I end up with 0xA0 ... that's all drm cares about. it then dispatches in its local table from here (with range checks etc. done before) so... it seems it works by either deliberate design or by luck of the design... but it will consistently work unless drm core code stops stripping this out.

so ... i'm safe. :) it ends up with 2 different ioctl values coming into the kernel but it ends up stripping out the size bits along the way and calling the right func which means the code can continue as it does now. i'm not going to suddenly see it break - i wasn't getting magically lucky.

Very interesting, in fact it's even better than that: drm_ioctl() also looks at the size (_IOC_SIZE(cmd)) and uses that to decide how much to copy in/out! It's as if drm_ioctl() was already meant to handle differences in size between compat and native struct... Very nice, for once that's something forward-thinking in an ioctl handler :D In fact that makes me feel like we could use the same approach in other situations where the command value changes, including the MMC one we've already encountered. Need to think about it some more.

Kevin