qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Julia Suvorova
Subject: Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix PC overwriting
Date: Tue, 22 Jan 2019 16:59:22 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]