Messages in this thread | | | From | "Li, Xin3" <> | Subject | RE: [PATCH 1/1] x86/vdso: use the right GDT_ENTRY_CPUNODE for 32-bit getcpu() on 64-bit kernel | Date | Wed, 29 Mar 2023 23:11:09 +0000 |
| |
> > > > +#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
| |