[Qemu-devel] [PATCH moxie] Fix bug in tlb_fill.

[Qemu-devel] [PATCH moxie] Fix bug in tlb_fill.

[Anthony Green]

Fix a simple bug in tlb_fill for moxie.  The port was mostly working
before, which is why I only really noticed it recently.  Thanks to
@jcmvbkbc for tracking it down.

Signed-off-by: Anthony Green <green@moxielogic.com>
---
 target-moxie/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-moxie/helper.c b/target-moxie/helper.c
index 6e0ac2a..6c36c49 100644
--- a/target-moxie/helper.c
+++ b/target-moxie/helper.c
@@ -55,8 +55,8 @@ void tlb_fill(CPUMoxieState *env, target_ulong addr, int is_write, int mmu_idx,
         if (retaddr) {
             cpu_restore_state(env, retaddr);
         }
+	cpu_loop_exit(env);
     }
-    cpu_loop_exit(env);
 }
 
 void helper_raise_exception(CPUMoxieState *env, int ex)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH moxie] Fix bug in tlb_fill.

[Max Filippov]

On Tue, May 14, 2013 at 12:04 AM, Anthony Green <green@moxielogic.com> wrote:
> Fix a simple bug in tlb_fill for moxie.  The port was mostly working
> before, which is why I only really noticed it recently.  Thanks to
> @jcmvbkbc for tracking it down.
>
> Signed-off-by: Anthony Green <green@moxielogic.com>
> ---
>  target-moxie/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-moxie/helper.c b/target-moxie/helper.c
> index 6e0ac2a..6c36c49 100644
> --- a/target-moxie/helper.c
> +++ b/target-moxie/helper.c
> @@ -55,8 +55,8 @@ void tlb_fill(CPUMoxieState *env, target_ulong addr, int is_write, int mmu_idx,
>          if (retaddr) {
>              cpu_restore_state(env, retaddr);
>          }
> +       cpu_loop_exit(env);
>      }
> -    cpu_loop_exit(env);
>  }


Hi Anthony,

that bug only revealed that some instructions (in that particular case jsra)
issue memory access while they have inconsistent registers state:

        case 0x03: /* jsra */
            {
                TCGv t1 = tcg_temp_new_i32();
                TCGv t2 = tcg_temp_new_i32();

                tcg_gen_movi_i32(t1, ctx->pc + 6);

                /* Make space for the static chain and return address.  */
                tcg_gen_subi_i32(t2, REG(1), 8);
                tcg_gen_mov_i32(REG(1), t2);
(1)-->                tcg_gen_qemu_st32(t1, REG(1), ctx->memidx);

                /* Push the current frame pointer.  */
                tcg_gen_subi_i32(t2, REG(1), 4);
                tcg_gen_mov_i32(REG(1), t2);
(2)-->                tcg_gen_qemu_st32(REG(0), REG(1), ctx->memidx);

                /* Set the pc and $fp.  */
                tcg_gen_mov_i32(REG(0), REG(1));

                gen_goto_tb(env, ctx, 0, cpu_ldl_code(env, ctx->pc+2));

                tcg_temp_free_i32(t1);
                tcg_temp_free_i32(t2);

                ctx->bstate = BS_BRANCH;
                length = 6;
            }

memory access at points (1) and (2) can abort the instruction (it did so
b/o the bug, but it may do so legitimately when you add MMU support),
but it has modified REG(1) at those points, which will not be restored.
It's probably worth carrying register modifications in some temporary
until after the point (2).

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH moxie] Fix bug in tlb_fill.

[Richard Henderson]

On 05/13/2013 01:33 PM, Max Filippov wrote:
> memory access at points (1) and (2) can abort the instruction (it did so
> b/o the bug, but it may do so legitimately when you add MMU support),
> but it has modified REG(1) at those points, which will not be restored.
> It's probably worth carrying register modifications in some temporary
> until after the point (2).

... Which you'll recall that I pointed out during initial review, AG.


r~

[Qemu-devel] ping.. Re: [PATCH moxie] Fix bug in tlb_fill.

[Anthony Green]

This patch still needs to be applied.  There was some follow-up
discussion on this patch back in May, but none of it negates the fact
that this patch needs to be applied.

Thanks!

AG

Anthony Green <green@moxielogic.com> writes:

