Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Thomas Gleixner]

On Sat, Mar 02 2024 at 12:37, Thomas Gleixner wrote:
> Bah. sparse is actually right. I completely missed the fact that this is
> an UP build which has:
>
> extern struct cpuinfo_x86	boot_cpu_data;
>
> #define cpu_info		boot_cpu_data
>
> So any access with this_cpu*(), per_cpu*() etc. is actually incorrect from
> sparse's point of view.
>
> From a compiler point of view it just works because __percpu dissolves
> and the whole thing produces correct code magically.
>
> Most places in x86 use cpu_data(cpu) to access per cpu data which is
> defined as per_cpu(cpu_info, cpu) for SMP and boot_cpu_info for UP.
>
> That's fine, but there are places like the MCE code which really needs
> raw_cpu_ptr(). Sure we can write ugly wrappers for that and for some
> other accessors. But that's all just wrong and ugly.
>
> The proper solution would be to force SMP for x86, but Linus shot it
> down when I wanted to do that last time.
>
> Let me think about it.

The below addresses _all_ percpu related sparse warnings except
the ones in arch/x86/cpu/bugs.o but that's a sparse problem:

   The following is handled correctly:

	DECLARE_PER_CPU(u64, foo);
	this_cpu_read(foo);

   But this is not:

	DECLARE_PER_CPU(u64, foo);
	DEFINE_PER_CPU(u64, foo);
	this_cpu_read(foo);

arch/x86/kernel/cpu/bugs.c:71:9: sparse: warning: incorrect type in initializer (different address spaces)
arch/x86/kernel/cpu/bugs.c:71:9: sparse:    expected void const [noderef] __percpu *__vpp_verify
arch/x86/kernel/cpu/bugs.c:71:9: sparse:    got unsigned long long *

Commenting out the DEFINE_PER_CPU(u64, x86_spec_ctrl_current) in that
file makes sparse happy, but that's obviously not a solution :)

This problem is unrelated to the UP cpu_info issue, which made me look
at this mess in the first place. It happens on SMP too and both on 32
and 64 bit.

The other __percpu related sparse warnings are valid.

  - The UP cpu_info mechanics are just a horrible hackery.

    The cure is to "waste" one 'struct cpu_info' size of memory and
    provide the per CPU cpu_info in the same way as on SMP with
    DEFINE_PER_CPU() and copy the boot_cpu_data over at the same point
    in the boot process.

    With that the unholy #define hack goes away and _all_ per CPU
    accessors can now be used. That allows to get rid of the cpu_data()
    indirection which is just annoying for SMP because it creates
    suboptimal code.

  - smp-msr and amd uncore lack __percpu annotations in data structures
    and function arguments. That's not UP specific and just plain wrong.

While at it I fixed also the valid_user_address() complaint which lacks
a __force in the type cast.

The UP muck is only compiled and not boot tested. There might be a few
things which need to be adjusted, but from a quick scan I did not see
anything obvious.

I'll go and split it up into reviewable chunks and actually test UP
unless someone beats me to it and is brave enough to give the below a
test ride.

Thanks,

        tglx
---
 arch/alpha/kernel/smp.c           |    5 -----
 arch/arc/kernel/smp.c             |    5 -----
 arch/csky/kernel/smp.c            |    4 ----
 arch/hexagon/kernel/smp.c         |    4 ----
 arch/openrisc/kernel/smp.c        |    4 ----
 arch/riscv/kernel/smpboot.c       |    4 ----
 arch/sparc/kernel/smp_64.c        |    4 ----
 arch/x86/events/amd/uncore.c      |    2 +-
 arch/x86/include/asm/desc.h       |    4 ++--
 arch/x86/include/asm/msr.h        |   20 ++++++++++----------
 arch/x86/include/asm/processor.h  |    5 -----
 arch/x86/include/asm/smp.h        |    5 -----
 arch/x86/include/asm/uaccess_64.h |    2 +-
 arch/x86/kernel/setup.c           |   13 +++++++++++++
 arch/x86/kernel/smpboot.c         |    5 +++++
 arch/x86/lib/msr-smp.c            |    9 ++++-----
 arch/x86/lib/msr.c                |    2 +-
 include/linux/smp.h               |   13 ++++++-------
 init/main.c                       |    4 ++++
 19 files changed, 47 insertions(+), 67 deletions(-)

--- a/arch/alpha/kernel/smp.c
+++ b/arch/alpha/kernel/smp.c
@@ -467,11 +467,6 @@ smp_prepare_cpus(unsigned int max_cpus)
 	smp_num_cpus = smp_num_probed;
 }
 
-void
-smp_prepare_boot_cpu(void)
-{
-}
-
 int
 __cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
--- a/arch/arc/kernel/smp.c
+++ b/arch/arc/kernel/smp.c
@@ -39,11 +39,6 @@ struct plat_smp_ops  __weak plat_smp_ops
 /* XXX: per cpu ? Only needed once in early secondary boot */
 struct task_struct *secondary_idle_tsk;
 
-/* Called from start_kernel */
-void __init smp_prepare_boot_cpu(void)
-{
-}
-
 static int __init arc_get_cpu_map(const char *name, struct cpumask *cpumask)
 {
 	unsigned long dt_root = of_get_flat_dt_root();
--- a/arch/csky/kernel/smp.c
+++ b/arch/csky/kernel/smp.c
@@ -152,10 +152,6 @@ void arch_irq_work_raise(void)
 }
 #endif
 
-void __init smp_prepare_boot_cpu(void)
-{
-}
-
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
 }
--- a/arch/hexagon/kernel/smp.c
+++ b/arch/hexagon/kernel/smp.c
@@ -114,10 +114,6 @@ void send_ipi(const struct cpumask *cpum
 	local_irq_restore(flags);
 }
 
-void __init smp_prepare_boot_cpu(void)
-{
-}
-
 /*
  * interrupts should already be disabled from the VM
  * SP should already be correct; need to set THREADINFO_REG
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -57,10 +57,6 @@ static void boot_secondary(unsigned int
 	spin_unlock(&boot_lock);
 }
 
-void __init smp_prepare_boot_cpu(void)
-{
-}
-
 void __init smp_init_cpus(void)
 {
 	struct device_node *cpu;
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -42,10 +42,6 @@
 
 static DECLARE_COMPLETION(cpu_running);
 
-void __init smp_prepare_boot_cpu(void)
-{
-}
-
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
 	int cpuid;
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -1206,10 +1206,6 @@ void __init smp_prepare_cpus(unsigned in
 {
 }
 
-void smp_prepare_boot_cpu(void)
-{
-}
-
 void __init smp_setup_processor_id(void)
 {
 	if (tlb_type == spitfire)
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -71,7 +71,7 @@ union amd_uncore_info {
 };
 
 struct amd_uncore {
-	union amd_uncore_info * __percpu info;
+	union amd_uncore_info  __percpu *info;
 	struct amd_uncore_pmu *pmus;
 	unsigned int num_pmus;
 	bool init_done;
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -51,13 +51,13 @@ DECLARE_INIT_PER_CPU(gdt_page);
 /* Provide the original GDT */
 static inline struct desc_struct *get_cpu_gdt_rw(unsigned int cpu)
 {
-	return per_cpu(gdt_page, cpu).gdt;
+	return per_cpu(gdt_page.gdt, cpu);
 }
 
 /* Provide the current original GDT */
 static inline struct desc_struct *get_current_gdt_rw(void)
 {
-	return this_cpu_ptr(&gdt_page)->gdt;
+	return this_cpu_ptr(gdt_page.gdt);
 }
 
 /* Provide the fixmap address of the remapped GDT */
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -13,10 +13,10 @@
 #include <asm/shared/msr.h>
 
 struct msr_info {
-	u32 msr_no;
-	struct msr reg;
-	struct msr *msrs;
-	int err;
+	u32			msr_no;
+	struct msr		reg;
+	struct msr __percpu	*msrs;
+	int			err;
 };
 
 struct msr_regs_info {
@@ -315,8 +315,8 @@ int rdmsr_on_cpu(unsigned int cpu, u32 m
 int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
 int rdmsrl_on_cpu(unsigned int cpu, u32 msr_no, u64 *q);
 int wrmsrl_on_cpu(unsigned int cpu, u32 msr_no, u64 q);
-void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs);
-void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs);
+void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr __percpu *msrs);
+void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr __percpu *msrs);
 int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
 int wrmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
 int rdmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 *q);
