| Message ID | 6b21fb046cf1c8ca760f5ad72fa3cc13b59c4069.1743092485.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] xen/config.h: Move BITS_PER_* definitions from asm/config.h to xen/config.h | expand |
On 27.03.2025 18:33, Oleksii Kurochko wrote: > BITS_PER_* values can be defined in a common way using compiler-provided macros. > Thus, these definitions are moved to xen/config.h to reduce duplication across > architectures. > > Additionally, *_BYTEORDER macros are removed, as BITS_PER_* values now come > directly from the compiler environment. > > The arch_fls() implementation for Arm and PPC is updated to use BITS_PER_INT > instead of a hardcoded value of 32. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit ... > --- a/xen/include/xen/config.h > +++ b/xen/include/xen/config.h > @@ -98,4 +98,14 @@ > #define ZERO_BLOCK_PTR ((void *)-1L) > #endif > > +#define BYTES_PER_LONG __SIZEOF_LONG__ ... I remain unconvinced that we actually need this (yet then not its sibling for int). All uses I can spot could be replaced by sizeof(long). This (and its sibling) would be needed only for #if uses, and there I would then wonder why we couldn't use __SIZEOF_LONG__ directly, now that we assume its universally available. Jan
On 28/03/2025 7:49 am, Jan Beulich wrote: > On 27.03.2025 18:33, Oleksii Kurochko wrote: >> BITS_PER_* values can be defined in a common way using compiler-provided macros. >> Thus, these definitions are moved to xen/config.h to reduce duplication across >> architectures. >> >> Additionally, *_BYTEORDER macros are removed, as BITS_PER_* values now come >> directly from the compiler environment. >> >> The arch_fls() implementation for Arm and PPC is updated to use BITS_PER_INT >> instead of a hardcoded value of 32. >> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > albeit ... > >> --- a/xen/include/xen/config.h >> +++ b/xen/include/xen/config.h >> @@ -98,4 +98,14 @@ >> #define ZERO_BLOCK_PTR ((void *)-1L) >> #endif >> >> +#define BYTES_PER_LONG __SIZEOF_LONG__ > ... I remain unconvinced that we actually need this (yet then not its sibling > for int). All uses I can spot could be replaced by sizeof(long). Perhaps, but that's orthogonal to centralising the definition. It can be deferred. > This (and its > sibling) would be needed only for #if uses, and there I would then wonder why > we couldn't use __SIZEOF_LONG__ directly, now that we assume its universally > available. Looking at the usages, sizeof(long) would be best, and shortest. While we do have a whole bunch of constants which are universally available now, I still want to use the more normal terms where possible. e.g. the difference between __BYTE_ORDER__ and BYTE_ORDER is relevant for the code shared with userspace, and using BYTE_ORDER across Xen is the cleaner option. ~Andrew
On 3/28/25 8:49 AM, Jan Beulich wrote: > On 27.03.2025 18:33, Oleksii Kurochko wrote: >> BITS_PER_* values can be defined in a common way using compiler-provided macros. >> Thus, these definitions are moved to xen/config.h to reduce duplication across >> architectures. >> >> Additionally, *_BYTEORDER macros are removed, as BITS_PER_* values now come >> directly from the compiler environment. >> >> The arch_fls() implementation for Arm and PPC is updated to use BITS_PER_INT >> instead of a hardcoded value of 32. >> >> Suggested-by: Andrew Cooper<andrew.cooper3@citrix.com> >> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com> > Reviewed-by: Jan Beulich<jbeulich@suse.com> > albeit ... > >> --- a/xen/include/xen/config.h >> +++ b/xen/include/xen/config.h >> @@ -98,4 +98,14 @@ >> #define ZERO_BLOCK_PTR ((void *)-1L) >> #endif >> >> +#define BYTES_PER_LONG __SIZEOF_LONG__ > ... I remain unconvinced that we actually need this (yet then not its sibling > for int). All uses I can spot could be replaced by sizeof(long). This (and its > sibling) would be needed only for #if uses, and there I would then wonder why > we couldn't use __SIZEOF_LONG__ directly, now that we assume its universally > available. I left only because|BYTES_PER_LONG| is used in ARM and x86 code. If no one minds, I can replace|BYTES_PER_LONG| with|__SIZEOF_LONG__|. ~ Oleksii
On 27/03/2025 5:33 pm, Oleksii Kurochko wrote: > As Anrew mentioned here https://lore.kernel.org/xen-devel/f294e2b1-db03-46b2-b46d-61e89b55cef3@suse.com/T/#ma4205392964ff913d9dfa8e044a4af59ed6aef88: > "This patch possibly wants to wait until I've fixed the compiler checks to update to the new baseline, ..." This is now committed. ~Andrew
On 27/03/2025 5:33 pm, Oleksii Kurochko wrote: > diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h > index d888b2314d..7d43159efb 100644 > --- a/xen/include/xen/config.h > +++ b/xen/include/xen/config.h > @@ -98,4 +98,14 @@ > #define ZERO_BLOCK_PTR ((void *)-1L) > #endif > > +#define BYTES_PER_LONG __SIZEOF_LONG__ > + > +#define BITS_PER_BYTE __CHAR_BIT__ > +#define BITS_PER_INT (BITS_PER_BYTE * __SIZEOF_INT__) > +#define BITS_PER_LONG (BITS_PER_BYTE * BYTES_PER_LONG) > +#define BITS_PER_LLONG (BITS_PER_BYTE * __SIZEOF_LONG_LONG__) > + > +/* It is assumed that sizeof(void *) == __alignof(void *) */ > +#define POINTER_ALIGN __SIZEOF_POINTER__ > + > #endif /* __XEN_CONFIG_H__ */ As it turns out, this is a prerequisite for the byteswap cleanup. ~Andrew
Hi, On 27/03/2025 17:33, Oleksii Kurochko wrote: > BITS_PER_* values can be defined in a common way using compiler-provided macros. > Thus, these definitions are moved to xen/config.h to reduce duplication across > architectures. > > Additionally, *_BYTEORDER macros are removed, as BITS_PER_* values now come > directly from the compiler environment. > > The arch_fls() implementation for Arm and PPC is updated to use BITS_PER_INT > instead of a hardcoded value of 32. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
diff --git a/xen/arch/arm/include/asm/bitops.h b/xen/arch/arm/include/asm/bitops.h index f163d9bb45..60686a3a55 100644 --- a/xen/arch/arm/include/asm/bitops.h +++ b/xen/arch/arm/include/asm/bitops.h @@ -22,8 +22,6 @@ #define __set_bit(n,p) set_bit(n,p) #define __clear_bit(n,p) clear_bit(n,p) -#define BITS_PER_BYTE 8 - #define ADDR (*(volatile int *) addr) #define CONST_ADDR (*(const volatile int *) addr) @@ -75,7 +73,7 @@ bool clear_mask16_timeout(uint16_t mask, volatile void *p, #define arch_ffs(x) ((x) ? 1 + __builtin_ctz(x) : 0) #define arch_ffsl(x) ((x) ? 1 + __builtin_ctzl(x) : 0) -#define arch_fls(x) ((x) ? 32 - __builtin_clz(x) : 0) +#define arch_fls(x) ((x) ? BITS_PER_INT - __builtin_clz(x) : 0) #define arch_flsl(x) ((x) ? BITS_PER_LONG - __builtin_clzl(x) : 0) #endif /* _ARM_BITOPS_H */ diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index 0a51142efd..5a02db6937 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -8,19 +8,11 @@ #define __ARM_CONFIG_H__ #if defined(CONFIG_ARM_64) -# define LONG_BYTEORDER 3 # define ELFSIZE 64 #else -# define LONG_BYTEORDER 2 # define ELFSIZE 32 #endif -#define BYTES_PER_LONG (1 << LONG_BYTEORDER) -#define BITS_PER_LONG (BYTES_PER_LONG << 3) -#define POINTER_ALIGN BYTES_PER_LONG - -#define BITS_PER_LLONG 64 - /* xen_ulong_t is always 64 bits */ #define BITS_PER_XEN_ULONG 64 diff --git a/xen/arch/ppc/include/asm/bitops.h b/xen/arch/ppc/include/asm/bitops.h index c942e9432e..e72942cca0 100644 --- a/xen/arch/ppc/include/asm/bitops.h +++ b/xen/arch/ppc/include/asm/bitops.h @@ -15,8 +15,6 @@ #define __set_bit(n, p) set_bit(n, p) #define __clear_bit(n, p) clear_bit(n, p) -#define BITS_PER_BYTE 8 - /* PPC bit number conversion */ #define PPC_BITLSHIFT(be) (BITS_PER_LONG - 1 - (be)) #define PPC_BIT(bit) (1UL << PPC_BITLSHIFT(bit)) @@ -121,7 +119,7 @@ static inline int test_and_set_bit(unsigned int nr, volatile void *addr) #define arch_ffs(x) ((x) ? 1 + __builtin_ctz(x) : 0) #define arch_ffsl(x) ((x) ? 1 + __builtin_ctzl(x) : 0) -#define arch_fls(x) ((x) ? 32 - __builtin_clz(x) : 0) +#define arch_fls(x) ((x) ? BITS_PER_INT - __builtin_clz(x) : 0) #define arch_flsl(x) ((x) ? BITS_PER_LONG - __builtin_clzl(x) : 0) #define arch_hweightl(x) __builtin_popcountl(x) diff --git a/xen/arch/ppc/include/asm/config.h b/xen/arch/ppc/include/asm/config.h index 148fb3074d..8e32edd5a5 100644 --- a/xen/arch/ppc/include/asm/config.h +++ b/xen/arch/ppc/include/asm/config.h @@ -6,19 +6,12 @@ #include <xen/page-size.h> #if defined(CONFIG_PPC64) -#define LONG_BYTEORDER 3 #define ELFSIZE 64 #define MAX_VIRT_CPUS 1024u #else #error "Unsupported PowerPC variant" #endif -#define BYTES_PER_LONG (1 << LONG_BYTEORDER) -#define BITS_PER_LONG (BYTES_PER_LONG << 3) -#define POINTER_ALIGN BYTES_PER_LONG - -#define BITS_PER_LLONG 64 - /* xen_ulong_t is always 64 bits */ #define BITS_PER_XEN_ULONG 64 diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h index 7141bd9e46..314c97c20a 100644 --- a/xen/arch/riscv/include/asm/config.h +++ b/xen/arch/riscv/include/asm/config.h @@ -119,25 +119,12 @@ #define HYPERVISOR_VIRT_START XEN_VIRT_START #if defined(CONFIG_RISCV_64) -# define INT_BYTEORDER 2 -# define LONG_BYTEORDER 3 # define ELFSIZE 64 # define MAX_VIRT_CPUS 128u #else # error "Unsupported RISCV variant" #endif -#define BYTES_PER_INT (1 << INT_BYTEORDER) -#define BITS_PER_INT (BYTES_PER_INT << 3) - -#define BYTES_PER_LONG (1 << LONG_BYTEORDER) -#define BITS_PER_LONG (BYTES_PER_LONG << 3) -#define POINTER_ALIGN BYTES_PER_LONG - -#define BITS_PER_LLONG 64 - -#define BITS_PER_BYTE 8 - /* xen_ulong_t is always 64 bits */ #define BITS_PER_XEN_ULONG 64 diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h index 19746f956e..f0123a7de9 100644 --- a/xen/arch/x86/include/asm/config.h +++ b/xen/arch/x86/include/asm/config.h @@ -7,16 +7,8 @@ #ifndef __X86_CONFIG_H__ #define __X86_CONFIG_H__ -#define LONG_BYTEORDER 3 #define CONFIG_PAGING_LEVELS 4 -#define BYTES_PER_LONG (1 << LONG_BYTEORDER) -#define BITS_PER_LONG (BYTES_PER_LONG << 3) -#define BITS_PER_BYTE 8 -#define POINTER_ALIGN BYTES_PER_LONG - -#define BITS_PER_LLONG 64 - #define BITS_PER_XEN_ULONG BITS_PER_LONG #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1 diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h index d888b2314d..7d43159efb 100644 --- a/xen/include/xen/config.h +++ b/xen/include/xen/config.h @@ -98,4 +98,14 @@ #define ZERO_BLOCK_PTR ((void *)-1L) #endif +#define BYTES_PER_LONG __SIZEOF_LONG__ + +#define BITS_PER_BYTE __CHAR_BIT__ +#define BITS_PER_INT (BITS_PER_BYTE * __SIZEOF_INT__) +#define BITS_PER_LONG (BITS_PER_BYTE * BYTES_PER_LONG) +#define BITS_PER_LLONG (BITS_PER_BYTE * __SIZEOF_LONG_LONG__) + +/* It is assumed that sizeof(void *) == __alignof(void *) */ +#define POINTER_ALIGN __SIZEOF_POINTER__ + #endif /* __XEN_CONFIG_H__ */
BITS_PER_* values can be defined in a common way using compiler-provided macros. Thus, these definitions are moved to xen/config.h to reduce duplication across architectures. Additionally, *_BYTEORDER macros are removed, as BITS_PER_* values now come directly from the compiler environment. The arch_fls() implementation for Arm and PPC is updated to use BITS_PER_INT instead of a hardcoded value of 32. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- GitLab CI tests results: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/1738505446 As Anrew mentioned here https://lore.kernel.org/xen-devel/f294e2b1-db03-46b2-b46d-61e89b55cef3@suse.com/T/#ma4205392964ff913d9dfa8e044a4af59ed6aef88: "This patch possibly wants to wait until I've fixed the compiler checks to update to the new baseline, ..." The patch with compiler fixes is https://lore.kernel.org/xen-devel/f79117a2-1763-4458-862f-a5219b09989a@citrix.com/T/#mf79cf8c1f6f6a3661c49bac84c1c15bbaae7422d. --- Changes in v2: - Add the comment "It is assumed that sizeof(void *) == __alignof(void *)" above POINTER_ALIGN definition. - Replace "<< 3" with "BITS_PER_BYTE" in definitions of BITS_PER_{INT,LONG,LONG_LONG}. --- xen/arch/arm/include/asm/bitops.h | 4 +--- xen/arch/arm/include/asm/config.h | 8 -------- xen/arch/ppc/include/asm/bitops.h | 4 +--- xen/arch/ppc/include/asm/config.h | 7 ------- xen/arch/riscv/include/asm/config.h | 13 ------------- xen/arch/x86/include/asm/config.h | 8 -------- xen/include/xen/config.h | 10 ++++++++++ 7 files changed, 12 insertions(+), 42 deletions(-)