Re: [Qemu-devel] [PATCH 2/3] target-arm: add env->tbflags

[Paolo Bonzini <pbonzini@redhat.com>]

> Doing this for all MSR writes is a bit sad, because a lot of them
> don't actually change the TB flags, and quite a few of them which
> previously we were able to code to not have to do a helper call
> at all (direct writes to fields) now get a pointless helper call.

True.  On the other hand, MSR writes terminate the TB so you are
losing all the TB state anyway.  Before these patches you weren't
recomputing the TB flags in the common case of adjacent MSR writes
on the same page (so QEMU could use linked TBs), now you are.
However, given the speedup from the patch, I felt it was premature
optimization.

If there is a case where you get the helper in the profile, it is
possible to add a new ARM_CP_KEEP_TBFLAGS flag to ARMCPRegInfo.

> You're also recalculating more often than stated here, in that
> you also recalc on any gen_lookup_tb() call in the 32-bit
> decoder. (This is just as well because for instance vec_len
> and vec_stride aren't set via the cp15 system register write
> path.)

Right.  This was of course on purpose, but the commit message
was imprecise.

> You're treating the PSTATE_SS flag as static, but you don't
> have anything which causes a recalculation of it on the code
> path which changes it (gen_ss_advance()).
> 
> The 32-bit SETEND instruction changes CPSR_E, which has
> an effect on the BE_DATA_MASK flag, but I don't think
> that code path will cause us to recalculate flags.

This actually points to a bigger deficiency, in that---even
outside the PSTATE_SS and SETEND code paths---both pstate_write
and cpsr_write need to recompute the flags.  But I think that's
the only other case left.

Do you prefer to have the setend and clear_pstate_ss helpers
call cpsr_write/pstate_write, or do you prefer to inline the
modification to the tbflags?

> I found this patch kind of difficult to review because
> it isn't obvious why we recalculate the static flags at
> the points where we do (ie whether those points are
> necessary and sufficient for correct behaviour). Most
> of the comments above are the result of my looking at
> whether some particular flags that I suspected of being
> tricky were handled correctly :-)

You definitely have a point here.  Adding an assertion requires
looking at CPUARMState in gen_intermediate_code.  You're not really
supposed to do that, but I guess it's okay as long as it's for
debugging purposes.

Paolo