Hello,
On Wed, 15 Jan 2025 18:30:41 +0900,
Benjamin Berg wrote:
> > > Maybe I am missing it, but I do not yet see proper FP register
> > > handling. This will be needed for task/thread switches and also signal
> > > emission/sigreturn. I am attaching the test program that I used to
> > > verify the correct behaviour when dealing with the recent changes to FP
> > > register handling in UML.
> >
> > thanks for the test program.
> >
> > I didn't address your comment on FP register handling as I couldn't
> > see any reproducer that causes the issue you raised (and maybe didn't
> > understand well the problem) so, the attached program helps a lot.
>
> Note that it doesn't directly test task/thread switches which only
> happen due to normal scheduling (i.e. via SIGALRM). So, it doesn't
> cover everything, but syscalls and signals are quite important by
> themselves. For normal UML both cases hit the same codepath, but I
> think in your case the SIGALRM entry point differs and should be tested
> separately.
indeed, scheduling works a different in nommu from normal UML.
> > Though nommu code only works with musl-libc, which I cannot use that
> > as-is, now I see what you meant with the first function, test_fp().
> >
> > (none):/# /root/test-signal-restore
> > pre-signal: 0.5
> > post-signal: 0.5
> > floating point register was not manipulated
> >
> > Tests on task switch (test_fp_ptrace) might need more work for me as
> > nommu only works with vfork(2) and vfork without exec(2) may not test
> > well on the implementation.
>
> Not sure you even have a working ptrace in nommu?
I don't think so.
> That reminds me. Please set arch_has_single_step to zero for nommu
> mode. If you figure out how to set the appropriate bit in EFLAGS when
> returning to userspace, then that would also work.
thanks, I'll fix this in the next revision.
> > and also I'm wishing to have this kind of useful tests as a reusable
> > way; as now I'm going to add a new configuration for UML, you're also
> > going for another SECCOMP mode over MMU, we may have at least 3
> > running modes for UML, which I think we should share the test
> > framework that we should pass. Not sure how it should be but using
> > Kunit is the first thing in my mind.
>
> My problem was, that I didn't know of a good place to put it. We could
> probably drop this test into tools/testing/selftests/x86, but how is it
> run then?
> As for kunit, that would be neat, but I it seems a bit more complicated
> to run userspace code from within the kernel.
thanks for sharing your experience.
I'll look for some nice place; for the moment, I would work with a
private/local environment.
for the fp register handling on syscall/signal, it is going to be like
this, not fully tested/verified yet though. with this, single syscall
adds 80 nsec delay (in my environment), which seems reasonable to me.
diff --git a/arch/x86/um/nommu/do_syscall_64.c
b/arch/x86/um/nommu/do_syscall_64.c
index 796beb0089fc..48b3d29e2db1 100644
--- a/arch/x86/um/nommu/do_syscall_64.c
+++ b/arch/x86/um/nommu/do_syscall_64.c
@@ -48,6 +48,9 @@ __visible void do_syscall_64(struct pt_regs *regs)
/* set fs register to the original host one */
os_x86_arch_prctl(0, ARCH_SET_FS, (void *)host_fs);
+ /* save fp registers */
+ asm volatile("fxsaveq %0" : "=m"(*(struct _xstate *)regs->regs.fp));
+
if (likely(syscall < NR_syscalls)) {
PT_REGS_SET_SYSCALL_RETURN(regs,
EXECUTE_SYSCALL(syscall, regs));
@@ -66,6 +69,9 @@ __visible void do_syscall_64(struct pt_regs *regs)
set_thread_flag(TIF_SIGPENDING);
interrupt_end();
+ /* restore fp registers */
+ asm volatile("fxrstorq %0" : : "m"((current->thread.regs.regs.fp)));
+
/* restore back fs register to userspace configured one */
os_x86_arch_prctl(0, ARCH_SET_FS,
(void *)(current->thread.regs.regs.gp[FS_BASE
diff --git a/arch/x86/um/shared/sysdep/ptrace.h
b/arch/x86/um/shared/sysdep/ptrace.h
index 8f7476ff6e95..7d553d9f05be 100644
--- a/arch/x86/um/shared/sysdep/ptrace.h
+++ b/arch/x86/um/shared/sysdep/ptrace.h
@@ -65,7 +65,7 @@ struct uml_pt_regs {
int is_user;
/* Dynamically sized FP registers (holds an XSTATE) */
- unsigned long fp[];
+ unsigned long fp[] __attribute__((aligned(16)));
};
-- Hajime
