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

[Julia Suvorova <jusual@mail.ru>]

On 21.01.2019 20:24, Peter Maydell wrote:
> On Thu, 17 Jan 2019 at 19:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> 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 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.