Use standard C types for 32 and 64 bit variables.

[petterreinholdtsen]

This make the code build on 64 bit PowerPC. It also ensure standard user space types are used instead of Linux specific types. This was required to get the code to build on amd64, where __u64 is 'unsigned long long', while uint64_t is 'unsigned long' (both with sizeof() == 8), and PRIx64 expect the latter.

All formatting of 64 bit types are converted to use PRIxN style formatting statements. Some 32 bit use is also converted, but did not try to track down every use as this do not affect PowerPC. Also drop the <linux/ioctl.h> include, as it seem to be unused.

This addresses https://bugs.debian.org/1081303

Fixes #14.

[lancesix]

Hi,

Thanks for the patch. I really do not think we should change the kfd_ioctl.h file for this as this is a verbotim copy of the kernel interface, and ideally one should be able to build the rest of rocm-dbgapi by relying on the version from /usr/include/linux/kfd_ioctl.h.

I think a more desirable approach could be to use the PRIxN macros in the format as this patch proposes, but be sure to static_cast<uint64_t> () all the __u64 values (similar for the 32 bits values). This is not ideal, but this would allow us to be able to log values from the driver without having to alter the reference header.

[petterreinholdtsen]

What about dropping the renaming of __u32, __s32 and __u64 and instead only change the #include in kfd_ioctl.h to something like this:

#include <stdint.h>
typedef int32_t __s32;
typedef uint32_t __u32;
typedef uint64_t __u64;

The change would be a lot smaller, while the effect is the same. It seem like a better option than hiding any problems with casting.

[lancesix]

The main limitation I see for this this approach is that it prevents up from dropping the copy of kfd_ioctl.h once that interface gets stable enough. Ideally, we should not embed it at all, and rely on /usr/include/linux/kfd_ioctl.h instead.

[lancesix]

cc @lmoriche

[petterreinholdtsen]

As can be seen on <URL:https://buildd.debian.org/status/package.php?p=rocdbgapi>, the Debian patch build on both amd64, arm64 and ppc64el. Any idea when a new upstream release will also work on ppc64el? Package status is provieded from <URL: https://tracker.debian.org/pkg/rocdbgapi >.

…

-- Happy hacking Petter Reinholdtsen

[petterreinholdtsen]

[lancesix]

The main limitation I see for this this approach is that it prevents up from dropping the copy of kfd_ioctl.h once that interface gets stable enough. Ideally, we should not embed it at all, and rely on `/usr/include/linux/kfd_ioctl.h` instead.

Perhaps you can convince the maintainers of /usr/include/linux/kfd_ioctl.h to switch to the standard C types? These types have been around since C99, ref <URL: https://en.cppreference.com/w/c/types/integer >, which is more than 25 years old. It is no longer necessary to make up ones own 32 and 64 bit types, like the __u32 and __u64 typedefs used here.

…

-- Happy hacking Petter Reinholdtsen

[lancesix]

Can you update the patch to use the approach you suggested earlier with:

#include <stdint.h>
typedef int32_t __s32;
typedef uint32_t __u32;
typedef uint64_t __u64;

That seems to be the best compromise at this point. If at some point we end up moving to not using a copy of this file in our repo, we'll revisit this.

I'm going to be away for a few days, I'll follow-up on this PR when I come back late next week.

[petterreinholdtsen]

[lancesix]

Can you update the patch to use the approach you suggested earlier with:

Sure. Done.

…

-- Happy hacking Petter Reinholdtsen

[zumbi]

I have test built the patch on ppc64le and I confirm this builds fine on that architecture.