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

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

[Julia Suvorova]

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

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

[Alistair Francis]

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
>
>

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

[Julia Suvorova]

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.

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Julia Suvorova]

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.

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

[Peter Maydell]

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

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

[Peter Maydell]

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

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

[no-reply]

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

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

[no-reply]

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

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

[Peter Maydell]

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

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

[Julia Suvorova]

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.

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

[Peter Maydell]

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