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

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 17 Jan 2019 at 10:58, Julia Suvorova <jusual@mail.ru> 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 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...)

thanks
-- PMM