@@ -345,14 +345,14 @@ static inline int wrmsrl_on_cpu(unsigned
 	return 0;
 }
 static inline void rdmsr_on_cpus(const struct cpumask *m, u32 msr_no,
-				struct msr *msrs)
+				struct msr __percpu *msrs)
 {
-	rdmsr_on_cpu(0, msr_no, &(msrs[0].l), &(msrs[0].h));
+	rdmsr_on_cpu(0, msr_no, this_cpu_ptr(&msrs->l), this_cpu_ptr(&msrs->h));
 }
 static inline void wrmsr_on_cpus(const struct cpumask *m, u32 msr_no,
-				struct msr *msrs)
+				struct msr __percpu *msrs)
 {
-	wrmsr_on_cpu(0, msr_no, msrs[0].l, msrs[0].h);
+	wrmsr_on_cpu(0, msr_no, this_cpu_read(msrs->l), this_cpu_read(msrs->h));
 }
 static inline int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no,
 				    u32 *l, u32 *h)
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -186,13 +186,8 @@ extern struct cpuinfo_x86	new_cpu_data;
 extern __u32			cpu_caps_cleared[NCAPINTS + NBUGINTS];
 extern __u32			cpu_caps_set[NCAPINTS + NBUGINTS];
 
-#ifdef CONFIG_SMP
 DECLARE_PER_CPU_READ_MOSTLY(struct cpuinfo_x86, cpu_info);
 #define cpu_data(cpu)		per_cpu(cpu_info, cpu)
-#else
-#define cpu_info		boot_cpu_data
-#define cpu_data(cpu)		boot_cpu_data
-#endif
 
 extern const struct seq_operations cpuinfo_op;
 
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -59,11 +59,6 @@ static inline void stop_other_cpus(void)
 	smp_ops.stop_other_cpus(1);
 }
 
-static inline void smp_prepare_boot_cpu(void)
-{
-	smp_ops.smp_prepare_boot_cpu();
-}
-
 static inline void smp_prepare_cpus(unsigned int max_cpus)
 {
 	smp_ops.smp_prepare_cpus(max_cpus);
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -54,7 +54,7 @@ static inline unsigned long __untagged_a
  * half and a user half.  When cast to a signed type, user pointers
  * are positive and kernel pointers are negative.
  */
-#define valid_user_address(x) ((long)(x) >= 0)
+#define valid_user_address(x) ((__force long)(x) >= 0)
 
 /*
  * User pointers can have tag bits on x86-64.  This scheme tolerates
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1211,6 +1211,19 @@ void __init i386_reserve_resources(void)
 
 #endif /* CONFIG_X86_32 */
 
+#ifndef CONFIG_SMP
+DEFINE_PER_CPU_READ_MOSTLY(struct cpuinfo_x86, cpu_info);
+EXPORT_PER_CPU_SYMBOL(cpu_info);
+
+void __init smp_prepare_boot_cpu(void)
+{
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	*c = boot_cpu_data;
+	c->initialized = true;
+}
+#endif
+
 static struct notifier_block kernel_offset_notifier = {
 	.notifier_call = dump_kernel_offset
 };
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1187,6 +1187,11 @@ void __init smp_prepare_cpus_common(void
 	set_cpu_sibling_map(0);
 }
 
+void __init smp_prepare_boot_cpu(void)
+{
+	smp_ops.smp_prepare_boot_cpu();
+}
+
 #ifdef CONFIG_X86_64
 /* Establish whether parallel bringup can be supported. */
 bool __init arch_cpuhp_init_parallel_bringup(void)
--- a/arch/x86/lib/msr-smp.c
+++ b/arch/x86/lib/msr-smp.c
@@ -9,10 +9,9 @@ static void __rdmsr_on_cpu(void *info)
 {
 	struct msr_info *rv = info;
 	struct msr *reg;
-	int this_cpu = raw_smp_processor_id();
 
 	if (rv->msrs)
-		reg = per_cpu_ptr(rv->msrs, this_cpu);
+		reg = this_cpu_ptr(rv->msrs);
 	else
 		reg = &rv->reg;
 
@@ -97,7 +96,7 @@ int wrmsrl_on_cpu(unsigned int cpu, u32
 EXPORT_SYMBOL(wrmsrl_on_cpu);
 
 static void __rwmsr_on_cpus(const struct cpumask *mask, u32 msr_no,
-			    struct msr *msrs,
+			    struct msr __percpu *msrs,
 			    void (*msr_func) (void *info))
 {
 	struct msr_info rv;
@@ -124,7 +123,7 @@ static void __rwmsr_on_cpus(const struct
  * @msrs:       array of MSR values
  *
  */
-void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs)
+void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr __percpu *msrs)
 {
 	__rwmsr_on_cpus(mask, msr_no, msrs, __rdmsr_on_cpu);
 }
@@ -138,7 +137,7 @@ EXPORT_SYMBOL(rdmsr_on_cpus);
  * @msrs:       array of MSR values
  *
  */
-void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr *msrs)
+void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr __percpu *msrs)
 {
 	__rwmsr_on_cpus(mask, msr_no, msrs, __wrmsr_on_cpu);
 }
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -8,7 +8,7 @@
 
 struct msr *msrs_alloc(void)
 {
-	struct msr *msrs = NULL;
+	struct msr __percpu *msrs = NULL;
 
 	msrs = alloc_percpu(struct msr);
 	if (!msrs) {
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -105,6 +105,12 @@ static inline void on_each_cpu_cond(smp_
 	on_each_cpu_cond_mask(cond_func, func, info, wait, cpu_online_mask);
 }
 
+/*
+ * Architecture specific boot CPU setup.  Defined as empty weak function in
+ * init/main.c. Architectures can override it.
+ */
+void smp_prepare_boot_cpu(void);
+
 #ifdef CONFIG_SMP
 
 #include <linux/preempt.h>
@@ -171,12 +177,6 @@ void generic_smp_call_function_single_in
 #define generic_smp_call_function_interrupt \
 	generic_smp_call_function_single_interrupt
 
-/*
- * Mark the boot cpu "online" so that it can call console drivers in
- * printk() and can access its per-cpu storage.
- */
-void smp_prepare_boot_cpu(void);
-
 extern unsigned int setup_max_cpus;
 extern void __init setup_nr_cpu_ids(void);
 extern void __init smp_init(void);
@@ -203,7 +203,6 @@ static inline void up_smp_call_function(
 			(up_smp_call_function(func, info))
 
 static inline void smp_send_reschedule(int cpu) { }
-#define smp_prepare_boot_cpu()			do {} while (0)
 #define smp_call_function_many(mask, func, info, wait) \
 			(up_smp_call_function(func, info))
 static inline void call_function_init(void) { }
--- a/init/main.c
+++ b/init/main.c
@@ -776,6 +776,10 @@ void __init __weak smp_setup_processor_i
 {
 }
 
+void __init __weak smp_prepare_boot_cpu(void)
+{
+}
+
 # if THREAD_SIZE >= PAGE_SIZE
 void __init __weak thread_stack_cache_init(void)
 {

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Thomas Gleixner]

On Sat, Mar 02 2024 at 16:44, Thomas Gleixner wrote:
> On Sat, Mar 02 2024 at 12:37, Thomas Gleixner wrote:
> The below addresses _all_ percpu related sparse warnings except
> the ones in arch/x86/cpu/bugs.o but that's a sparse problem:
>
>    The following is handled correctly:
>
> 	DECLARE_PER_CPU(u64, foo);
> 	this_cpu_read(foo);
>
>    But this is not:
>
> 	DECLARE_PER_CPU(u64, foo);
> 	DEFINE_PER_CPU(u64, foo);
> 	this_cpu_read(foo);
>
> arch/x86/kernel/cpu/bugs.c:71:9: sparse: warning: incorrect type in initializer (different address spaces)
> arch/x86/kernel/cpu/bugs.c:71:9: sparse:    expected void const [noderef] __percpu *__vpp_verify
> arch/x86/kernel/cpu/bugs.c:71:9: sparse:    got unsigned long long *
>
> Commenting out the DEFINE_PER_CPU(u64, x86_spec_ctrl_current) in that
> file makes sparse happy, but that's obviously not a solution :)

Correction. I found the real issue:

DEFINE_PER_CPU(u64, x86_spec_ctrl_current);
EXPORT_SYMBOL_GPL(x86_spec_ctrl_current);

I had commented out both. But the real reason is the EXPORT_SYMBOL,
which obviously wants to be EXPORT_PER_CPU_SYMBOL_GPL...

So sparse was right. Nothing to see here.

Thanks,

        tglx

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Linus Torvalds]

On Sat, 2 Mar 2024 at 14:00, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> I had commented out both. But the real reason is the EXPORT_SYMBOL,
> which obviously wants to be EXPORT_PER_CPU_SYMBOL_GPL...

Side note: while it's nice to hear that sparse kind of got this right,
I wonder what gcc does when we start using the named address spaces
for percpu variables.

We actively make EXPORT_PER_CPU_SYMBOL_XYZ be a no-op for sparse
exactly because sparse ended up warning about the regular
EXPORT_SYMBOL, and we didn't have any "real" per-cpu export model.

So EXPORT_PER_CPU_SYMBOL_GPL() is kind of an artificial "shut up
sparse". But with __seg_gs/fs support for native percpu symbols with
gcc, I wonder if we'll hit the same thing. Or is there something that
makes gcc not warn about the named address spaces?

Because in many ways the gcc named address spaces _should_ be pretty
much equivalent to the sparse ones.

            Linus

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Thomas Gleixner]

On Sat, Mar 02 2024 at 14:49, Linus Torvalds wrote:
> On Sat, 2 Mar 2024 at 14:00, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> I had commented out both. But the real reason is the EXPORT_SYMBOL,
>> which obviously wants to be EXPORT_PER_CPU_SYMBOL_GPL...
>
> Side note: while it's nice to hear that sparse kind of got this right,
> I wonder what gcc does when we start using the named address spaces
> for percpu variables.
>
> We actively make EXPORT_PER_CPU_SYMBOL_XYZ be a no-op for sparse
> exactly because sparse ended up warning about the regular
> EXPORT_SYMBOL, and we didn't have any "real" per-cpu export model.

Right.

> So EXPORT_PER_CPU_SYMBOL_GPL() is kind of an artificial "shut up
> sparse".

Aside of that it's also making it clear what this is about. So I don't
think it's purely artifical.

> But with __seg_gs/fs support for native percpu symbols with
> gcc, I wonder if we'll hit the same thing. Or is there something that
> makes gcc not warn about the named address spaces?

Right now the pending code in tip does not complain about the
EXPORT_PER_CPU_SYMBOL_GPL() part because our current macro maze is
hideous. Here is the preprocessor output.

This is DECLARE_PER_CPU() in the header:

extern __attribute__((section(".data..percpu" ""))) __typeof__(u64) x86_spec_ctrl_current;

Here is DEFINE_PER_CPU():

__attribute__((section(".data..percpu" ""))) __typeof__(u64) x86_spec_ctrl_current;

And the EXPORT:

extern typeof(x86_spec_ctrl_current) x86_spec_ctrl_current;

static void * __attribute__((__used__))
   __attribute__((__section__(".discard.addressable")))
   __UNIQUE_ID___addressable_x86_spec_ctrl_current804 = (void *)(uintptr_t)&x86_spec_ctrl_current;

   asm(".section \".export_symbol\",\"a\" ;
       __export_symbol_x86_spec_ctrl_current: ;
       .asciz \"GPL\" ; .asciz \"\" ; .balign 8 ; .quad x86_spec_ctrl_current ; .previous");

And the __seg_gs magic happens only in the per CPU accessor itself:

__attribute__((__noinline__)) __attribute__((no_instrument_function))
 __attribute((__section__(".noinstr.text")))
 __attribute__((__no_sanitize_address__))
 __attribute__((__no_profile_instrument_function__))
 u64 spec_ctrl_current(void)
{
 return ({
    // this_cpu_read(x86_spec_ctrl_current)

    typeof(x86_spec_ctrl_current) pscr_ret__;

    do { const void *__vpp_verify = (typeof((&(x86_spec_ctrl_current)) + 0))((void *)0); (void)__vpp_verify;
    } while (0);

    switch(sizeof(x86_spec_ctrl_current))
    {
    case 1: pscr_ret__ = ({
            *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
            break;
    case 2: pscr_ret__ = ({
            *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
            break;
    case 4: pscr_ret__ = ({
            *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
            break;
    case 8: pscr_ret__ = ({
            *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
            break;

    default: __bad_size_call_parameter(); break;
    }

    pscr_ret__;
  });
}

So all the export etc. just works because it all operates on a plain
data type and the __seg_gs is only bolted on via type casts in the
accessors.

As the per cpu variables are in the .data..percpu section the linker
puts them at address 0 and upwards. So the cast to a __seg_gs pointer
makes it end up at the real kernel address because of GSBASE + "offset".

The compiler converts this to RIP relative addressing:

  movq   $0x0,%gs:0x7e14169f(%rip)        # 1ba08 <fpu_fpregs_owner_ctx>

This obviously has a downside. If I do:

   u64 foo;

   this_cpu_read(foo);

the compiler is just happy to build that w/o complaining and it will
only explode at runtime because foo is a kernel data address which added
to GSBASE will result in accessing some random address:

  mov    %gs:0x15d08d4(%rip),%rax        # ffffffff834aac60 <x86_spec_ctrl_base>

This is not at all different from the inline ASM based version which is
in your tree. The only difference is that the macro maze is pure C and
the __set_gs cast allows the compiler to (micro) optimize, e.g. 'mov
%gs:...; movzbl' into a single 'movzbl'.

IOW, right now the only defense against such a mistake is actually the
sparse check. Maybe one of the coccinelle scripts has something similar,
I don't know.

I did not follow the __set_gs work closely, so I don't know whether Uros
ever tried to actually mark the per CPU variable __set_gs right away,
which would obviously catch the above 'foo' nonsense.

I think this should just work, but that would obviously require to do
the type cast magic at the EXPORT_SYMBOL side and in some other places.

Thanks,

        tglx

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

On Sun, Mar 3, 2024 at 5:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sat, Mar 02 2024 at 14:49, Linus Torvalds wrote:
> > On Sat, 2 Mar 2024 at 14:00, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> I had commented out both. But the real reason is the EXPORT_SYMBOL,
> >> which obviously wants to be EXPORT_PER_CPU_SYMBOL_GPL...
> >
> > Side note: while it's nice to hear that sparse kind of got this right,
> > I wonder what gcc does when we start using the named address spaces
> > for percpu variables.
> >
> > We actively make EXPORT_PER_CPU_SYMBOL_XYZ be a no-op for sparse
> > exactly because sparse ended up warning about the regular
> > EXPORT_SYMBOL, and we didn't have any "real" per-cpu export model.
>
> Right.
>
> > So EXPORT_PER_CPU_SYMBOL_GPL() is kind of an artificial "shut up
> > sparse".
>
> Aside of that it's also making it clear what this is about. So I don't
> think it's purely artifical.
>
> > But with __seg_gs/fs support for native percpu symbols with
> > gcc, I wonder if we'll hit the same thing. Or is there something that
> > makes gcc not warn about the named address spaces?
>
> Right now the pending code in tip does not complain about the
> EXPORT_PER_CPU_SYMBOL_GPL() part because our current macro maze is
> hideous. Here is the preprocessor output.
>
> This is DECLARE_PER_CPU() in the header:
>
> extern __attribute__((section(".data..percpu" ""))) __typeof__(u64) x86_spec_ctrl_current;
>
> Here is DEFINE_PER_CPU():
>
> __attribute__((section(".data..percpu" ""))) __typeof__(u64) x86_spec_ctrl_current;
>
> And the EXPORT:
>
> extern typeof(x86_spec_ctrl_current) x86_spec_ctrl_current;
>
> static void * __attribute__((__used__))
>    __attribute__((__section__(".discard.addressable")))
>    __UNIQUE_ID___addressable_x86_spec_ctrl_current804 = (void *)(uintptr_t)&x86_spec_ctrl_current;
>
>    asm(".section \".export_symbol\",\"a\" ;
>        __export_symbol_x86_spec_ctrl_current: ;
>        .asciz \"GPL\" ; .asciz \"\" ; .balign 8 ; .quad x86_spec_ctrl_current ; .previous");
>
> And the __seg_gs magic happens only in the per CPU accessor itself:
>
> __attribute__((__noinline__)) __attribute__((no_instrument_function))
>  __attribute((__section__(".noinstr.text")))
>  __attribute__((__no_sanitize_address__))
>  __attribute__((__no_profile_instrument_function__))
>  u64 spec_ctrl_current(void)
> {
>  return ({
>     // this_cpu_read(x86_spec_ctrl_current)
>
>     typeof(x86_spec_ctrl_current) pscr_ret__;
>
>     do { const void *__vpp_verify = (typeof((&(x86_spec_ctrl_current)) + 0))((void *)0); (void)__vpp_verify;
>     } while (0);
>
>     switch(sizeof(x86_spec_ctrl_current))
>     {
>     case 1: pscr_ret__ = ({
>             *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
>             break;
>     case 2: pscr_ret__ = ({
>             *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
>             break;
>     case 4: pscr_ret__ = ({
>             *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
>             break;
>     case 8: pscr_ret__ = ({
>             *(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
>             break;
>
>     default: __bad_size_call_parameter(); break;
>     }
>
>     pscr_ret__;
>   });
> }
>
> So all the export etc. just works because it all operates on a plain
> data type and the __seg_gs is only bolted on via type casts in the
> accessors.
>
> As the per cpu variables are in the .data..percpu section the linker
> puts them at address 0 and upwards. So the cast to a __seg_gs pointer
> makes it end up at the real kernel address because of GSBASE + "offset".
>
> The compiler converts this to RIP relative addressing:
>
>   movq   $0x0,%gs:0x7e14169f(%rip)        # 1ba08 <fpu_fpregs_owner_ctx>
>
> This obviously has a downside. If I do:
>
>    u64 foo;
>
>    this_cpu_read(foo);
>
> the compiler is just happy to build that w/o complaining and it will
> only explode at runtime because foo is a kernel data address which added
> to GSBASE will result in accessing some random address:
>
>   mov    %gs:0x15d08d4(%rip),%rax        # ffffffff834aac60 <x86_spec_ctrl_base>
>
> This is not at all different from the inline ASM based version which is
> in your tree. The only difference is that the macro maze is pure C and
> the __set_gs cast allows the compiler to (micro) optimize, e.g. 'mov
> %gs:...; movzbl' into a single 'movzbl'.
>
> IOW, right now the only defense against such a mistake is actually the
> sparse check. Maybe one of the coccinelle scripts has something similar,
> I don't know.
>
> I did not follow the __set_gs work closely, so I don't know whether Uros
> ever tried to actually mark the per CPU variable __set_gs right away,
> which would obviously catch the above 'foo' nonsense.

No, because [1]:

"gcc does not provide a way to remove segment qualifiers, which is needed
to use typeof() to create local instances of the per-cpu variable. For
this reason, do not use the segment qualifier for per-cpu variables, and
do casting using the segment qualifier instead."

[1] https://lore.kernel.org/lkml/20190823224424.15296-3-namit@vmware.com/

Uros.

>
> I think this should just work, but that would obviously require to do
> the type cast magic at the EXPORT_SYMBOL side and in some other places.
>
> Thanks,
>
>         tglx
>
>

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Thomas Gleixner]

On Sun, Mar 03 2024 at 20:03, Uros Bizjak wrote:
> On Sun, Mar 3, 2024 at 5:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> I did not follow the __set_gs work closely, so I don't know whether Uros
>> ever tried to actually mark the per CPU variable __set_gs right away,
>> which would obviously catch the above 'foo' nonsense.
>
> No, because [1]:
>
> "gcc does not provide a way to remove segment qualifiers, which is needed
> to use typeof() to create local instances of the per-cpu variable. For
> this reason, do not use the segment qualifier for per-cpu variables, and
> do casting using the segment qualifier instead."

Right. I just figured that out myself when playing with it in user
space.

That's so sad because it would provide us compiler based __percpu
validation.

Right now this simply does not work and __verify_pcp_ptr(ptr) is not
doing anything except when sparse looks at it.

Sigh.

        tglx

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Mar 03 2024 at 20:03, Uros Bizjak wrote:
> > On Sun, Mar 3, 2024 at 5:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> I did not follow the __set_gs work closely, so I don't know whether Uros
> >> ever tried to actually mark the per CPU variable __set_gs right away,
> >> which would obviously catch the above 'foo' nonsense.
> >
> > No, because [1]:
> >
> > "gcc does not provide a way to remove segment qualifiers, which is needed
> > to use typeof() to create local instances of the per-cpu variable. For
> > this reason, do not use the segment qualifier for per-cpu variables, and
> > do casting using the segment qualifier instead."
>
> Right. I just figured that out myself when playing with it in user
> space.
>
> That's so sad because it would provide us compiler based __percpu
> validation.

Unfortunately, the c compiler can't strip qualifiers, so typeof() is
of limited use also when const and volatile qualifiers are used.
Perhaps some extension could be introduced to c standard to provide an
unqualified type, e.g. typeof_unqual().

Uros.

> Right now this simply does not work and __verify_pcp_ptr(ptr) is not
> doing anything except when sparse looks at it.
>
> Sigh.
>
>         tglx
>

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Sun, Mar 03 2024 at 20:03, Uros Bizjak wrote:
> > > On Sun, Mar 3, 2024 at 5:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> I did not follow the __set_gs work closely, so I don't know whether Uros
> > >> ever tried to actually mark the per CPU variable __set_gs right away,
> > >> which would obviously catch the above 'foo' nonsense.
> > >
> > > No, because [1]:
> > >
> > > "gcc does not provide a way to remove segment qualifiers, which is needed
> > > to use typeof() to create local instances of the per-cpu variable. For
> > > this reason, do not use the segment qualifier for per-cpu variables, and
> > > do casting using the segment qualifier instead."
> >
> > Right. I just figured that out myself when playing with it in user
> > space.
> >
> > That's so sad because it would provide us compiler based __percpu
> > validation.
>
> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> of limited use also when const and volatile qualifiers are used.
> Perhaps some extension could be introduced to c standard to provide an
> unqualified type, e.g. typeof_unqual().

Oh, there is one in C23 [1].

[1] https://en.cppreference.com/w/c/language/typeof

Uros.

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

On Sun, Mar 3, 2024 at 9:24 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > On Sun, Mar 03 2024 at 20:03, Uros Bizjak wrote:
> > > > On Sun, Mar 3, 2024 at 5:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > > >> I did not follow the __set_gs work closely, so I don't know whether Uros
> > > >> ever tried to actually mark the per CPU variable __set_gs right away,
> > > >> which would obviously catch the above 'foo' nonsense.
> > > >
> > > > No, because [1]:
> > > >
> > > > "gcc does not provide a way to remove segment qualifiers, which is needed
> > > > to use typeof() to create local instances of the per-cpu variable. For
> > > > this reason, do not use the segment qualifier for per-cpu variables, and
> > > > do casting using the segment qualifier instead."
> > >
> > > Right. I just figured that out myself when playing with it in user
> > > space.
> > >
> > > That's so sad because it would provide us compiler based __percpu
> > > validation.
> >
> > Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> > of limited use also when const and volatile qualifiers are used.
> > Perhaps some extension could be introduced to c standard to provide an
> > unqualified type, e.g. typeof_unqual().
>
> Oh, there is one in C23 [1].
>
> [1] https://en.cppreference.com/w/c/language/typeof

FYI: gcc-14 compiles this testcase:

--cut here--
__seg_gs int a;
__typeof_unqual__(a) b;

int foo (void)
{
  return a;
}

int bar (void)
{
  return b;
}
--cut here--

to (gcc -O2):

foo:
       movl    %gs:a(%rip), %eax
       ret

bar:
       movl    b(%rip), %eax
       ret

So, it *does* strip the __seg_gs qualifier here.

Uros.

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Thomas Gleixner]

On Sun, Mar 03 2024 at 21:24, Uros Bizjak wrote:
> On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> > That's so sad because it would provide us compiler based __percpu
>> > validation.
>>
>> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
>> of limited use also when const and volatile qualifiers are used.
>> Perhaps some extension could be introduced to c standard to provide an
>> unqualified type, e.g. typeof_unqual().
>
> Oh, there is one in C23 [1].

Yes. I found it right after ranting.

gcc >= 14 and clang >= 16 have support for it of course only when adding
-std=c2x to the command line.

Sigh. The name space qualifiers are non standard and then the thing
which makes them more useful is hidden behind a standard.

Why can't we have useful tools?

Though the whole thing looks worthwhile:

#define verify_per_cpu_ptr(ptr)						\
do {									\
	const void __seg_gs *__vpp_verify = (typeof((ptr) + 0))NULL;    \
	(void)__vpp_verify;						\
} while (0)

#define per_cpu_ptr(ptr, cpu)						\
({									\
	verify_per_cpu_ptr(ptr);					\
	(typeof_unqual(*(ptr)) *)(uintptr_t)ptr + per_cpu_offset(cpu);	\
})

unsigned int __seg_gs test;

unsigned int foo1(unsigned int cpu)
{
	return *per_cpu_ptr(&test, cpu);
}

unsigned int foo2(unsigned int cpu)
{
	unsigned int x, *p = per_cpu_ptr(&x, cpu);

	return *p;
}

x.c:29:23: error: initializing 'const __attribute__((address_space(256))) void *' with an expression of type 'typeof ((&x) + 0)' (aka 'unsigned int *') changes address space of pointer
        unsigned int x, *p = per_cpu_ptr(&x, cpu);

That's exactly what we want. It would have caught all the long standing
and ignored __percpu sparse warnings right away.

This also simplifies all the other per cpu accessors. The most trivial
is read()

#define verify_per_cpu(variable)					\
{									\
	const unsigned int __s = sizeof(variable);			\
									\
	verify_per_cpu_ptr(&(variable));				\
	BUILD_BUG_ON(__s == 1 || __s == 2 || __s == 4 || __s == 8,	\
		     "Wrong size for per CPU variable");		\
}

#define __pcpu_read(variable)						\
({									\
	verify_per_cpu(variable);					\
	READ_ONCE(variable);						\
})

which in turn catches all the mistakes, i.e. wrong namespace and wrong
size.

I'm really tempted to implement this as an alternative to the current
pile of macro horrors. Of course this requires to figure out first what
kind of damage -std=c2x will do.

I get to that in my copious spare time some day.

Thanks,

        tglx

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Mar 03 2024 at 21:24, Uros Bizjak wrote:
> > On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >> On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > That's so sad because it would provide us compiler based __percpu
> >> > validation.
> >>
> >> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> >> of limited use also when const and volatile qualifiers are used.
> >> Perhaps some extension could be introduced to c standard to provide an
> >> unqualified type, e.g. typeof_unqual().
> >
> > Oh, there is one in C23 [1].
>
> Yes. I found it right after ranting.
>
> gcc >= 14 and clang >= 16 have support for it of course only when adding
> -std=c2x to the command line.
>
> Sigh. The name space qualifiers are non standard and then the thing
> which makes them more useful is hidden behind a standard.

With GCC, you can use __typeof_unqual__ (please note underscores)
without -std=c2x [1]:

"... Alternate spelling __typeof_unqual__ is available in all C modes
and provides non-atomic unqualified version of what __typeof__
operator returns..."

Please also see the example in my last post. It can be compiled without -std=...

[1] https://gcc.gnu.org/onlinedocs/gcc/Typeof.html

Uros.

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Thomas Gleixner]

On Mon, Mar 04 2024 at 06:42, Uros Bizjak wrote:

> On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> On Sun, Mar 03 2024 at 21:24, Uros Bizjak wrote:
>> > On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>> >> On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> > That's so sad because it would provide us compiler based __percpu
>> >> > validation.
>> >>
>> >> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
>> >> of limited use also when const and volatile qualifiers are used.
>> >> Perhaps some extension could be introduced to c standard to provide an
>> >> unqualified type, e.g. typeof_unqual().
>> >
>> > Oh, there is one in C23 [1].
>>
>> Yes. I found it right after ranting.
>>
>> gcc >= 14 and clang >= 16 have support for it of course only when adding
>> -std=c2x to the command line.
>>
>> Sigh. The name space qualifiers are non standard and then the thing
>> which makes them more useful is hidden behind a standard.
>
> With GCC, you can use __typeof_unqual__ (please note underscores)
> without -std=c2x [1]:
>
> "... Alternate spelling __typeof_unqual__ is available in all C modes
> and provides non-atomic unqualified version of what __typeof__
> operator returns..."
>
> Please also see the example in my last post. It can be compiled without -std=...

With gcc >= 14. Not so with clang...

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

On Mon, Mar 4, 2024 at 8:07 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, Mar 04 2024 at 06:42, Uros Bizjak wrote:
>
> > On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> On Sun, Mar 03 2024 at 21:24, Uros Bizjak wrote:
> >> > On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >> >> On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >> > That's so sad because it would provide us compiler based __percpu
> >> >> > validation.
> >> >>
> >> >> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> >> >> of limited use also when const and volatile qualifiers are used.
> >> >> Perhaps some extension could be introduced to c standard to provide an
> >> >> unqualified type, e.g. typeof_unqual().
> >> >
> >> > Oh, there is one in C23 [1].
> >>
> >> Yes. I found it right after ranting.
> >>
> >> gcc >= 14 and clang >= 16 have support for it of course only when adding
> >> -std=c2x to the command line.
> >>
> >> Sigh. The name space qualifiers are non standard and then the thing
> >> which makes them more useful is hidden behind a standard.
> >
> > With GCC, you can use __typeof_unqual__ (please note underscores)
> > without -std=c2x [1]:
> >
> > "... Alternate spelling __typeof_unqual__ is available in all C modes
> > and provides non-atomic unqualified version of what __typeof__
> > operator returns..."
> >
> > Please also see the example in my last post. It can be compiled without -std=...
>
> With gcc >= 14. Not so with clang...

Please note that clang-17.0.6 currently fails to compile kernel with
named address spaces [1]. So perhaps kernel can use __typeof_unqual__
(available without -std=c2x) in the hope that clang implements
__typeof_unqual__ in one of its next releases, following the examples
of GCC [2] and MSVC[3].

[1] https://lore.kernel.org/lkml/20240320173758.GA3017166@dev-arch.thelio-3990X/
[2] https://gcc.gnu.org/onlinedocs/gcc/Typeof.html
[3] https://learn.microsoft.com/en-us/cpp/c-language/typeof-unqual-c?view=msvc-170

Uros.

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Nathan Chancellor]

On Tue, Apr 02, 2024 at 01:43:00PM +0200, Uros Bizjak wrote:
> On Mon, Mar 4, 2024 at 8:07 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Mon, Mar 04 2024 at 06:42, Uros Bizjak wrote:
> >
> > > On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >>
> > >> On Sun, Mar 03 2024 at 21:24, Uros Bizjak wrote:
> > >> > On Sun, Mar 3, 2024 at 9:21 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > >> >> On Sun, Mar 3, 2024 at 9:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> >> > That's so sad because it would provide us compiler based __percpu
> > >> >> > validation.
> > >> >>
> > >> >> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> > >> >> of limited use also when const and volatile qualifiers are used.
> > >> >> Perhaps some extension could be introduced to c standard to provide an
> > >> >> unqualified type, e.g. typeof_unqual().
> > >> >
> > >> > Oh, there is one in C23 [1].
> > >>
> > >> Yes. I found it right after ranting.
> > >>
> > >> gcc >= 14 and clang >= 16 have support for it of course only when adding
> > >> -std=c2x to the command line.
> > >>
> > >> Sigh. The name space qualifiers are non standard and then the thing
> > >> which makes them more useful is hidden behind a standard.
> > >
> > > With GCC, you can use __typeof_unqual__ (please note underscores)
> > > without -std=c2x [1]:
> > >
> > > "... Alternate spelling __typeof_unqual__ is available in all C modes
> > > and provides non-atomic unqualified version of what __typeof__
> > > operator returns..."
> > >
> > > Please also see the example in my last post. It can be compiled without -std=...
> >
> > With gcc >= 14. Not so with clang...
> 
> Please note that clang-17.0.6 currently fails to compile kernel with
> named address spaces [1]. So perhaps kernel can use __typeof_unqual__
> (available without -std=c2x) in the hope that clang implements
> __typeof_unqual__ in one of its next releases, following the examples
> of GCC [2] and MSVC[3].

This is now supported in clang 19.0.0 (main):

https://github.com/llvm/llvm-project/commit/cc308f60d41744b5920ec2e2e5b25e1273c8704b

I have inquired about applying this to the 18.x series, such that it
would either make 18.1.3 or 18.1.4, but that is still open for
discussion.

I think the error that I mentioned at [1] is resolved with using
__typeof_unqual__, I tested this diff, which is likely incorrect but
allows me to continue testing without that warning/error due to -Werror:

diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 20696df5d567..fc77c99d2e80 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -95,7 +95,7 @@
 
 #endif /* CONFIG_SMP */
 
-#define __my_cpu_type(var)	typeof(var) __percpu_seg_override
+#define __my_cpu_type(var)	__typeof_unqual__(var) __percpu_seg_override
 #define __my_cpu_ptr(ptr)	(__my_cpu_type(*ptr)*)(__force uintptr_t)(ptr)
 #define __my_cpu_var(var)	(*__my_cpu_ptr(&(var)))
 #define __percpu_arg(x)		__percpu_prefix "%" #x

However, I get a crash in LLVM's backend with that diff applied on top
of commit 034dd140a6d8 ("Merge branch into tip/master: 'x86/shstk'"),
which appears to be another tangential issue. I've filed
https://github.com/ClangBuiltLinux/linux/issues/2013 so that we don't
lose track of this.

Cheers,
Nathan

Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 3412 bytes --]

On Wed, Apr 3, 2024 at 7:57 PM Nathan Chancellor <nathan@kernel.org> wrote:

> > > > With GCC, you can use __typeof_unqual__ (please note underscores)
> > > > without -std=c2x [1]:
> > > >
> > > > "... Alternate spelling __typeof_unqual__ is available in all C modes
> > > > and provides non-atomic unqualified version of what __typeof__
> > > > operator returns..."
> > > >
> > > > Please also see the example in my last post. It can be compiled without -std=...
> > >
> > > With gcc >= 14. Not so with clang...
> >
> > Please note that clang-17.0.6 currently fails to compile kernel with
> > named address spaces [1]. So perhaps kernel can use __typeof_unqual__
> > (available without -std=c2x) in the hope that clang implements
> > __typeof_unqual__ in one of its next releases, following the examples
> > of GCC [2] and MSVC[3].
>
> This is now supported in clang 19.0.0 (main):
>
> https://github.com/llvm/llvm-project/commit/cc308f60d41744b5920ec2e2e5b25e1273c8704b
>
> I have inquired about applying this to the 18.x series, such that it
> would either make 18.1.3 or 18.1.4, but that is still open for
> discussion.
>
> I think the error that I mentioned at [1] is resolved with using
> __typeof_unqual__, I tested this diff, which is likely incorrect but
> allows me to continue testing without that warning/error due to -Werror:
>
> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
> index 20696df5d567..fc77c99d2e80 100644
> --- a/arch/x86/include/asm/percpu.h
> +++ b/arch/x86/include/asm/percpu.h
> @@ -95,7 +95,7 @@
>
>  #endif /* CONFIG_SMP */
>
> -#define __my_cpu_type(var)     typeof(var) __percpu_seg_override
> +#define __my_cpu_type(var)     __typeof_unqual__(var) __percpu_seg_override
>  #define __my_cpu_ptr(ptr)      (__my_cpu_type(*ptr)*)(__force uintptr_t)(ptr)
>  #define __my_cpu_var(var)      (*__my_cpu_ptr(&(var)))
>  #define __percpu_arg(x)                __percpu_prefix "%" #x

IMO, the above change is not correct. Currently, the percpu variables
still live in generic address space, and the above casts are used at
the usage site of the percpu variable to convert pointer from generic
to disjoint __seg_gs address space (please see [2], section 6.17.5).
The -Wduplicate-decl-specifier warning at [1] (if correct) perhaps
points to the percpu accessor chain. GCC does not care about duplicate
__seg_gs conversions (the operation is idempotent), but the issue
should be corrected nevertheless.

BTW: With the above approach we get all the benefits of named address
spaces, but *not* checks for invalid access between disjoint address
spaces. This check is currently done by sparse (this is the reason for
__force in the above cast chain), but the check is not enabled by
default. The proposed improvement would *define* the percpu variable
in __seg_gs named address space, so the compiler will error out with
"assignment/return from pointer to non-enclosed address space" when
invalid access is detected (please see attached testcase, should be
compiled with gcc-14 due to usage of __typeof_unqual__) or warn with
"cast to generic address space pointer from disjoint ‘__seg_gs’
address space pointer" with explicit cast.

[1] https://lore.kernel.org/lkml/20240320173758.GA3017166@dev-arch.thelio-3990X/
[2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

Uros.

[-- Attachment #2: seg.c --]
[-- Type: text/x-csrc, Size: 549 bytes --]

int __seg_gs var;

void foo1 (void)
{
  asm volatile ("# %0" :: "m" (var));
}

int foo2 (void)
{
  return var;
}

int __seg_gs *bar1 (void)
{
  return &var;
}

int *bar2 (void)
{
  // return &var; /* error */
  return (int *)&var;
}

int *bar3 (void)
{
  int *p;

  //  p = &var; /* error */
  p = (int *)&var;

  return p;
}

int __seg_gs *baz1 (void)
{
  typeof(var) *p; /* (__seg_gs int *) */

  p = &var;

  return p;
}

int *baz2 (void)
{
  __typeof_unqual__(var) *p; /* (int *) */

  // p = &var; /* error */
  p = (int *)&var;

  return p;
}

[RFC PATCH] Use x86 named address spaces to catch "sparse: incorrect type in initializer (different address spaces)" __percpu errors

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 5394 bytes --]

On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <tglx@linutronix.de> wrote:

> >> > That's so sad because it would provide us compiler based __percpu
> >> > validation.
> >>
> >> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> >> of limited use also when const and volatile qualifiers are used.
> >> Perhaps some extension could be introduced to c standard to provide an
> >> unqualified type, e.g. typeof_unqual().
> >
> > Oh, there is one in C23 [1].
>
> Yes. I found it right after ranting.
>
> gcc >= 14 and clang >= 16 have support for it of course only when adding
> -std=c2x to the command line.
>
> Sigh. The name space qualifiers are non standard and then the thing
> which makes them more useful is hidden behind a standard.
>
> Why can't we have useful tools?
>
> Though the whole thing looks worthwhile:
>
> #define verify_per_cpu_ptr(ptr)                                         \
> do {                                                                    \
>         const void __seg_gs *__vpp_verify = (typeof((ptr) + 0))NULL;    \
>         (void)__vpp_verify;                                             \
> } while (0)
>
> #define per_cpu_ptr(ptr, cpu)                                           \
> ({                                                                      \
>         verify_per_cpu_ptr(ptr);                                        \
>         (typeof_unqual(*(ptr)) *)(uintptr_t)ptr + per_cpu_offset(cpu);  \
> })
>
> unsigned int __seg_gs test;
>
> unsigned int foo1(unsigned int cpu)
> {
>         return *per_cpu_ptr(&test, cpu);
> }
>
> unsigned int foo2(unsigned int cpu)
> {
>         unsigned int x, *p = per_cpu_ptr(&x, cpu);
>
>         return *p;
> }
>
> x.c:29:23: error: initializing 'const __attribute__((address_space(256))) void *' with an expression of type 'typeof ((&x) + 0)' (aka 'unsigned int *') changes address space of pointer
>         unsigned int x, *p = per_cpu_ptr(&x, cpu);
>
> That's exactly what we want. It would have caught all the long standing
> and ignored __percpu sparse warnings right away.
>
> This also simplifies all the other per cpu accessors. The most trivial
> is read()
>
> #define verify_per_cpu(variable)                                        \
> {                                                                       \
>         const unsigned int __s = sizeof(variable);                      \
>                                                                         \
>         verify_per_cpu_ptr(&(variable));                                \
>         BUILD_BUG_ON(__s == 1 || __s == 2 || __s == 4 || __s == 8,      \
>                      "Wrong size for per CPU variable");                \
> }
>
> #define __pcpu_read(variable)                                           \
> ({                                                                      \
>         verify_per_cpu(variable);                                       \
>         READ_ONCE(variable);                                            \
> })
>
> which in turn catches all the mistakes, i.e. wrong namespace and wrong
> size.
>
> I'm really tempted to implement this as an alternative to the current
> pile of macro horrors. Of course this requires to figure out first what
> kind of damage -std=c2x will do.
>
> I get to that in my copious spare time some day.

Please find attached the prototype patch that does the above.

The idea of the patch is to add named address qualifier to the __percpu tag:

-# define __percpu    BTF_TYPE_TAG(percpu)
+# define __percpu    __percpu_seg_override BTF_TYPE_TAG(percpu)

So instead of being merely a benign hint to the checker, __percpu
becomes the real x86 named address space qualifier to enable the
compiler checks for access to different address spaces. Following the
above change, we can remove various casts that cast "fake" percpu
addresses at the usage site and use the kernel type system to handle
named AS qualified addresses instead:

-#define __my_cpu_type(var)    typeof(var) __percpu_seg_override
-#define __my_cpu_ptr(ptr)    (__my_cpu_type(*(ptr))*)(__force uintptr_t)(ptr)
-#define __my_cpu_var(var)    (*__my_cpu_ptr(&(var)))
-#define __percpu_arg(x)        __percpu_prefix "%" #x
+#define __my_cpu_type(var)    typeof(var)
+#define __my_cpu_ptr(ptr)    (ptr)
+#define __my_cpu_var(var)    (var)
+#define __percpu_arg(x)        "%" #x

As can be seen from the patch, various temporary non-percpu variables
need to be declared with __typeof_unqual__ to use unqualified base
type without named AS qualifier. In addition to the named AS
qualifier, __typeof_unqual__ also strips const and volatile
qualifiers, so it can enable some further optimizations involving
this_cpu_read_stable, not a topic of this patch.

The patch is against the recent -tip tree and needs to be compiled
with gcc-14. It is tested by compiling and booting the defconfig
kernel, but other than that, as a prototype patch, it does not even
try to be a generic patch that would handle compilers without
__typeof_unqual__ support. The patch unearths and fixes some address
space inconsistencies to avoid __verify_pcpu_ptr and x86 named address
space compile failures with a defconfig compilation, demonstrating the
effectiveness of the proposed approach.

Uros.

[-- Attachment #2: pcpu-unqual.diff.txt --]
[-- Type: text/plain, Size: 8716 bytes --]

diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 3bedee1801e2..d2505a47dc27 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -89,10 +89,10 @@
 
 #endif /* CONFIG_SMP */
 
-#define __my_cpu_type(var)	typeof(var) __percpu_seg_override
-#define __my_cpu_ptr(ptr)	(__my_cpu_type(*(ptr))*)(__force uintptr_t)(ptr)
-#define __my_cpu_var(var)	(*__my_cpu_ptr(&(var)))
-#define __percpu_arg(x)		__percpu_prefix "%" #x
+#define __my_cpu_type(var)	typeof(var)
+#define __my_cpu_ptr(ptr)	(ptr)
+#define __my_cpu_var(var)	(var)
+#define __percpu_arg(x)		"%" #x
 #define __force_percpu_arg(x)	__force_percpu_prefix "%" #x
 
 /*
@@ -148,7 +148,7 @@
 do {									\
 	__pcpu_type_##size pto_val__ = __pcpu_cast_##size(_val);	\
 	if (0) {		                                        \
-		typeof(_var) pto_tmp__;					\
+		__typeof_unqual__(_var) pto_tmp__;			\
 		pto_tmp__ = (_val);					\
 		(void)pto_tmp__;					\
 	}								\
@@ -173,7 +173,7 @@ do {									\
 			      ((val) == 1 || (val) == -1)) ?		\
 				(int)(val) : 0;				\
 	if (0) {							\
-		typeof(var) pao_tmp__;					\
+		__typeof_unqual__(var) pao_tmp__;			\
 		pao_tmp__ = (val);					\
 		(void)pao_tmp__;					\
 	}								\
@@ -223,7 +223,7 @@ do {									\
  */
 #define raw_percpu_xchg_op(_var, _nval)					\
 ({									\
-	typeof(_var) pxo_old__ = raw_cpu_read(_var);			\
+	__typeof_unqual__(_var) pxo_old__ = raw_cpu_read(_var);		\
 	raw_cpu_write(_var, _nval);					\
 	pxo_old__;							\
 })
@@ -235,7 +235,7 @@ do {									\
  */
 #define this_percpu_xchg_op(_var, _nval)				\
 ({									\
-	typeof(_var) pxo_old__ = this_cpu_read(_var);			\
+	__typeof_unqual__(_var) pxo_old__ = this_cpu_read(_var);	\
 	do { } while (!this_cpu_try_cmpxchg(_var, &pxo_old__, _nval));	\
 	pxo_old__;							\
 })
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index a817ed0724d1..f5d6ad351cc4 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -560,9 +560,10 @@ void early_setup_idt(void)
 void __head startup_64_setup_gdt_idt(void)
 {
 	void *handler = NULL;
+	struct desc_struct *gdt = (struct desc_struct *)(uintptr_t)init_per_cpu_var(gdt_page.gdt);
 
 	struct desc_ptr startup_gdt_descr = {
-		.address = (unsigned long)&RIP_REL_REF(init_per_cpu_var(gdt_page.gdt)),
+		.address = (unsigned long)&RIP_REL_REF(*gdt),
 		.size    = GDT_SIZE - 1,
 	};
 
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 3df0025d12aa..ae52721bc79e 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -1223,6 +1223,6 @@ EXPORT_SYMBOL_GPL(__devm_alloc_percpu);
 void devm_free_percpu(struct device *dev, void __percpu *pdata)
 {
 	WARN_ON(devres_destroy(dev, devm_percpu_release, devm_percpu_match,
-			       (__force void *)pdata));
+			       (__force void *)(uintptr_t)pdata));
 }
 EXPORT_SYMBOL_GPL(devm_free_percpu);
diff --git a/fs/aio.c b/fs/aio.c
index 0f4f531c9780..baba27450696 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -100,7 +100,7 @@ struct kioctx {
 
 	unsigned long		user_id;
 
-	struct __percpu kioctx_cpu *cpu;
+	struct kioctx_cpu __percpu *cpu;
 
 	/*
 	 * For percpu reqs_available, number of slots we move to/from global
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 2abaa3a825a9..7c574d686486 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -57,7 +57,7 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
 #  define __user	BTF_TYPE_TAG(user)
 # endif
 # define __iomem
-# define __percpu	BTF_TYPE_TAG(percpu)
+# define __percpu	__percpu_seg_override BTF_TYPE_TAG(percpu)
 # define __rcu		BTF_TYPE_TAG(rcu)
 
 # define __chk_user_ptr(x)	(void)0
diff --git a/include/linux/part_stat.h b/include/linux/part_stat.h
index abeba356bc3f..0e02e9d60114 100644
--- a/include/linux/part_stat.h
+++ b/include/linux/part_stat.h
@@ -33,7 +33,7 @@ struct disk_stats {
 
 #define part_stat_read(part, field)					\
 ({									\
-	typeof((part)->bd_stats->field) res = 0;			\
+	__typeof_unqual__((part)->bd_stats->field) res = 0;		\
 	unsigned int _cpu;						\
 	for_each_possible_cpu(_cpu)					\
 		res += per_cpu_ptr((part)->bd_stats, _cpu)->field; \
diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index ec3573119923..4cb667887c81 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -233,13 +233,13 @@ do {									\
 #define per_cpu_ptr(ptr, cpu)						\
 ({									\
 	__verify_pcpu_ptr(ptr);						\
-	SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu)));			\
+	(__typeof_unqual__(*(ptr)) *)(uintptr_t)SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \
 })
 
 #define raw_cpu_ptr(ptr)						\
 ({									\
 	__verify_pcpu_ptr(ptr);						\
-	arch_raw_cpu_ptr(ptr);						\
+	(__typeof_unqual__(*(ptr)) *)(uintptr_t)arch_raw_cpu_ptr(ptr);	\
 })
 
 #ifdef CONFIG_DEBUG_PREEMPT
@@ -315,7 +315,7 @@ static __always_inline void __this_cpu_preempt_check(const char *op) { }
 
 #define __pcpu_size_call_return(stem, variable)				\
 ({									\
-	typeof(variable) pscr_ret__;					\
+	__typeof_unqual__(variable) pscr_ret__;				\
 	__verify_pcpu_ptr(&(variable));					\
 	switch(sizeof(variable)) {					\
 	case 1: pscr_ret__ = stem##1(variable); break;			\
@@ -330,7 +330,7 @@ static __always_inline void __this_cpu_preempt_check(const char *op) { }
 
 #define __pcpu_size_call_return2(stem, variable, ...)			\
 ({									\
-	typeof(variable) pscr2_ret__;					\
+	__typeof_unqual__(variable) pscr2_ret__;			\
 	__verify_pcpu_ptr(&(variable));					\
 	switch(sizeof(variable)) {					\
 	case 1: pscr2_ret__ = stem##1(variable, __VA_ARGS__); break;	\
diff --git a/include/linux/prandom.h b/include/linux/prandom.h
index f7f1e5251c67..f2ed5b72b3d6 100644
--- a/include/linux/prandom.h
+++ b/include/linux/prandom.h
@@ -10,6 +10,7 @@
 
 #include <linux/types.h>
 #include <linux/once.h>
+#include <linux/percpu.h>
 #include <linux/random.h>
 
 struct rnd_state {
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6c2cb4e4f48d..d82fe78f0658 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -849,7 +849,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
 
 	cpu_events = alloc_percpu(typeof(*cpu_events));
 	if (!cpu_events)
-		return (void __percpu __force *)ERR_PTR(-ENOMEM);
+		return (void __percpu __force *)(uintptr_t)ERR_PTR(-ENOMEM);
 
 	cpus_read_lock();
 	for_each_online_cpu(cpu) {
@@ -868,7 +868,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
 		return cpu_events;
 
 	unregister_wide_hw_breakpoint(cpu_events);
-	return (void __percpu __force *)ERR_PTR(err);
+	return (void __percpu __force *)(uintptr_t)ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
 
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index 6083883c4fe0..1c8fca7e6fd6 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -184,7 +184,7 @@ EXPORT_SYMBOL_GPL(__percpu_down_read);
 
 #define per_cpu_sum(var)						\
 ({									\
-	typeof(var) __sum = 0;						\
+	__typeof_unqual__(var) __sum = 0;				\
 	int cpu;							\
 	compiletime_assert_atomic_type(__sum);				\
 	for_each_possible_cpu(cpu)					\
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f397510edc9b..7dd6392c9c52 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -377,7 +377,7 @@ struct workqueue_struct {
 
 	/* hot fields used during command issue, aligned to cacheline */
 	unsigned int		flags ____cacheline_aligned; /* WQ: WQ_* flags */
-	struct pool_workqueue __percpu __rcu **cpu_pwq; /* I: per-cpu pwqs */
+	struct pool_workqueue * __percpu __rcu *cpu_pwq; /* I: per-cpu pwqs */
 	struct wq_node_nr_active *node_nr_active[]; /* I: per-node nr_active */
 };
 
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 44dd133594d4..74f438f469d8 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -175,7 +175,7 @@ int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount,
 		INIT_LIST_HEAD(&fbc[i].list);
 #endif
 		fbc[i].count = amount;
-		fbc[i].counters = (void *)counters + (i * counter_size);
+		fbc[i].counters = (void __percpu *)counters + (i * counter_size);
 
 		debug_percpu_counter_activate(&fbc[i]);
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index 331848eca7d3..4dfc0ea92513 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10651,7 +10651,7 @@ noinline void netdev_core_stats_inc(struct net_device *dev, u32 offset)
 			return;
 	}
 
-	field = (__force unsigned long __percpu *)((__force void *)p + offset);
+	field = (unsigned long __percpu *)(void __percpu *)(p + offset);
 	this_cpu_inc(*field);
 }
 EXPORT_SYMBOL_GPL(netdev_core_stats_inc);