Re: [PATCH REGRESSION] um: Fix cpu_feature_enabled() build breakage

>
> Thanks for this pointer.
>
> > https://lore.kernel.org/all/[email protected]/
>
> Indeed, both our fixes seem to be for the same breakage.
>
> Personally I prefer my patch, as it reverts the specific change which
> introduced the breakage into the tree. Franz' change does not look incorrect
> but is a larger change and should go through a regular release cycle.

I also prefer your patch. It is much simpler and doesn't require
changing the cpu flags at all.

However, getting the minimal patch for um in ASAP would also be good,
as a broken master is not nice for development.

Thanks,
Fabian

>
> > Ciao, Thorsten
> > > Two independent breakages meet here:
> > >
> > >  1. cpu_feature_enabled() and this_cpu_has() still test
> > >     DISABLED_MASK_BIT_SET()/REQUIRED_MASK_BIT_SET().  Those used to
> > >     come from <asm/disabled-features.h>/<asm/required-features.h> via
> > >     <asm/cpufeatures.h>, but since commit 8f97566c8a81
> > >     ("x86/cpufeatures: Remove {disabled,required}-features.h") they
> > >     live in the generated <asm/cpufeaturemasks.h>, which only the
> > >     arch/x86 archprepare rule generates.  ARCH=um never generates nor
> > >     includes it, so the references have been dangling since then.
> > >
> > >  2. The _static_cpu_has() macro expands to itself.  Commit
> > >     3eaa50e1e255 ("x86/cpu: Hide and rename static_cpu_has()") renamed
> > >     the inline function _static_cpu_has() -> __static_cpu_has() and
> > >     the macro static_cpu_has() -> _static_cpu_has(), but in the UML
> > >     header the macro's out-of-line branch kept calling
> > >     _static_cpu_has(), which now names the macro itself and is left
> > >     unexpanded by the preprocessor.
> > >
> > > Fix the macro to call __static_cpu_has(), as the x86 header does, and
> > > drop the mask based short-circuits from cpu_feature_enabled() and
> > > this_cpu_has().  UML has no compile-time feature masking to express:
> > > capabilities are copied from the host's CPUID at boot, see commit
> > > d8fb32f4790f ("um: Add support for host CPU flags and alignment"),
> > > and generating cpufeaturemasks.h from a UML .config would wrongly
> > > mark features disabled simply because the gating CONFIG_X86_* symbols
> > > do not exist for ARCH=um.
> > >
> > > Fixes: 3eaa50e1e255 ("x86/cpu: Hide and rename static_cpu_has()")
> > > Fixes: 8f97566c8a81 ("x86/cpufeatures: Remove {disabled,required}-features.h")
> > > Signed-off-by: Fabian Franz <[email protected]>
> > > Assisted-By: Claude Fable 5 <[email protected]>
> > > ---
> > >  arch/um/include/asm/cpufeature.h | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/um/include/asm/cpufeature.h b/arch/um/include/asm/cpufeature.h
> > > index f7770083c0a4..20b2c7ead62e 100644
> > > --- a/arch/um/include/asm/cpufeature.h
> > > +++ b/arch/um/include/asm/cpufeature.h
> > > @@ -37,8 +37,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
> > >      test_cpu_cap(c, bit)
> > >
> > >  #define this_cpu_has(bit)                                          \
> > > -   (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 :  \
> > > -    x86_this_cpu_test_bit(bit, cpu_info.x86_capability))
> > > +    x86_this_cpu_test_bit(bit, cpu_info.x86_capability)
> > >
> > >  /*
> > >   * This macro is for detection of features which need kernel
> > > @@ -48,8 +47,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
> > >   * supporting a possible guest feature where host support for it
> > >   * is not relevant.
> > >   */
> > > -#define cpu_feature_enabled(bit)   \
> > > -   (__builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0 : _static_cpu_has(bit))
> > > +#define cpu_feature_enabled(bit)   _static_cpu_has(bit)
> > >
> > >  #define boot_cpu_has(bit)  cpu_has(&boot_cpu_data, bit)
> > >
> > > @@ -112,7 +110,7 @@ static __always_inline bool __static_cpu_has(u16 bit)
> > >  (                                                          \
> > >     __builtin_constant_p(boot_cpu_has(bit)) ?               \
> > >             boot_cpu_has(bit) :                             \
> > > -           _static_cpu_has(bit)                            \
> > > +           __static_cpu_has(bit)                           \
> > >  )
> > >
> > >  #define cpu_has_bug(c, bit)                cpu_has(c, (bit))
> > >
> > > --
> > > 2.43.0
> > >
> > > Every ARCH=um build has failed since the 7.3 merge window.
> > >
> > > #regzbot introduced: 3ed403bbc967
> > >
> > > Steps to reproduce on master:
> > >
> > > make ARCH=um O=/tmp/um-build defconfig
> > > make ARCH=um O=/tmp/um-build -j$(nproc)
> > >
> > > fails with:
> > >
> > > /home/ubuntu/projects/linux/arch/x86/include/asm/tsc.h: In function ‘get_cycles’:
> > > /home/ubuntu/projects/linux/arch/um/include/asm/cpufeature.h:52:39: error: implicit declaration of function ‘DISABLED_MASK_BIT_SET’ [-Werror=implicit-function-declaration]
> > >    52 |         (__builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0 : _static_cpu_has(bit))
> > >       |                                       ^~~~~~~~~~~~~~~~~~~~~
> > > /home/ubuntu/projects/linux/arch/x86/include/asm/tsc.h:79:14: note: in expansion of macro ‘cpu_feature_enabled’
> > >    79 |         if (!cpu_feature_enabled(X86_FEATURE_TSC))
> > >       |              ^~~~~~~~~~~~~~~~~~~
> > > /home/ubuntu/projects/linux/arch/um/include/asm/cpufeature.h:115:17: error: implicit declaration of function ‘_static_cpu_has’; did you mean ‘__static_cpu_has’? [-Werror=implicit-function-declaration]
> > >   115 |                 _static_cpu_has(bit)                            \
> > >       |                 ^~~~~~~~~~~~~~~
> > > /home/ubuntu/projects/linux/arch/um/include/asm/cpufeature.h:52:72: note: in expansion of macro ‘_static_cpu_has’
> > >    52 | builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0 : _static_cpu_has(bit))
> > >       |                                                             ^~~~~~~~~~~~~~~
> > >
> > > If this is known please disregard. I am also not 100% sure if this is the right fix or if it's better to bring back the macros to um.
> > >
> > > In any case it would be good for um to build again on the master branch out of the box.
>