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

[Julia Suvorova]

On 21.01.2019 20:24, Peter Maydell wrote:
> On Thu, 17 Jan 2019 at 19:27, Peter Maydell <address@hidden> wrote:
> > On Thu, 17 Jan 2019 at 10:58, Julia Suvorova <address@hidden> wrote:
> > > On 17.01.2019 13:13, Stefan Hajnoczi wrote:
> > > > generic_loader_reset() calls cpu_reset(s->cpu) followed by
> > > > CPUClass->set_pc(s->cpu, s->addr).
> > > >
> > > > ARM's arm_cpu_set_pc() doesn't special-case the Thumb bit (that's only
> > > > done in arm_cpu_reset()) so we end up with an invalid PC for Thumb mode
> > > > addresses.
> > > >
> > > > Maybe the following arm_cpu_reset() code should be moved to
> > > > arm_cpu_set_pc():
> > > >
> > > >     env->regs[15] = initial_pc & ~1;
> > > >     env->thumb = initial_pc & 1;
> > > >
> > > > Then arm_cpu_reset() can call arm_cpu_set_pc() instead of duplicating
> > > > this code.
> > >
> > > No, set_pc() is called in cpu_tb_exec() to restore the PC value and
> > > therefore should not be changed.
> >
> > The set_pc hook is also called for the gdbstub 'c' and 's' packets
> > if they supply an address. I am not sure what the correct behaviour
> > there is (it might be tricky to find out or test, because the
> > 'c' and 's' packets are deprecated in favour of vCont which doesn't
> > allow the address argument at all, and recent gdb neither emits
> > 'c addr' nor supports it in its gdbserver implementation).
>
> I asked Linaro's gdb developer, and they thought that the gdb
> 'c addr' behaviour ought to be "look at bit 0 and switch to
> Thumb or Arm mode accordingly".

Thanks a lot!

> > I notice that the MIPS set_pc() hook implementation does
> > set the M16 bit based on the low bit of the PC value;
> > they avoid the problem in cpu_tb_exec() by providing
> > the alternative synchronize_from_tb hook instead.
> >
> > 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?

Best regards, Julia Suvorova.