Re: [PATCH 22/24] bsd-user/arm/target_arch_signal.h: arm set_mcontext

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 5559 bytes --]