| Message ID | 20220809060627.115847-2-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm/selftests: Two rseq_test fixes | expand |
* Gavin Shan: > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > index a54d4d05a058..acb1bf1f06b3 100644 > --- a/tools/testing/selftests/kvm/rseq_test.c > +++ b/tools/testing/selftests/kvm/rseq_test.c > @@ -9,6 +9,7 @@ > #include <string.h> > #include <signal.h> > #include <syscall.h> > +#include <dlfcn.h> > #include <sys/ioctl.h> > #include <sys/sysinfo.h> > #include <asm/barrier.h> I'm surprised that there isn't a Makefile update to link with -ldl (still required for glibc 2.33 and earlier). > @@ -36,6 +37,8 @@ static __thread volatile struct rseq __rseq = { > */ > #define NR_TASK_MIGRATIONS 100000 > > +static bool __rseq_ownership; > +static volatile struct rseq *__rseq_info; > static pthread_t migration_thread; > static cpu_set_t possible_mask; > static int min_cpu, max_cpu; > @@ -49,11 +52,33 @@ static void guest_code(void) > GUEST_SYNC(0); > } > > +static void sys_rseq_ownership(void) > +{ > + long *offset; > + unsigned int *size, *flags; > + > + offset = dlsym(RTLD_NEXT, "__rseq_offset"); > + size = dlsym(RTLD_NEXT, "__rseq_size"); > + flags = dlsym(RTLD_NEXT, "__rseq_flags"); > + > + if (offset && size && *size && flags) { > + __rseq_ownership = false; > + __rseq_info = (struct rseq *)((uintptr_t)__builtin_thread_pointer() + > + *offset); __builtin_thread_pointer doesn't work on all architectures/GCC versions. Is this a problem for selftests? Thanks, Florian
* Gavin Shan: >> __builtin_thread_pointer doesn't work on all architectures/GCC >> versions. >> Is this a problem for selftests? >> > > It's a problem as the test case is running on all architectures. I think I > need introduce our own __builtin_thread_pointer() for where it's not > supported: (1) PowerPC (2) x86 without GCC 11 > > Please let me know if I still have missed cases where > __buitin_thread_pointer() isn't supported? As far as I know, these are the two outliers that also have rseq support. The list is a bit longer if we also consider non-rseq architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know about the Linux architectures without glibc support). Thanks, Florian
Hi Florian, On 8/9/22 4:33 PM, Florian Weimer wrote: >> diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c >> index a54d4d05a058..acb1bf1f06b3 100644 >> --- a/tools/testing/selftests/kvm/rseq_test.c >> +++ b/tools/testing/selftests/kvm/rseq_test.c >> @@ -9,6 +9,7 @@ >> #include <string.h> >> #include <signal.h> >> #include <syscall.h> >> +#include <dlfcn.h> >> #include <sys/ioctl.h> >> #include <sys/sysinfo.h> >> #include <asm/barrier.h> > > I'm surprised that there isn't a Makefile update to link with -ldl > (still required for glibc 2.33 and earlier). > In next revision, I will add '-ldl' into tools/testing/selftests/kvm/Makefile. >> @@ -36,6 +37,8 @@ static __thread volatile struct rseq __rseq = { >> */ >> #define NR_TASK_MIGRATIONS 100000 >> >> +static bool __rseq_ownership; >> +static volatile struct rseq *__rseq_info; >> static pthread_t migration_thread; >> static cpu_set_t possible_mask; >> static int min_cpu, max_cpu; >> @@ -49,11 +52,33 @@ static void guest_code(void) >> GUEST_SYNC(0); >> } >> >> +static void sys_rseq_ownership(void) >> +{ >> + long *offset; >> + unsigned int *size, *flags; >> + >> + offset = dlsym(RTLD_NEXT, "__rseq_offset"); >> + size = dlsym(RTLD_NEXT, "__rseq_size"); >> + flags = dlsym(RTLD_NEXT, "__rseq_flags"); >> + >> + if (offset && size && *size && flags) { >> + __rseq_ownership = false; >> + __rseq_info = (struct rseq *)((uintptr_t)__builtin_thread_pointer() + >> + *offset); > > __builtin_thread_pointer doesn't work on all architectures/GCC versions. > Is this a problem for selftests? > It's a problem as the test case is running on all architectures. I think I need introduce our own __builtin_thread_pointer() for where it's not supported: (1) PowerPC (2) x86 without GCC 11 Please let me know if I still have missed cases where __buitin_thread_pointer() isn't supported? Thanks, Gavin
Hi Florian, On 8/9/22 5:16 PM, Florian Weimer wrote: >>> __builtin_thread_pointer doesn't work on all architectures/GCC >>> versions. >>> Is this a problem for selftests? >>> >> >> It's a problem as the test case is running on all architectures. I think I >> need introduce our own __builtin_thread_pointer() for where it's not >> supported: (1) PowerPC (2) x86 without GCC 11 >> >> Please let me know if I still have missed cases where >> __buitin_thread_pointer() isn't supported? > > As far as I know, these are the two outliers that also have rseq > support. The list is a bit longer if we also consider non-rseq > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know > about the Linux architectures without glibc support). > For kvm/selftests, there are 3 architectures involved actually. So we just need consider 4 cases: aarch64, x86, s390 and other. For other case, we just use __builtin_thread_pointer() to maintain code's integrity, but it's not called at all. I think kvm/selftest is always relying on glibc if I'm correct. Thanks, Gavin
----- Gavin Shan <gshan@redhat.com> wrote: > Hi Florian, > > On 8/9/22 5:16 PM, Florian Weimer wrote: > >>> __builtin_thread_pointer doesn't work on all architectures/GCC > >>> versions. > >>> Is this a problem for selftests? > >>> > >> > >> It's a problem as the test case is running on all architectures. I think I > >> need introduce our own __builtin_thread_pointer() for where it's not > >> supported: (1) PowerPC (2) x86 without GCC 11 > >> > >> Please let me know if I still have missed cases where > >> __buitin_thread_pointer() isn't supported? > > > > As far as I know, these are the two outliers that also have rseq > > support. The list is a bit longer if we also consider non-rseq > > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know > > about the Linux architectures without glibc support). > > > > For kvm/selftests, there are 3 architectures involved actually. So we > just need consider 4 cases: aarch64, x86, s390 and other. For other > case, we just use __builtin_thread_pointer() to maintain code's > integrity, but it's not called at all. > > I think kvm/selftest is always relying on glibc if I'm correct. All those are handled in the rseq selftests and in librseq. Why duplicate all that logic again? Thanks, Mathieu > > Thanks, > Gavin >
----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > ----- Gavin Shan <gshan@redhat.com> wrote: >> Hi Florian, >> >> On 8/9/22 5:16 PM, Florian Weimer wrote: >> >>> __builtin_thread_pointer doesn't work on all architectures/GCC >> >>> versions. >> >>> Is this a problem for selftests? >> >>> >> >> >> >> It's a problem as the test case is running on all architectures. I think I >> >> need introduce our own __builtin_thread_pointer() for where it's not >> >> supported: (1) PowerPC (2) x86 without GCC 11 >> >> >> >> Please let me know if I still have missed cases where >> >> __buitin_thread_pointer() isn't supported? >> > >> > As far as I know, these are the two outliers that also have rseq >> > support. The list is a bit longer if we also consider non-rseq >> > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >> > about the Linux architectures without glibc support). >> > >> >> For kvm/selftests, there are 3 architectures involved actually. So we >> just need consider 4 cases: aarch64, x86, s390 and other. For other >> case, we just use __builtin_thread_pointer() to maintain code's >> integrity, but it's not called at all. >> >> I think kvm/selftest is always relying on glibc if I'm correct. > > All those are handled in the rseq selftests and in librseq. Why duplicate all > that logic again? More to the point, considering that we have all the relevant rseq registration code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, is there an easy way to get test applications in tools/testing/selftests/kvm and in tools/testing/selftests/rseq to share that common code ? Keeping duplicated compatibility code is bad for long-term maintainability. Thanks, Mathieu
On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: > ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > > > ----- Gavin Shan <gshan@redhat.com> wrote: > >> Hi Florian, > >> > >> On 8/9/22 5:16 PM, Florian Weimer wrote: > >> >>> __builtin_thread_pointer doesn't work on all architectures/GCC > >> >>> versions. > >> >>> Is this a problem for selftests? > >> >>> > >> >> > >> >> It's a problem as the test case is running on all architectures. I think I > >> >> need introduce our own __builtin_thread_pointer() for where it's not > >> >> supported: (1) PowerPC (2) x86 without GCC 11 > >> >> > >> >> Please let me know if I still have missed cases where > >> >> __buitin_thread_pointer() isn't supported? > >> > > >> > As far as I know, these are the two outliers that also have rseq > >> > support. The list is a bit longer if we also consider non-rseq > >> > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know > >> > about the Linux architectures without glibc support). > >> > > >> > >> For kvm/selftests, there are 3 architectures involved actually. So we > >> just need consider 4 cases: aarch64, x86, s390 and other. For other > >> case, we just use __builtin_thread_pointer() to maintain code's > >> integrity, but it's not called at all. > >> > >> I think kvm/selftest is always relying on glibc if I'm correct. > > > > All those are handled in the rseq selftests and in librseq. Why duplicate all > > that logic again? > > More to the point, considering that we have all the relevant rseq registration > code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread > pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, > is there an easy way to get test applications in tools/testing/selftests/kvm > and in tools/testing/selftests/rseq to share that common code ? > > Keeping duplicated compatibility code is bad for long-term maintainability. Any reason not to simply add tools/lib/rseq.c and then expose a helper to get the registered rseq struct?
Hi Mathieu and Sean, On 8/10/22 7:38 AM, Sean Christopherson wrote: > On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: >>> ----- Gavin Shan <gshan@redhat.com> wrote: >>>> On 8/9/22 5:16 PM, Florian Weimer wrote: >>>>>>> __builtin_thread_pointer doesn't work on all architectures/GCC >>>>>>> versions. >>>>>>> Is this a problem for selftests? >>>>>>> >>>>>> >>>>>> It's a problem as the test case is running on all architectures. I think I >>>>>> need introduce our own __builtin_thread_pointer() for where it's not >>>>>> supported: (1) PowerPC (2) x86 without GCC 11 >>>>>> >>>>>> Please let me know if I still have missed cases where >>>>>> __buitin_thread_pointer() isn't supported? >>>>> >>>>> As far as I know, these are the two outliers that also have rseq >>>>> support. The list is a bit longer if we also consider non-rseq >>>>> architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >>>>> about the Linux architectures without glibc support). >>>>> >>>> >>>> For kvm/selftests, there are 3 architectures involved actually. So we >>>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>>> case, we just use __builtin_thread_pointer() to maintain code's >>>> integrity, but it's not called at all. >>>> >>>> I think kvm/selftest is always relying on glibc if I'm correct. >>> >>> All those are handled in the rseq selftests and in librseq. Why duplicate all >>> that logic again? >> >> More to the point, considering that we have all the relevant rseq registration >> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >> is there an easy way to get test applications in tools/testing/selftests/kvm >> and in tools/testing/selftests/rseq to share that common code ? >> >> Keeping duplicated compatibility code is bad for long-term maintainability. > > Any reason not to simply add tools/lib/rseq.c and then expose a helper to get the > registered rseq struct? > There are couple of reasons, not to share tools/testing/selftests/rseq/librseq.so or add tools/lib/librseq.so. Please let me know if the arguments making sense to you? - By design, selftests/rseq and selftests/kvm are parallel. It's going to introduce unnecessary dependency for selftests/kvm to use selftests/rseq/librseq.so. To me, it makes the maintainability even harder. - What selftests/kvm needs is rseq-thread-pointer.h, which accounts for ~5% of functionalities, provided by selftests/rseq/librseq.so. - I'm not too much familiar with selftests/rseq, but it seems it need heavy rework before it can become tools/lib/librseq.so. However, I'm not sure if the effort is worthwhile. The newly added library is fully used by testtests/rseq. ~5% of that is going to be used by selftests/kvm. In this case, we still have cross-dependency issue. I personally prefer not to use selftests/rseq/librseq.so or add tools/lib/librseq.so, but I need your feedback. Please share your thoughts. Thanks, Gavin
On 8/9/22 14:21, Mathieu Desnoyers wrote: >> For kvm/selftests, there are 3 architectures involved actually. So we >> just need consider 4 cases: aarch64, x86, s390 and other. For other >> case, we just use __builtin_thread_pointer() to maintain code's >> integrity, but it's not called at all. >> >> I think kvm/selftest is always relying on glibc if I'm correct. > All those are handled in the rseq selftests and in librseq. Why duplicate all that logic again? Yeah, rseq_test should reuse librseq code. The simplest way, if slightly hackish, is to do something like diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 690b499c3471..6c192b0ec304 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) UNAME_M := riscv endif LIBKVM += lib/assert.c LIBKVM += lib/elf.c LIBKVM += lib/guest_modes.c @@ -198,7 +199,7 @@ endif CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) and just #include "../rseq/rseq.c" in rseq_test.c. Thanks, Paolo
Hi Paolo, On 8/10/22 7:14 PM, Paolo Bonzini wrote: > On 8/9/22 14:21, Mathieu Desnoyers wrote: >>> For kvm/selftests, there are 3 architectures involved actually. So we >>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>> case, we just use __builtin_thread_pointer() to maintain code's >>> integrity, but it's not called at all. >>> >>> I think kvm/selftest is always relying on glibc if I'm correct. >> All those are handled in the rseq selftests and in librseq. Why duplicate all that logic again? > > Yeah, rseq_test should reuse librseq code. The simplest way, > if slightly hackish, is to do something like > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index 690b499c3471..6c192b0ec304 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) > UNAME_M := riscv > endif > > LIBKVM += lib/assert.c > LIBKVM += lib/elf.c > LIBKVM += lib/guest_modes.c > @@ -198,7 +199,7 @@ endif > CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ > -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ > -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ > - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) > + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq > > no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ > $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) > > > and just #include "../rseq/rseq.c" in rseq_test.c. > Thank you. It's really a nice idea. I think it's best way to share "../rseq/rseq.c". In this way, we needn't to rely on "../rseq/librseq.so", which is compiled by "../rseq/Makefile". I will modify the code accordingly in v2 :) Thanks, Gavin
----- On Aug 9, 2022, at 5:38 PM, Sean Christopherson seanjc@google.com wrote: > On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers >> mathieu.desnoyers@efficios.com wrote: >> >> > ----- Gavin Shan <gshan@redhat.com> wrote: >> >> Hi Florian, >> >> >> >> On 8/9/22 5:16 PM, Florian Weimer wrote: >> >> >>> __builtin_thread_pointer doesn't work on all architectures/GCC >> >> >>> versions. >> >> >>> Is this a problem for selftests? >> >> >>> >> >> >> >> >> >> It's a problem as the test case is running on all architectures. I think I >> >> >> need introduce our own __builtin_thread_pointer() for where it's not >> >> >> supported: (1) PowerPC (2) x86 without GCC 11 >> >> >> >> >> >> Please let me know if I still have missed cases where >> >> >> __buitin_thread_pointer() isn't supported? >> >> > >> >> > As far as I know, these are the two outliers that also have rseq >> >> > support. The list is a bit longer if we also consider non-rseq >> >> > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >> >> > about the Linux architectures without glibc support). >> >> > >> >> >> >> For kvm/selftests, there are 3 architectures involved actually. So we >> >> just need consider 4 cases: aarch64, x86, s390 and other. For other >> >> case, we just use __builtin_thread_pointer() to maintain code's >> >> integrity, but it's not called at all. >> >> >> >> I think kvm/selftest is always relying on glibc if I'm correct. >> > >> > All those are handled in the rseq selftests and in librseq. Why duplicate all >> > that logic again? >> >> More to the point, considering that we have all the relevant rseq registration >> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >> is there an easy way to get test applications in tools/testing/selftests/kvm >> and in tools/testing/selftests/rseq to share that common code ? >> >> Keeping duplicated compatibility code is bad for long-term maintainability. > > Any reason not to simply add tools/lib/rseq.c and then expose a helper to get > the > registered rseq struct? Indeed, moving rseq.c to tools/lib/ would allow building a .so from any selftest which needs to use it. And we could move the relevant rseq helper header files to tools/include/rseq/* as well. Thoughts ? Thanks, Mathieu
----- On Aug 10, 2022, at 5:14 AM, Paolo Bonzini pbonzini@redhat.com wrote: > On 8/9/22 14:21, Mathieu Desnoyers wrote: >>> For kvm/selftests, there are 3 architectures involved actually. So we >>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>> case, we just use __builtin_thread_pointer() to maintain code's >>> integrity, but it's not called at all. >>> >>> I think kvm/selftest is always relying on glibc if I'm correct. >> All those are handled in the rseq selftests and in librseq. Why duplicate all >> that logic again? > > Yeah, rseq_test should reuse librseq code. The simplest way, > if slightly hackish, is to do something like > > diff --git a/tools/testing/selftests/kvm/Makefile > b/tools/testing/selftests/kvm/Makefile > index 690b499c3471..6c192b0ec304 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) > UNAME_M := riscv > endif > > LIBKVM += lib/assert.c > LIBKVM += lib/elf.c > LIBKVM += lib/guest_modes.c > @@ -198,7 +199,7 @@ endif > CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ > -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ > -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ > - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) > + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq > > no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ > $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) > > > and just #include "../rseq/rseq.c" in rseq_test.c. Hi Paolo, Indeed, this hack seems to be a good approach to immediately fix things without moving around all source files and headers. In the longer term, I'd prefer Sean's proposal to move rseq.c to tools/lib/ (and to move rseq headers to tools/include/rseq/). This can be done in a follow up phase though. I'll put a note on my todo list for after I come back from vacation. I'll be able to do this refactoring on top of this fix. Thanks, Mathieu > > Thanks, > > Paolo
On 8/10/22 14:17, Mathieu Desnoyers wrote: > Indeed, this hack seems to be a good approach to immediately fix things without > moving around all source files and headers. In the longer term, I'd prefer Sean's > proposal to move rseq.c to tools/lib/ (and to move rseq headers to tools/include/rseq/). > This can be done in a follow up phase though. I'll put a note on my todo list > for after I come back from vacation. Great, Gavin, are you going to repost using librseq? >> Yeah, rseq_test should reuse librseq code. The simplest way, >> if slightly hackish, is to do something like >> >> diff --git a/tools/testing/selftests/kvm/Makefile >> b/tools/testing/selftests/kvm/Makefile >> index 690b499c3471..6c192b0ec304 100644 >> --- a/tools/testing/selftests/kvm/Makefile >> +++ b/tools/testing/selftests/kvm/Makefile >> @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) >> UNAME_M := riscv >> endif >> >> LIBKVM += lib/assert.c >> LIBKVM += lib/elf.c >> LIBKVM += lib/guest_modes.c >> @@ -198,7 +199,7 @@ endif >> CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ >> -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ >> -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ >> - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) >> + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq >> >> no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ >> $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) >> >> >> and just #include "../rseq/rseq.c" in rseq_test.c. Paolo
----- On Aug 9, 2022, at 8:37 PM, Gavin Shan gshan@redhat.com wrote: > Hi Mathieu and Sean, > > On 8/10/22 7:38 AM, Sean Christopherson wrote: >> On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >>> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers >>> mathieu.desnoyers@efficios.com wrote: >>>> ----- Gavin Shan <gshan@redhat.com> wrote: >>>>> On 8/9/22 5:16 PM, Florian Weimer wrote: >>>>>>>> __builtin_thread_pointer doesn't work on all architectures/GCC >>>>>>>> versions. >>>>>>>> Is this a problem for selftests? >>>>>>>> >>>>>>> >>>>>>> It's a problem as the test case is running on all architectures. I think I >>>>>>> need introduce our own __builtin_thread_pointer() for where it's not >>>>>>> supported: (1) PowerPC (2) x86 without GCC 11 >>>>>>> >>>>>>> Please let me know if I still have missed cases where >>>>>>> __buitin_thread_pointer() isn't supported? >>>>>> >>>>>> As far as I know, these are the two outliers that also have rseq >>>>>> support. The list is a bit longer if we also consider non-rseq >>>>>> architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >>>>>> about the Linux architectures without glibc support). >>>>>> >>>>> >>>>> For kvm/selftests, there are 3 architectures involved actually. So we >>>>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>>>> case, we just use __builtin_thread_pointer() to maintain code's >>>>> integrity, but it's not called at all. >>>>> >>>>> I think kvm/selftest is always relying on glibc if I'm correct. >>>> >>>> All those are handled in the rseq selftests and in librseq. Why duplicate all >>>> that logic again? >>> >>> More to the point, considering that we have all the relevant rseq registration >>> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >>> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >>> is there an easy way to get test applications in tools/testing/selftests/kvm >>> and in tools/testing/selftests/rseq to share that common code ? >>> >>> Keeping duplicated compatibility code is bad for long-term maintainability. >> >> Any reason not to simply add tools/lib/rseq.c and then expose a helper to get >> the >> registered rseq struct? >> > > There are couple of reasons, not to share > tools/testing/selftests/rseq/librseq.so > or add tools/lib/librseq.so. Please let me know if the arguments making sense > to you? > > - By design, selftests/rseq and selftests/kvm are parallel. It's going to > introduce > unnecessary dependency for selftests/kvm to use selftests/rseq/librseq.so. To > me, > it makes the maintainability even harder. In terms of build system, yes, selftests/rseq and selftests/kvm are side-by-side, and I agree it is odd to have a cross-dependency. That's where moving rseq.c to tools/lib/ makes sense. > > - What selftests/kvm needs is rseq-thread-pointer.h, which accounts for ~5% of > functionalities, provided by selftests/rseq/librseq.so. I've never seen this type of argument used to prevent using a library before, except on extremely memory-constrained devices, which is not our target here. Even if you would only use 1% of the features of a library, it does not justify reimplementing that 1% if that code already sits within the same project (kernel selftests). > > - I'm not too much familiar with selftests/rseq, but it seems it need heavy > rework before it can become tools/lib/librseq.so. However, I'm not sure if > the effort is worthwhile. The newly added library is fully used by > testtests/rseq. ~5% of that is going to be used by selftests/kvm. > In this case, we still have cross-dependency issue. No, it's just moving files around and a bit of Makefile modifications. That's the simple part. > > I personally prefer not to use selftests/rseq/librseq.so or add > tools/lib/librseq.so, > but I need your feedback. Please share your thoughts. I strongly favor that we use a two steps approach: 1) immediate fix: include ../rseq/rseq.c into your test code and use the headers, as proposed by Paolo. 2) I'll move librseq code into tools/lib/ and tools/include/rseq/, and adapt the users accordingly. (after the end of my vacation) Thanks, Mathieu > Thanks, > Gavin
On 8/10/22 14:29, Mathieu Desnoyers wrote: >> - By design, selftests/rseq and selftests/kvm are parallel. It's going to >> introduce >> unnecessary dependency for selftests/kvm to use selftests/rseq/librseq.so. To >> me, >> it makes the maintainability even harder. > In terms of build system, yes, selftests/rseq and selftests/kvm are side-by-side, > and I agree it is odd to have a cross-dependency. > > That's where moving rseq.c to tools/lib/ makes sense. > >> - What selftests/kvm needs is rseq-thread-pointer.h, which accounts for ~5% of >> functionalities, provided by selftests/rseq/librseq.so. > I've never seen this type of argument used to prevent using a library before, except > on extremely memory-constrained devices, which is not our target here. I agree. To me, the main argument against moving librseq to tools/lib is a variant of the build-system argument, namely that recursive Make sucks[1] and selftests/kvm right now does not use tools/lib. So, for a single-file library, it may be simply not worth the hassle. On the other hand, if "somebody else" does the work, I would have no problem with having selftests/kvm depend on tools/lib, not at all. Thanks, Paolo [1] Kbuild is a marvel that makes it work, but it works because there are no such cross-subdirectory dependencies and anyway tools/testing/selftests does not use Kbuild.
Hi Paolo and Mathieu, On 8/10/22 10:19 PM, Paolo Bonzini wrote: > On 8/10/22 14:17, Mathieu Desnoyers wrote: >> Indeed, this hack seems to be a good approach to immediately fix things without >> moving around all source files and headers. In the longer term, I'd prefer Sean's >> proposal to move rseq.c to tools/lib/ (and to move rseq headers to tools/include/rseq/). >> This can be done in a follow up phase though. I'll put a note on my todo list >> for after I come back from vacation. > > Great, Gavin, are you going to repost using librseq? > It seems you've merged v2. I will post additional patches to use tools/lib/librseq.so, depending on what Mathieu will have. Mathieu, Please let me know if there are anything I can help. >>> Yeah, rseq_test should reuse librseq code. The simplest way, >>> if slightly hackish, is to do something like >>> >>> diff --git a/tools/testing/selftests/kvm/Makefile >>> b/tools/testing/selftests/kvm/Makefile >>> index 690b499c3471..6c192b0ec304 100644 >>> --- a/tools/testing/selftests/kvm/Makefile >>> +++ b/tools/testing/selftests/kvm/Makefile >>> @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) >>> UNAME_M := riscv >>> endif >>> >>> LIBKVM += lib/assert.c >>> LIBKVM += lib/elf.c >>> LIBKVM += lib/guest_modes.c >>> @@ -198,7 +199,7 @@ endif >>> CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ >>> -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ >>> -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ >>> - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) >>> + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq >>> >>> no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ >>> $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) >>> >>> >>> and just #include "../rseq/rseq.c" in rseq_test.c. Thanks, Gavin
Hi Mathieu, On 8/10/22 10:13 PM, Mathieu Desnoyers wrote: > ----- On Aug 9, 2022, at 5:38 PM, Sean Christopherson seanjc@google.com wrote: >> On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >>> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers >>> mathieu.desnoyers@efficios.com wrote: [...] >>>> >>>> All those are handled in the rseq selftests and in librseq. Why duplicate all >>>> that logic again? >>> >>> More to the point, considering that we have all the relevant rseq registration >>> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >>> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >>> is there an easy way to get test applications in tools/testing/selftests/kvm >>> and in tools/testing/selftests/rseq to share that common code ? >>> >>> Keeping duplicated compatibility code is bad for long-term maintainability. >> >> Any reason not to simply add tools/lib/rseq.c and then expose a helper to get >> the >> registered rseq struct? > > Indeed, moving rseq.c to tools/lib/ would allow building a .so from any selftest > which needs to use it. > > And we could move the relevant rseq helper header files to tools/include/rseq/* > as well. > > Thoughts ? > One question is how librseq.so can be built automatically, when I'm going to build tools/testing/selftests/kvm/rseq_test. # cd linux/tools/testing/selftests/kvm # make rseq_test It's not perfect if I have to build tools/lib/librseq.so in advance, in order to build tools/testing/selftests/kvm/rseq_test for the sake of dependency. Thanks, Gavin
diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index a54d4d05a058..acb1bf1f06b3 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -9,6 +9,7 @@ #include <string.h> #include <signal.h> #include <syscall.h> +#include <dlfcn.h> #include <sys/ioctl.h> #include <sys/sysinfo.h> #include <asm/barrier.h> @@ -36,6 +37,8 @@ static __thread volatile struct rseq __rseq = { */ #define NR_TASK_MIGRATIONS 100000 +static bool __rseq_ownership; +static volatile struct rseq *__rseq_info; static pthread_t migration_thread; static cpu_set_t possible_mask; static int min_cpu, max_cpu; @@ -49,11 +52,33 @@ static void guest_code(void) GUEST_SYNC(0); } +static void sys_rseq_ownership(void) +{ + long *offset; + unsigned int *size, *flags; + + offset = dlsym(RTLD_NEXT, "__rseq_offset"); + size = dlsym(RTLD_NEXT, "__rseq_size"); + flags = dlsym(RTLD_NEXT, "__rseq_flags"); + + if (offset && size && *size && flags) { + __rseq_ownership = false; + __rseq_info = (struct rseq *)((uintptr_t)__builtin_thread_pointer() + + *offset); + } else { + __rseq_ownership = true; + __rseq_info = &__rseq; + } +} + static void sys_rseq(int flags) { int r; - r = syscall(__NR_rseq, &__rseq, sizeof(__rseq), flags, RSEQ_SIG); + if (!__rseq_ownership) + return; + + r = syscall(__NR_rseq, __rseq_info, sizeof(*__rseq_info), flags, RSEQ_SIG); TEST_ASSERT(!r, "rseq failed, errno = %d (%s)", errno, strerror(errno)); } @@ -218,6 +243,7 @@ int main(int argc, char *argv[]) calc_min_max_cpu(); + sys_rseq_ownership(); sys_rseq(0); /* @@ -256,7 +282,7 @@ int main(int argc, char *argv[]) */ smp_rmb(); cpu = sched_getcpu(); - rseq_cpu = READ_ONCE(__rseq.cpu_id); + rseq_cpu = READ_ONCE(__rseq_info->cpu_id); smp_rmb(); } while (snapshot != atomic_read(&seq_cnt));
The rseq information is registered by TLS, starting from glibc-2.35. In this case, the test always fails due to syscall(__NR_rseq). For example, on RHEL9.1 where upstream glibc-2.35 features are enabled on downstream glibc-2.34, the test fails like below. # ./rseq_test ==== Test Assertion Failure ==== rseq_test.c:60: !r pid=112043 tid=112043 errno=22 - Invalid argument 1 0x0000000000401973: main at rseq_test.c:226 2 0x0000ffff84b6c79b: ?? ??:0 3 0x0000ffff84b6c86b: ?? ??:0 4 0x0000000000401b6f: _start at ??:? rseq failed, errno = 22 (Invalid argument) # rpm -aq | grep glibc-2 glibc-2.34-39.el9.aarch64 Fix the issue by using the registered rseq information from TLS if it exists. Otherwise, we're going to register our own rseq information as before. Reported-by: Yihuang Yu <yihyu@redhat.com> Suggested-by: Florian Weimer <fweimer@redhat.com> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Signed-off-by: Gavin Shan <gshan@redhat.com> --- tools/testing/selftests/kvm/rseq_test.c | 30 +++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)