Patchwork [moxie] Fix bug in tlb_fill.

login
register
mail settings
Submitter Anthony Green
Date May 13, 2013, 8:04 p.m.
Message ID <1368475464-3116-1-git-send-email-green@moxielogic.com>
Download mbox | patch
Permalink /patch/243535/
State New
Headers show

Comments

Anthony Green - May 13, 2013, 8:04 p.m.
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(-)
Richard Henderson - May 14, 2013, 4:02 p.m.
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~
Anthony Green - Dec. 15, 2013, 4:10 a.m.
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)
Andreas Färber - Dec. 15, 2013, 6:51 p.m.
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
Peter Maydell - Dec. 15, 2013, 7:03 p.m.
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
Anthony Green - Dec. 15, 2013, 7:26 p.m.
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
Peter Crosthwaite - Dec. 16, 2013, 12:07 a.m.
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
>

Patch

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)