| Message ID | 20230516145334.1271347-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Feature check cleanup | expand |
On 16.05.2023 16:53, Andrew Cooper wrote: > --- a/xen/arch/x86/include/asm/cpufeature.h > +++ b/xen/arch/x86/include/asm/cpufeature.h > @@ -7,6 +7,7 @@ > #define __ASM_I386_CPUFEATURE_H > > #include <xen/const.h> > +#include <xen/stdbool.h> > #include <asm/cpuid.h> This isn't needed up here, and ... > @@ -17,7 +18,6 @@ > #define X86_FEATURE_ALWAYS X86_FEATURE_LM > > #ifndef __ASSEMBLY__ > -#include <xen/bitops.h> ... putting it here would (a) eliminate a header dependency for assembly sources including this file (perhaps indirectly) and (b) eliminate the risk of a build breakage if something was added to that header which isn't valid assembly. Preferably with the adjustment Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 17/05/2023 3:01 pm, Jan Beulich wrote: > On 16.05.2023 16:53, Andrew Cooper wrote: >> --- a/xen/arch/x86/include/asm/cpufeature.h >> +++ b/xen/arch/x86/include/asm/cpufeature.h >> @@ -7,6 +7,7 @@ >> #define __ASM_I386_CPUFEATURE_H >> >> #include <xen/const.h> >> +#include <xen/stdbool.h> >> #include <asm/cpuid.h> > This isn't needed up here, and ... > >> @@ -17,7 +18,6 @@ >> #define X86_FEATURE_ALWAYS X86_FEATURE_LM >> >> #ifndef __ASSEMBLY__ >> -#include <xen/bitops.h> > ... putting it here would (a) eliminate a header dependency for > assembly sources including this file (perhaps indirectly) and (b) > eliminate the risk of a build breakage if something was added to > that header which isn't valid assembly. b) That's a weak argument for headers in general, but you're saying it about our copy of stdbool.h which probably the least likely header for that ever to be true. a) Not really, because cpuid.h pulls in a reasonable chunk of the world, including types.h and therefore stdbool.h. cpuid.h is necessary to make the X86_FEATURE_ALWAYS -> X86_FEATURE_LM, which is used by asm for alternatives. I'm tempted to just omit it. cpufeature.h has one of the most tangled set of headers we've got, and I can't find any reasonable way to make it less bad. > Preferably with the adjustment > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, ~Andrew
On 19.05.2023 17:24, Andrew Cooper wrote: > On 17/05/2023 3:01 pm, Jan Beulich wrote: >> On 16.05.2023 16:53, Andrew Cooper wrote: >>> --- a/xen/arch/x86/include/asm/cpufeature.h >>> +++ b/xen/arch/x86/include/asm/cpufeature.h >>> @@ -7,6 +7,7 @@ >>> #define __ASM_I386_CPUFEATURE_H >>> >>> #include <xen/const.h> >>> +#include <xen/stdbool.h> >>> #include <asm/cpuid.h> >> This isn't needed up here, and ... >> >>> @@ -17,7 +18,6 @@ >>> #define X86_FEATURE_ALWAYS X86_FEATURE_LM >>> >>> #ifndef __ASSEMBLY__ >>> -#include <xen/bitops.h> >> ... putting it here would (a) eliminate a header dependency for >> assembly sources including this file (perhaps indirectly) and (b) >> eliminate the risk of a build breakage if something was added to >> that header which isn't valid assembly. > > b) That's a weak argument for headers in general, but you're saying it > about our copy of stdbool.h which probably the least likely header for > that ever to be true. > > a) Not really, because cpuid.h pulls in a reasonable chunk of the world, > including types.h and therefore stdbool.h. cpuid.h is necessary to make > the X86_FEATURE_ALWAYS -> X86_FEATURE_LM, which is used by asm for > alternatives. > > I'm tempted to just omit it. cpufeature.h has one of the most tangled > set of headers we've got, and I can't find any reasonable way to make it > less bad. Yeah, omitting would certainly be fine with me. Jan
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h index 4140ec0938b2..4f827cc6ff91 100644 --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -7,6 +7,7 @@ #define __ASM_I386_CPUFEATURE_H #include <xen/const.h> +#include <xen/stdbool.h> #include <asm/cpuid.h> #define cpufeat_word(idx) ((idx) / 32) @@ -17,7 +18,6 @@ #define X86_FEATURE_ALWAYS X86_FEATURE_LM #ifndef __ASSEMBLY__ -#include <xen/bitops.h> struct cpuinfo_x86 { unsigned char x86; /* CPU family */ @@ -43,8 +43,15 @@ struct cpuinfo_x86 { extern struct cpuinfo_x86 boot_cpu_data; -#define cpu_has(c, bit) test_bit(bit, (c)->x86_capability) -#define boot_cpu_has(bit) test_bit(bit, boot_cpu_data.x86_capability) +static inline bool cpu_has(const struct cpuinfo_x86 *info, unsigned int feat) +{ + return info->x86_capability[cpufeat_word(feat)] & cpufeat_mask(feat); +} + +static inline bool boot_cpu_has(unsigned int feat) +{ + return cpu_has(&boot_cpu_data, feat); +} #define CPUID_PM_LEAF 6 #define CPUID6_ECX_APERFMPERF_CAPABILITY 0x1
One area where Xen deviates from Linux is that test_bit() forces a volatile read. This leads to poor code generation, because the optimiser cannot merge bit operations on the same word. Drop the use of test_bit(), and write the expressions in regular C. This removes the include of bitops.h (which is a frequent source of header tangles), and it offers the optimiser far more flexibility. Bloat-o-meter reports a net change of: add/remove: 0/0 grow/shrink: 21/87 up/down: 641/-2751 (-2110) with half of that in x86_emulate() alone. vmx_ctxt_switch_to() seems to be the fastpath with the greatest delta at -24, where the optimiser has successfully removed the branch hidden in cpu_has_msr_tsc_aux. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/include/asm/cpufeature.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) base-commit: 8f9c8274a4e3e860bd777269cb2c91971e9fa69e prerequisite-patch-id: ceeba7d5ab9498cb188e5012953c7e8c9a86347d prerequisite-patch-id: c0957b9e1157ae6eb8de973c96716fd02587c486 prerequisite-patch-id: d2574bba15748cd021e5b33fa50e6cadc38863b6 prerequisite-patch-id: 0f66cd4287ffdc06f24dc01c7d26fb428f3e8c09 prerequisite-patch-id: a585f61b546ff96be3624ff253f8100b2f465de6 prerequisite-patch-id: 54551cdefaca083b4a4b97528d27d0f3dc9753ee prerequisite-patch-id: 051423463e4a34728ab524f03e801e7103777684