qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL


From: Richard Henderson
Subject: Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
Date: Sat, 15 Oct 2022 06:01:54 +1100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2

On 10/15/22 04:49, Peter Maydell wrote:
On Tue, 4 Oct 2022 at 20:27, Richard Henderson
<richard.henderson@linaro.org> wrote:

On 10/4/22 09:23, Peter Maydell wrote:
@@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)

   static void gen_exception_internal_insn(DisasContext *s, int excp)
   {
+    target_ulong pc_save = s->pc_save;
+
       gen_a64_update_pc(s, 0);
       gen_exception_internal(excp);
       s->base.is_jmp = DISAS_NORETURN;
+    s->pc_save = pc_save;

What is trashing s->pc_save that we have to work around like this,
here and in the other similar changes ?

gen_a64_update_pc trashes pc_save.

Off of the top of my head, I can't remember what conditionally uses exceptions 
(single
step?).  But the usage pattern that is interesting is

      brcond(x, y, L1)
      update_pc(disp1);
      exit-or-exception.
L1:
      update_pc(disp2);
      exit-or-exception.

where at L1 we should have the same pc_save value as we did at the brcond.  
Saving and
restoring around (at least some of) the DISAS_NORETURN points achieves that.

(I figured it would be easiest to continue this conversation
in this thread rather than breaking it up to reply to the v6
equivalent patch.)

I guess it does, but it feels like a weird place to be doing that.
If what we want is "at the label L1 we know the pc_save value
needs to be some specific thing", then shouldn't we
achieve that by setting pc_save to a specific value at that
point? e.g. wrapping gen_set_label() with a function that
does "set the label here, guest PC value is going to be this".
Which should generally be the save_pc value at the point
where you emit the brcond() to this label, right? Maybe you
could even have a little struct that wraps up "TCGLabel*
and the pc_save value associated with a branch to it".
But anyway, I think the less confusing way to handle this is
that the changes to pc_save happen at the label, because that's
where the runtime flow-of-control is already being dealt with.

Ok, I'll re-work this.

Also, I think that you need to do something for the case
in translate.c where we call tcg_remove_ops_after() --
that now needs to fix pc_save back up to the value it had
when we called tcg_last_op().

Yes.


(There is also one of those
in target/i386, I haven't checked whether the i386 pcrel
handling series took care of that.)

I'll double-check that case too, thanks.


r~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]