Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix PC overwriting

[Peter Maydell]

On Tue, 22 Jan 2019 at 13:59, Julia Suvorova <address@hidden> wrote:
> On 21.01.2019 20:24, Peter Maydell wrote:
> >> I think that probably what we ought to do is define that:
> >>   * set_pc has the logic that does whatever is expected
> >>     when the user sets the PC either by hand or when a
> >>     ELF file is loaded
> >>   * if that is not what is wanted in cpu_tb_exec() then
> >>     the target must implement the synchronize_from_tb hook
> >>     as well
> >>
> >> The trick here is figuring out whether we have a coherent
> >> cross-architecture definition of what we want set_pc's
> >> behaviour to be (or at least, if we are just baking in
> >> "act like an ELF file" we should document that...

> I checked all architectures. The trick with synchronize_from_tb() is
> made in mips and tricore. Others simply set "env->pc = value", some of
> them implement the more complex synchronize_from_tb() for use in
> cpu_tb_exec().
>
> I'm going to set "act like an ELF file" meaning to set_pc(), although I
> cannot be sure that simply setting pc to a value always means it in
> architectures other than these three.
>
> set_pc() is also called as cpu_set_pc() in the boot files, so we can
> remove all additional checks from them.
>
> Is the definition update for set_pc() and cpu_set_pc() in
> include/qom/cpu.h enough for documentation?

I think that is the documentation we ought to enhance to
make sure it's clear what we mean by "set the PC" (ie
"the same semantics as for setting execution to start at an
ELF file entry point"). The docs of synchronize_from_tb
are also a bit minimal.

How about this for some documentation text for cpu.h ?

 @set_pc: Callback for setting the Program Counter register. This
       should have the semantics used by the target architecture when
       setting the PC from a source such as an ELF file entry point;
       for example on Arm it will also set the Thumb mode bit based
       on the least significant bit of the new PC value.
       If the target behaviour here is anything other than "set
       the PC register to the value passed in" then the target must
       also implement the synchronize_from_tb hook.

 @synchronize_from_tb: Callback for synchronizing state from a TCG
       #TranslationBlock. This is called when we abandon execution
       of a TB before starting it, and must set all parts of the CPU
       state which the previous TB in the chain may not have updated.
       This always includes at least the program counter; some targets
       will need to do more. If this hook is not implemented then the
       default is to call @set_pc(tb->pc).

The code changes we need are:
 * implement synchronize_from_tb for Arm (since TYPE_AARCH64_CPU
   inherits from TYPE_ARM_CPU this means we either need to have
   the code handle both AArch64 and AArch32, or else implement
   separate versions of the hook for both)
 * make set_pc for Arm have the set-thumb-mode behaviour
 * remove now unnecessary code in target/arm/arm-powerctl.c that
   sets thumb mode itself and clears out the low bit of the PC value
 * ditto in hw/arm/boot.c

thanks
-- PMM