> Fix a simple bug in tlb_fill for moxie.  The port was mostly working
> before, which is why I only really noticed it recently.  Thanks to
> @jcmvbkbc for tracking it down.
>
> Signed-off-by: Anthony Green <green@moxielogic.com>
> ---
>  target-moxie/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-moxie/helper.c b/target-moxie/helper.c
> index 6e0ac2a..6c36c49 100644
> --- a/target-moxie/helper.c
> +++ b/target-moxie/helper.c
> @@ -55,8 +55,8 @@ void tlb_fill(CPUMoxieState *env, target_ulong addr, int is_write, int mmu_idx,
>          if (retaddr) {
>              cpu_restore_state(env, retaddr);
>          }
> +	cpu_loop_exit(env);
>      }
> -    cpu_loop_exit(env);
>  }
>  
>  void helper_raise_exception(CPUMoxieState *env, int ex)

Re: [Qemu-devel] ping.. Re: [PATCH moxie] Fix bug in tlb_fill.

[Andreas Färber]

Hi,

Am 15.12.2013 05:10, schrieb Anthony Green:
> 
> This patch still needs to be applied.  There was some follow-up
> discussion on this patch back in May, but none of it negates the fact
> that this patch needs to be applied.

It introduces a tab, please fix.

Apart from that I believe I reported some inconsistencies between
targets in that function, so "all targets except moxie do it conditional
to FOO" may be a convincing explanation independent of the bug you were
discussing that may or may not be otherwise workaroundable.

And since this is purely in target-moxie I would suggest to simply send
a pull as target maintainer once you have someone trustworthy's
Reviewed-by and it doesn't break `make check`, similar to how it's done
for OpenRISC.

And as usual the warning that I have a patch lying around that changes
the cpu_loop_exit() argument to CPUState, but I may not get to rebasing
that until the holidays, so no imminent conflict.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] ping.. Re: [PATCH moxie] Fix bug in tlb_fill.

[Peter Maydell]

On 15 December 2013 18:51, Andreas Färber <afaerber@suse.de> wrote:
> Am 15.12.2013 05:10, schrieb Anthony Green:
>> This patch still needs to be applied.  There was some follow-up
>> discussion on this patch back in May, but none of it negates the fact
>> that this patch needs to be applied.
>
> It introduces a tab, please fix.
>
> Apart from that I believe I reported some inconsistencies between
> targets in that function, so "all targets except moxie do it conditional
> to FOO" may be a convincing explanation independent of the bug you were
> discussing that may or may not be otherwise workaroundable.

I dug out the thread where you did that (which turns out to be
private mail and not qemu-devel). In follow up to that RTH and I
agreed that it definitely is a bug and this patch is the correct fix.

> And since this is purely in target-moxie I would suggest to simply send
> a pull as target maintainer once you have someone trustworthy's
> Reviewed-by and it doesn't break `make check`, similar to how it's done
> for OpenRISC.

Yes, this makes sense to me, especially since Anthony has a number
of other moxie patches on list at this point.

thanks
-- PMM

Re: [Qemu-devel] ping.. Re: [PATCH moxie] Fix bug in tlb_fill.

[Anthony Green]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 15 December 2013 18:51, Andreas Färber <afaerber@suse.de> wrote:
>> And since this is purely in target-moxie I would suggest to simply send
>> a pull as target maintainer once you have someone trustworthy's
>> Reviewed-by and it doesn't break `make check`, similar to how it's done
>> for OpenRISC.
>
> Yes, this makes sense to me, especially since Anthony has a number
> of other moxie patches on list at this point.

According to this page...

http://wiki.qemu.org/Contribute/SubmitAPullRequest

...I need to get a key signed by a member of the QEMU community in
person.  Are there any QEMU hackers in the Toronto area?

AG

Re: [Qemu-devel] ping.. Re: [PATCH moxie] Fix bug in tlb_fill.

[Peter Crosthwaite]

On Mon, Dec 16, 2013 at 5:26 AM, Anthony Green <green@moxielogic.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 15 December 2013 18:51, Andreas Färber <afaerber@suse.de> wrote:
>>> And since this is purely in target-moxie I would suggest to simply send
>>> a pull as target maintainer once you have someone trustworthy's
>>> Reviewed-by and it doesn't break `make check`, similar to how it's done
>>> for OpenRISC.
>>
>> Yes, this makes sense to me, especially since Anthony has a number
>> of other moxie patches on list at this point.
>
> According to this page...
>
> http://wiki.qemu.org/Contribute/SubmitAPullRequest
>
> ...I need to get a key signed by a member of the QEMU community in
> person.  Are there any QEMU hackers in the Toronto area?
>

While this question has air, anyone in and around Brisbane Australia who
can sign me? (although that's probably a long shot).

I can get a key signed by Edgar if that is enough.

Regards,
Peter

> AG
>