If the memory is set using a file, and PC is specified on the command
line, it will be overwritten with the value 'entry'. This is not only
illogical, but also incorrect, because the load_ * functions do not take
into account the specifics of the ARM-M PC.
Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
hw/core/generic-loader.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index fbae05fb3b..fc81fd8e14 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -152,7 +152,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
if (size < 0 || s->force_raw) {
/* Default to the maximum size being the machine's ram size */
size = load_image_targphys_as(s->file, s->addr, ram_size, as);
- } else {
+ } else if (!s->addr) {
s->addr = entry;
}
--
2.17.1
On Tue, Jan 15, 2019 at 7:04 AM Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> wrote: > > If the memory is set using a file, and PC is specified on the command > line, it will be overwritten with the value 'entry'. This is not only > illogical, but also incorrect, because the load_ * functions do not take > into account the specifics of the ARM-M PC. How does this come up? I see that the value of entry will force overwrite the PC addr, but doesn't force_raw fix that? Is there a common use case of loading an ELF/uimage but having to manually specify a start address? Alistair > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > --- > hw/core/generic-loader.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > index fbae05fb3b..fc81fd8e14 100644 > --- a/hw/core/generic-loader.c > +++ b/hw/core/generic-loader.c > @@ -152,7 +152,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > if (size < 0 || s->force_raw) { > /* Default to the maximum size being the machine's ram size */ > size = load_image_targphys_as(s->file, s->addr, ram_size, as); > - } else { > + } else if (!s->addr) { > s->addr = entry; > } > > -- > 2.17.1 > >
On 16.01.2019 0:51, Alistair Francis wrote: > On Tue, Jan 15, 2019 at 7:04 AM Julia Suvorova via Qemu-devel > <qemu-devel@nongnu.org> wrote: >> >> If the memory is set using a file, and PC is specified on the command >> line, it will be overwritten with the value 'entry'. This is not only >> illogical, but also incorrect, because the load_ * functions do not take >> into account the specifics of the ARM-M PC. > > How does this come up? > I see that the value of entry will force overwrite the PC addr, but > doesn't force_raw fix that? Is there a common use case of loading an > ELF/uimage but having to manually specify a start address? generic_loader_reset() is called after arm_cpu_reset() and damages PC (it is wrong to call arm_cpu_set_pc() with entry to set ARM PC reset value). Therefore, I tried to configure PC manually and ran into this problem. By the way, I do not know the right way to fix the original issue. Try to replace generic_loader_reset() with the device reset function or change the reset order or transfer PC reset value setting to a separate function and associate it with cpu. What do you think about it? Best regards, Julia Suvorova.
On Wed, Jan 16, 2019 at 10:05:58PM +0300, Julia Suvorova via Qemu-devel wrote: > On 16.01.2019 0:51, Alistair Francis wrote: > > On Tue, Jan 15, 2019 at 7:04 AM Julia Suvorova via Qemu-devel > > <qemu-devel@nongnu.org> wrote: > > > > > > If the memory is set using a file, and PC is specified on the command > > > line, it will be overwritten with the value 'entry'. This is not only > > > illogical, but also incorrect, because the load_ * functions do not take > > > into account the specifics of the ARM-M PC. > > > > How does this come up? > > I see that the value of entry will force overwrite the PC addr, but > > doesn't force_raw fix that? Is there a common use case of loading an > > ELF/uimage but having to manually specify a start address? > > generic_loader_reset() is called after arm_cpu_reset() and damages PC > (it is wrong to call arm_cpu_set_pc() with entry to set ARM PC reset > value). Therefore, I tried to configure PC manually and ran into this > problem. > > By the way, I do not know the right way to fix the original issue. Try > to replace generic_loader_reset() with the device reset function or > change the reset order or transfer PC reset value setting to a separate > function and associate it with cpu. What do you think about it? 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. I haven't checked whether more special logic is needed in arm_cpu_set_pc() aside from setting Thumb mode, but I think this should do the trick? Stefan
On 17.01.2019 13:13, Stefan Hajnoczi wrote: > On Wed, Jan 16, 2019 at 10:05:58PM +0300, Julia Suvorova via Qemu-devel wrote: >> On 16.01.2019 0:51, Alistair Francis wrote: >>> On Tue, Jan 15, 2019 at 7:04 AM Julia Suvorova via Qemu-devel >>> <qemu-devel@nongnu.org> wrote: >>>> >>>> If the memory is set using a file, and PC is specified on the command >>>> line, it will be overwritten with the value 'entry'. This is not only >>>> illogical, but also incorrect, because the load_ * functions do not take >>>> into account the specifics of the ARM-M PC. >>> >>> How does this come up? >>> I see that the value of entry will force overwrite the PC addr, but >>> doesn't force_raw fix that? Is there a common use case of loading an >>> ELF/uimage but having to manually specify a start address? >> >> generic_loader_reset() is called after arm_cpu_reset() and damages PC >> (it is wrong to call arm_cpu_set_pc() with entry to set ARM PC reset >> value). Therefore, I tried to configure PC manually and ran into this >> problem. >> >> By the way, I do not know the right way to fix the original issue. Try >> to replace generic_loader_reset() with the device reset function or >> change the reset order or transfer PC reset value setting to a separate >> function and associate it with cpu. What do you think about it? > > 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. > I haven't checked whether more special logic is needed in > arm_cpu_set_pc() aside from setting Thumb mode, but I think this should > do the trick? The logic is a bit more complicated, but I think that it can be transferred to a separate function, but not to arm_cpu_set_pc(). Best regards, Julia Suvorova.
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
On Thu, 17 Jan 2019 at 19:27, Peter Maydell <peter.maydell@linaro.org> wrote: > recent gdb neither emits > 'c addr' nor supports it in its gdbserver implementation ...and I dug about in the gdb git history, and as far as I can tell even back in 2005 or so gdb's gdbserver never supported the 'addr' parameter to 'c' and 's', and the gdb end of the remote protocol doesn't emit it, which raises the question of whether anybody ever actually used it! thanks -- PMM
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 -- PMM
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.
On Tue, 22 Jan 2019 at 13:59, Julia Suvorova <jusual@mail.ru> wrote: > On 21.01.2019 20:24, Peter Maydell wrote: > >> 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? I think that is the documentation we ought to enhance to make sure it's clear what we mean by "set the PC" (ie "the same semantics as for setting execution to start at an ELF file entry point"). The docs of synchronize_from_tb are also a bit minimal. How about this for some documentation text for cpu.h ? @set_pc: Callback for setting the Program Counter register. This should have the semantics used by the target architecture when setting the PC from a source such as an ELF file entry point; for example on Arm it will also set the Thumb mode bit based on the least significant bit of the new PC value. If the target behaviour here is anything other than "set the PC register to the value passed in" then the target must also implement the synchronize_from_tb hook. @synchronize_from_tb: Callback for synchronizing state from a TCG #TranslationBlock. This is called when we abandon execution of a TB before starting it, and must set all parts of the CPU state which the previous TB in the chain may not have updated. This always includes at least the program counter; some targets will need to do more. If this hook is not implemented then the default is to call @set_pc(tb->pc). The code changes we need are: * implement synchronize_from_tb for Arm (since TYPE_AARCH64_CPU inherits from TYPE_ARM_CPU this means we either need to have the code handle both AArch64 and AArch32, or else implement separate versions of the hook for both) * make set_pc for Arm have the set-thumb-mode behaviour * remove now unnecessary code in target/arm/arm-powerctl.c that sets thumb mode itself and clears out the low bit of the PC value * ditto in hw/arm/boot.c thanks -- PMM
Patchew URL: https://patchew.org/QEMU/20190115143650.15725-1-jusual@mail.ru/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20190115143650.15725-1-jusual@mail.ru Subject: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix PC overwriting === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' be175e9 hw/core/generic-loader: Fix PC overwriting === OUTPUT BEGIN === ERROR: Author email address is mangled by the mailing list #2: Author: Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> total: 1 errors, 0 warnings, 8 lines checked Commit be175e9f052e (hw/core/generic-loader: Fix PC overwriting) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190115143650.15725-1-jusual@mail.ru/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20190115143650.15725-1-jusual@mail.ru/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=14 === TEST SCRIPT END === CC qapi/qapi-commands-transaction.o CC qapi/qapi-commands-ui.o /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name': /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation] strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors The full log is available at http://patchew.org/logs/20190115143650.15725-1-jusual@mail.ru/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.