| Message ID | 20211019164447.16359-23-imp@bsdimp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bsd-user: arm (32-bit) support | expand |
On Tue, Oct 19, 2021 at 11:45 AM Warner Losh <imp@bsdimp.com> wrote: > > Move the machine context to the CPU state. > > Signed-off-by: Stacey Son <sson@FreeBSD.org> > Signed-off-by: Klye Evans <kevans@FreeBSD.org> > Signed-off-by: Warner Losh <imp@bsdimp.com> > --- > bsd-user/arm/target_arch_signal.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/bsd-user/arm/target_arch_signal.h b/bsd-user/arm/target_arch_signal.h > index 302fdc2846..1d051af9ae 100644 > --- a/bsd-user/arm/target_arch_signal.h > +++ b/bsd-user/arm/target_arch_signal.h > @@ -201,4 +201,35 @@ static inline abi_long get_mcontext(CPUARMState *regs, target_mcontext_t *mcp, > return err; > } > > +/* Compare to arm/arm/machdep.c set_mcontext() */ > +static inline abi_long set_mcontext(CPUARMState *regs, target_mcontext_t *mcp, > + int srflag) > +{ > + int err = 0; > + const uint32_t *gr = mcp->__gregs; > + uint32_t cpsr; > + > + regs->regs[0] = tswap32(gr[TARGET_REG_R0]); > + regs->regs[1] = tswap32(gr[TARGET_REG_R1]); > + regs->regs[2] = tswap32(gr[TARGET_REG_R2]); > + regs->regs[3] = tswap32(gr[TARGET_REG_R3]); > + regs->regs[4] = tswap32(gr[TARGET_REG_R4]); > + regs->regs[5] = tswap32(gr[TARGET_REG_R5]); > + regs->regs[6] = tswap32(gr[TARGET_REG_R6]); > + regs->regs[7] = tswap32(gr[TARGET_REG_R7]); > + regs->regs[8] = tswap32(gr[TARGET_REG_R8]); > + regs->regs[9] = tswap32(gr[TARGET_REG_R9]); > + regs->regs[10] = tswap32(gr[TARGET_REG_R10]); > + regs->regs[11] = tswap32(gr[TARGET_REG_R11]); > + regs->regs[12] = tswap32(gr[TARGET_REG_R12]); > + > + regs->regs[13] = tswap32(gr[TARGET_REG_SP]); > + regs->regs[14] = tswap32(gr[TARGET_REG_LR]); > + regs->regs[15] = tswap32(gr[TARGET_REG_PC]); > + cpsr = tswap32(gr[TARGET_REG_CPSR]); > + cpsr_write(regs, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr); > + > + return err; > +} > + > #endif /* !_TARGET_ARCH_SIGNAL_H_ */ > -- > 2.32.0 > Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
On 10/19/21 9:44 AM, Warner Losh wrote: > + regs->regs[15] = tswap32(gr[TARGET_REG_PC]); > + cpsr = tswap32(gr[TARGET_REG_CPSR]); > + cpsr_write(regs, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr); Hmm. What's the expected behaviour if the saved CPSR state does not match the PC state wrt thumb? I'm ok if this should fail in spectacular ways, I just wanna know. I *think* what will happen at the moment is that qemu will go into a whacky state in which the translator will read and interpret unaligned data. I have a pending patch set for arm to raise unaligned exceptions for mis-aligned pc. For arm32 mode, this is fine, and we'll raise the exception. But for thumb mode, this is architecturally impossible, and the translator will assert. The assert is going to be a problem. There are a couple of options: (1) TARGET_REG_PC wins: unset PC[0] and adjust CPSR[T] to match. (2) CPSR_T wins: unset pc[0] if CPSR[T] is set, unchanged otherwise. (In the Arm ARM psueodcode, pc[0] is hardwired to 0 in thumb mode.) (3) Don't worry about matching PC[0] and CPSR[T], but do prevent an impossible situation and unset PC[0] always. If PC[1] is set, and CPSR[T] is unset, then we'll raise unaligned when the cpu restarts. And, finally, you're missing the mc_vfp_* handling. r~
On 10/19/21 9:44 AM, Warner Losh wrote: > + cpsr = tswap32(gr[TARGET_REG_CPSR]); > + cpsr_write(regs, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr); The kernel's set_mcontext validates CPSR_{M,I,F}. r~
On Thu, Oct 28, 2021 at 11:53 AM Richard Henderson < richard.henderson@linaro.org> wrote: > On 10/19/21 9:44 AM, Warner Losh wrote: > > + regs->regs[15] = tswap32(gr[TARGET_REG_PC]); > > + cpsr = tswap32(gr[TARGET_REG_CPSR]); > > + cpsr_write(regs, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr); > > Hmm. What's the expected behaviour if the saved CPSR state does not match > the PC state > wrt thumb? > > I'm ok if this should fail in spectacular ways, I just wanna know. > > I *think* what will happen at the moment is that qemu will go into a > whacky state in which > the translator will read and interpret unaligned data. > > I have a pending patch set for arm to raise unaligned exceptions for > mis-aligned pc. For > arm32 mode, this is fine, and we'll raise the exception. But for thumb > mode, this is > architecturally impossible, and the translator will assert. > > The assert is going to be a problem. There are a couple of options: > > (1) TARGET_REG_PC wins: unset PC[0] and adjust CPSR[T] to match. > > (2) CPSR_T wins: unset pc[0] if CPSR[T] is set, unchanged otherwise. (In > the Arm ARM > psueodcode, pc[0] is hardwired to 0 in thumb mode.) > > (3) Don't worry about matching PC[0] and CPSR[T], but do prevent an > impossible situation > and unset PC[0] always. If PC[1] is set, and CPSR[T] is unset, then we'll > raise unaligned > when the cpu restarts. > Consider this program: #include <signal.h> #include <stdio.h> #include <unistd.h> int g; void fortytwo(int arg) { g = 42; } int main(int argc, char **argv) { g = 123; signal(SIGALRM, fortytwo); alarm(1); pause(); printf("G is %d\n", g); } Built for 'arm' I get G is 42 Build -mthumb I get qemu: uncaught target signal 11 (Segmentation fault) - core dumped Segmentation fault So even without your new assert, there are some issues I need to look into before I can get to this very interesting case :(. These are all good questions, though. I clearly have some work to do here... And, finally, you're missing the mc_vfp_* handling. > Yes. We don't really support that at the moment, but I'll look into how hard that might be to add. Warner
On Thu, Oct 28, 2021 at 6:07 PM Warner Losh <imp@bsdimp.com> wrote: > > > On Thu, Oct 28, 2021 at 11:53 AM Richard Henderson < > richard.henderson@linaro.org> wrote: > >> On 10/19/21 9:44 AM, Warner Losh wrote: >> > + regs->regs[15] = tswap32(gr[TARGET_REG_PC]); >> > + cpsr = tswap32(gr[TARGET_REG_CPSR]); >> > + cpsr_write(regs, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr); >> >> Hmm. What's the expected behaviour if the saved CPSR state does not >> match the PC state >> wrt thumb? >> >> I'm ok if this should fail in spectacular ways, I just wanna know. >> >> I *think* what will happen at the moment is that qemu will go into a >> whacky state in which >> the translator will read and interpret unaligned data. >> >> I have a pending patch set for arm to raise unaligned exceptions for >> mis-aligned pc. For >> arm32 mode, this is fine, and we'll raise the exception. But for thumb >> mode, this is >> architecturally impossible, and the translator will assert. >> >> The assert is going to be a problem. There are a couple of options: >> >> (1) TARGET_REG_PC wins: unset PC[0] and adjust CPSR[T] to match. >> >> (2) CPSR_T wins: unset pc[0] if CPSR[T] is set, unchanged otherwise. (In >> the Arm ARM >> psueodcode, pc[0] is hardwired to 0 in thumb mode.) >> >> (3) Don't worry about matching PC[0] and CPSR[T], but do prevent an >> impossible situation >> and unset PC[0] always. If PC[1] is set, and CPSR[T] is unset, then >> we'll raise unaligned >> when the cpu restarts. >> > > Consider this program: > #include <signal.h> > #include <stdio.h> > #include <unistd.h> > int g; > void fortytwo(int arg) { g = 42; } > int main(int argc, char **argv) { > g = 123; > signal(SIGALRM, fortytwo); alarm(1); pause(); > printf("G is %d\n", g); > } > > Built for 'arm' I get > G is 42 > Build -mthumb I get > qemu: uncaught target signal 11 (Segmentation fault) - core dumped > Segmentation fault > > So even without your new assert, there are some issues I need to look into > before I can > get to this very interesting case :(. These are all good questions, > though. I clearly have > some work to do here... > Turns out I just needed to filter things correctly, and the changes to bsd-user/arm/target_arch_thread.h made the thumb signals work. I've not yet written tests that run T32 instructions and get a A32 signal (or vice versa). I've also not tried to do the same with T32 and A32 threads (well, threads executing in those two modes and switching between them). That is beyond the scope of this set of patches, though. Real FreeBSD blindly sets these values and hopes the processor generates a fault for invalid states. With the filtering I added for the next round, we'll at least ensure that PC[0] == CPSR[T]. This ensures consistency, but I don't know how well it will fare in the real world. FreeBSD's thumb support wrt mcontext and thumb has only recently become more robust, but it doesn't check in the kernel. > And, finally, you're missing the mc_vfp_* handling. >> > > Yes. We don't really support that at the moment, but I'll look into how > hard that might be > to add. > I've added it here and in get_mcontext too. I'll also include an up-to-date copy of a pointer to the tip of the bsd-user fork so you can see the current state of thigns like signal.c, which I have penciled in for after the aarch and riscv64 patches that I have lined up (but haven't done the common errors between the archs yet). Since I'd either need to seen a super-large review or delay things somewhat for signal.c, I'd like to get the other architectures in since they are almost ready unless there's a compelling reason to do signal.c and all its dependencies next. But that's getting a bit far afield from this one patch.... And thank you for finding this and the other hard issues with this series! It's been instructive and gives me a few things to double check on the other, unsent series. Warner
diff --git a/bsd-user/arm/target_arch_signal.h b/bsd-user/arm/target_arch_signal.h index 302fdc2846..1d051af9ae 100644 --- a/bsd-user/arm/target_arch_signal.h +++ b/bsd-user/arm/target_arch_signal.h @@ -201,4 +201,35 @@ static inline abi_long get_mcontext(CPUARMState *regs, target_mcontext_t *mcp, return err; } +/* Compare to arm/arm/machdep.c set_mcontext() */ +static inline abi_long set_mcontext(CPUARMState *regs, target_mcontext_t *mcp, + int srflag) +{ + int err = 0; + const uint32_t *gr = mcp->__gregs; + uint32_t cpsr; + + regs->regs[0] = tswap32(gr[TARGET_REG_R0]); + regs->regs[1] = tswap32(gr[TARGET_REG_R1]); + regs->regs[2] = tswap32(gr[TARGET_REG_R2]); + regs->regs[3] = tswap32(gr[TARGET_REG_R3]); + regs->regs[4] = tswap32(gr[TARGET_REG_R4]); + regs->regs[5] = tswap32(gr[TARGET_REG_R5]); + regs->regs[6] = tswap32(gr[TARGET_REG_R6]); + regs->regs[7] = tswap32(gr[TARGET_REG_R7]); + regs->regs[8] = tswap32(gr[TARGET_REG_R8]); + regs->regs[9] = tswap32(gr[TARGET_REG_R9]); + regs->regs[10] = tswap32(gr[TARGET_REG_R10]); + regs->regs[11] = tswap32(gr[TARGET_REG_R11]); + regs->regs[12] = tswap32(gr[TARGET_REG_R12]); + + regs->regs[13] = tswap32(gr[TARGET_REG_SP]); + regs->regs[14] = tswap32(gr[TARGET_REG_LR]); + regs->regs[15] = tswap32(gr[TARGET_REG_PC]); + cpsr = tswap32(gr[TARGET_REG_CPSR]); + cpsr_write(regs, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr); + + return err; +} + #endif /* !_TARGET_ARCH_SIGNAL_H_ */