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

Workaround for Linux PowerPC GPL-only cpu_has_feature() #14590

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

Low-power
Copy link
Contributor

Motivation and Context

Workaround #14545

Description

Copied from commit message:

Linux since 4.7 makes interface 'cpu_has_feature' to use jump labels on
powerpc if CONFIG_JUMP_LABEL_FEATURE_CHECKS is enabled, in this case however
this inline function references GPL-only symbol 'cpu_feature_keys'.

ZFS currently uses 'cpu_has_feature' either directly or indirectly from
several places; while it is unknown how this issue didn't break ZFS on 64-bit
little-endian powerpc, it is known to break ZFS with many Linux versions on
both 32-bit and 64-bit big-endian powerpc.

Until this issue is fixed in Linux, we have to workaround it by providing our
own version of the inline function without using jump labels.

How Has This Been Tested?

It has been tested with a root-on-ZFS configuration using linux-image-6.1.0-6-powerpc64 from Debian, as well as another system using Linux 4.1.42 to ensure it didn't break older Linux versions that didn't affected by the bug.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@Low-power Low-power force-pushed the powerpc-cpu-has-feature-compat branch 3 times, most recently from aa96b32 to a070e76 Compare March 7, 2023 10:28
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 7, 2023
@Low-power
Copy link
Contributor Author

I just borrowed your comment and created a #define for cpu_has_feature in the only place it should be needed.

I actually plan to do the same originally, but unfortunately it didn't work, due to the reference by flush_dcache_page. This is an inline function defined in arch/powerpc/include/asm/cacheflush.h of Linux source tree, a later #define can't override cpu_has_feature referenced already by it.

@behlendorf
Copy link
Contributor

I see, it was exported as a symbol until the 5.13 kernel where it was converted to a static inline. That's why I missed it. In that case, how about we #define our own version of flush_dcache_page when flush_dcache_page isn't available to us. It's a simple enough function and hasn't changed since it was moved to the header in 5.13. Then since we're defining it it will be able to use our cpu_has_feature macro wrapper.

@Low-power Low-power force-pushed the powerpc-cpu-has-feature-compat branch from a070e76 to fd0ba0b Compare March 9, 2023 06:14
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for sorting this out. Just one comment inline.

include/os/linux/kernel/linux/dcache_compat.h Outdated Show resolved Hide resolved
Linux since 4.7 makes interface 'cpu_has_feature' to use jump labels on
powerpc if CONFIG_JUMP_LABEL_FEATURE_CHECKS is enabled, in this case
however the inline function references GPL-only symbol
'cpu_feature_keys'.

ZFS currently uses 'cpu_has_feature' either directly or indirectly from
several places; while it is unknown how this issue didn't break ZFS on
64-bit little-endian powerpc, it is known to break ZFS with many Linux
versions on both 32-bit and 64-bit big-endian powerpc.

Until this issue is fixed in Linux, we have to workaround it by
overriding affected inline functions without depending on
'cpu_feature_keys'.

Signed-off-by: WHR <msl0000023508@gmail.com>
@Low-power Low-power force-pushed the powerpc-cpu-has-feature-compat branch from fd0ba0b to 73fc2f5 Compare March 9, 2023 18:04
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 10, 2023
@behlendorf behlendorf merged commit 589f59b into openzfs:master Mar 10, 2023
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Mar 13, 2023
Linux since 4.7 makes interface 'cpu_has_feature' to use jump labels on
powerpc if CONFIG_JUMP_LABEL_FEATURE_CHECKS is enabled, in this case
however the inline function references GPL-only symbol
'cpu_feature_keys'.

ZFS currently uses 'cpu_has_feature' either directly or indirectly from
several places; while it is unknown how this issue didn't break ZFS on
64-bit little-endian powerpc, it is known to break ZFS with many Linux
versions on both 32-bit and 64-bit big-endian powerpc.

Until this issue is fixed in Linux, we have to workaround it by
overriding affected inline functions without depending on
'cpu_feature_keys'.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: WHR <msl0000023508@gmail.com>
Closes openzfs#14590
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 16, 2023
Linux since 4.7 makes interface 'cpu_has_feature' to use jump labels on
powerpc if CONFIG_JUMP_LABEL_FEATURE_CHECKS is enabled, in this case
however the inline function references GPL-only symbol
'cpu_feature_keys'.

ZFS currently uses 'cpu_has_feature' either directly or indirectly from
several places; while it is unknown how this issue didn't break ZFS on
64-bit little-endian powerpc, it is known to break ZFS with many Linux
versions on both 32-bit and 64-bit big-endian powerpc.

Until this issue is fixed in Linux, we have to workaround it by
overriding affected inline functions without depending on
'cpu_feature_keys'.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: WHR <msl0000023508@gmail.com>
Closes openzfs#14590
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 2, 2023
Linux since 4.7 makes interface 'cpu_has_feature' to use jump labels on
powerpc if CONFIG_JUMP_LABEL_FEATURE_CHECKS is enabled, in this case
however the inline function references GPL-only symbol
'cpu_feature_keys'.

ZFS currently uses 'cpu_has_feature' either directly or indirectly from
several places; while it is unknown how this issue didn't break ZFS on
64-bit little-endian powerpc, it is known to break ZFS with many Linux
versions on both 32-bit and 64-bit big-endian powerpc.

Until this issue is fixed in Linux, we have to workaround it by
overriding affected inline functions without depending on
'cpu_feature_keys'.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: WHR <msl0000023508@gmail.com>
Closes openzfs#14590
tonyhutter pushed a commit that referenced this pull request Jun 5, 2023
Linux since 4.7 makes interface 'cpu_has_feature' to use jump labels on
powerpc if CONFIG_JUMP_LABEL_FEATURE_CHECKS is enabled, in this case
however the inline function references GPL-only symbol
'cpu_feature_keys'.

ZFS currently uses 'cpu_has_feature' either directly or indirectly from
several places; while it is unknown how this issue didn't break ZFS on
64-bit little-endian powerpc, it is known to break ZFS with many Linux
versions on both 32-bit and 64-bit big-endian powerpc.

Until this issue is fixed in Linux, we have to workaround it by
overriding affected inline functions without depending on
'cpu_feature_keys'.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: WHR <msl0000023508@gmail.com>
Closes #14590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants