|
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.
[Prev in Thread] | Current Thread | [Next in Thread] |