Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update s390x syscall wrappers for RHEL-9.6 #1426

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

joe-lawrence
Copy link
Contributor

@@ -92,7 +92,8 @@
#elif defined(CONFIG_S390)

/* arch/s390/include/asm/syscall_wrapper.h versions */
# if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 3, 0)
# if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 3, 0) || \
RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(9, 6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to "#ifdef RHEL_RELEASE_CODE" before using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that's right.

BTW, do you know if the preprocessor can handle this on a single line like:

#if defined(RHEL_RELEASE_CODE) && RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(9, 6)

w/o resorting to defining the macros in the non-RH case:

#ifndef RHEL_RELEASE_CODE
# define RHEL_RELEASE_CODE 1
# define RHEL_RELEASE_VERSION(x,y) 0
#endif

In other places in this repo, we defined HAVE_FEATURE macros based on up/down-stream versions and then switched around that instead. Do you have a preference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should work. Either way works for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm not sure if that one-liner will work, i.e., I don't know whether the preprocessor stops evaluating the line when defined() returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm not sure if that one-liner will work, i.e., I don't know whether the preprocessor stops evaluating the line when defined() returns false.

Seems like it doesn't:

$ cat test.c
#if defined(RHEL_RELEASE_CODE) && defined(RHEL_RELEASE_VERSION) && RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(9, 6)
#endif

$ gcc -c test.c -o test
test.c:1:109: error: missing binary operator before token "("
    1 | #if defined(RHEL_RELEASE_CODE) && defined(RHEL_RELEASE_VERSION) && RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(9, 6)
      |                                                                                                             ^

And my thought of trying to define RHEL_RELEASE_CODE / RHEL_RELEASE_VERSION wouldn't work too well either as it would only be accurate if the code checks RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION. Flip it around to <= and it will enable unintended blocks :(

So I think it will have to be the longer HAVE_xyz form.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a shorter way to do it, how about (on top of this MR):

diff --git a/kmod/patch/kpatch-syscall.h b/kmod/patch/kpatch-syscall.h
index 8bd94f3a897f..058380bd88b3 100644
--- a/kmod/patch/kpatch-syscall.h
+++ b/kmod/patch/kpatch-syscall.h
@@ -91,9 +91,22 @@
 
 #elif defined(CONFIG_S390)
 
+# if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 3, 0)
+#  define KPATCH_SYSCALL_WRAPPERS_V2
+# else
+#  define KPATCH_SYSCALL_WRAPPERS_V1
+# endif
+
+# if defined(RHEL_RELEASE_CODE)
+#  if RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(9, 6)
+#   define KPATCH_SYSCALL_WRAPPERS_V2
+#  else
+#   define KPATCH_SYSCALL_WRAPPERS_V1
+# endif
+
+
 /* arch/s390/include/asm/syscall_wrapper.h versions */
-# if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 3, 0) || \
-     RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(9, 6)
+#if defined(KPATCH_SYSCALL_WRAPPERS_V2)
 
 #define __KPATCH_S390_SYS_STUBx(x, name, ...)                                          \
        long __s390_sys##name(struct pt_regs *regs);                            \
@@ -158,7 +171,7 @@
        __diag_pop();                                                                   \
        static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
 
-#endif /* LINUX_VERSION_CODE */
+#endif /* KPATCH_SYSCALL_WRAPPERS_V2 */
 
 #elif defined(CONFIG_PPC64)
```

@joe-lawrence joe-lawrence force-pushed the s390x-rhel-9.6-syscall branch from 524e071 to 2b76244 Compare December 9, 2024 16:06
@joe-lawrence
Copy link
Contributor Author

v2

  • Created KPATCH_SYSCALL_WRAPPERS_Vx macros for the s390x code to switch on

@jpoimboe
Copy link
Member

jpoimboe commented Dec 9, 2024

Looks good, for readability can we do something like below (basically V1 first and add a comment to the '#else')?

#ifdef KPATCH_SYSCALL_WRAPPERS_V1

....

#else /* KPATCH_SYSCALL_WRAPPERS_V2 */

....

#endif /* KPATCH_SYSCALL_WRAPPERS_V2 */

@joe-lawrence joe-lawrence force-pushed the s390x-rhel-9.6-syscall branch from 2b76244 to 73fcbe0 Compare December 9, 2024 20:58
@joe-lawrence
Copy link
Contributor Author

v3:

  • Tweak #else line as suggested by Josh

@jpoimboe
Copy link
Member

jpoimboe commented Dec 9, 2024

I was also suggesting the V1 should come first, just so they're in logical order.

RHEL-9.6 backported the upstream v6.3 s390x syscall updates, so add a
distro-specific kernel version check around the correct definitions.

Signed-off-by: Joe Lawrence <[email protected]>
@joe-lawrence joe-lawrence force-pushed the s390x-rhel-9.6-syscall branch from 73fcbe0 to e42c799 Compare December 10, 2024 00:00
@joe-lawrence
Copy link
Contributor Author

v4:

  • Move the s390x syscall wrappers into ascending version order now that they don't (directly) rely on LINUX_VERSION_CODE inequalities.

@joe-lawrence joe-lawrence merged commit a9a7360 into dynup:master Dec 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants