RE: [PATCH 1/1] x86/vdso: use the right GDT_ENTRY_CPUNODE for 32-bit getcpu() on 64-bit kernel

[Li, Xin3]

> > > > +#ifndef BUILD_VDSO32_64
> > > >  #define GDT_ENTRY_CPUNODE		28
> > > > +#else
> > > > +#define GDT_ENTRY_CPUNODE		15
> > > > +#endif
> > >
> > > Isn't this kinda a hack?
> > >
> > > First, it means that we'll now have two duplicate versions of this:
> > >
> > > 	#define GDT_ENTRY_CPUNODE		15
> > >
> > > in the same file.
> > >
> > > Second, if any other users of fake_32bit_build.h for the VDSO show
> > > up, they'll need a similar #ifdef.
> > >
> > > I think I'd much rather if we define all of the GDT_ENTRY_* macros
> > > in
> > > *one* place, then make that *one* place depend on BUILD_VDSO32_64.
> >
> > Sounds a better way, let me try.
> >
> > > Also, about the *silent* failure...  Do we not have a selftest for this somewhere?
> >
> > When lsl is used, we should check ZF which indicates whether the
> > segment limit is loaded successfully.  Seems we need to refactor
> vdso_read_cpunode() a bit.
> 
> Hi Dave,
> 
> How about the following patch to address the silent failure?
> 
> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
> index 794f69625780..d75ce4afff5b 100644
> --- a/arch/x86/include/asm/segment.h
> +++ b/arch/x86/include/asm/segment.h
> @@ -254,7 +254,10 @@ static inline void vdso_read_cpunode(unsigned *cpu,
> unsigned *node)
>          *
>          * If RDPID is available, use it.
>          */
> -       alternative_io ("lsl %[seg],%[p]",
> +       alternative_io ("lsl %[seg],%[p]\n"
> +                       "jz 1f\n"
> +                       "mov $-1,%[p]\n"
> +                       "1:",
>                         ".byte 0xf3,0x0f,0xc7,0xf8", /* RDPID %eax/rax */
>                         X86_FEATURE_RDPID,
>                         [p] "=a" (p), [seg] "r" (__CPUNODE_SEG));
> 
> 
> And the test then reports CPU id 4095 (not a big enough #), which can be used to
> indicate a failure of the lsl instruction:

Having to say, it's a *bad* idea to use a special # to indicate an error.
But there is also no reasonable errno for getcpu() to return to its caller,
saying "we had a problem in the syscall's kernel implementation".

> 
> ~/selftests$ sudo ./run_kselftest.sh -t x86:test_vsyscall_32 TAP version 13
> 1..1
> # selftests: x86: test_vsyscall_32
> # [RUN] test gettimeofday()
> #       vDSO time offsets: 0.000028 0.000000
> # [OK]  vDSO gettimeofday()'s timeval was okay # [RUN] test time() # [OK]  vDSO
> time() is okay # [RUN] getcpu() on CPU 0
> # [FAIL]        vDSO reported CPU 4095 but should be 0
> # [FAIL]        vDSO reported node 65535 but should be 0
> # [RUN] getcpu() on CPU 1
> # [FAIL]        vDSO reported CPU 4095 but should be 1
> # [FAIL]        vDSO reported node 65535 but should be 0
> not ok 1 selftests: x86: test_vsyscall_32 # exit=1
> 
> Thanks!
>   Xin