Patchwork [4/4] target-arm: neon fix

login
register
mail settings
Submitter Riku Voipio
Date Feb. 5, 2010, 3:52 p.m.
Message ID <1265385151-11024-5-git-send-email-riku.voipio@iki.fi>
Download mbox | patch
Permalink /patch/44655/
State New
Headers show

Comments

Riku Voipio - Feb. 5, 2010, 3:52 p.m.
From: Juha Riihimäki <juha.riihimaki@nokia.com>

add an extra check in "two registers and a shift" to ensure element
size decoding logic cannot fail.

Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
Signed-off-by: Riku Voipio <riku.voipio@nokia.com>
---
 target-arm/translate.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Laurent Desnogues - Feb. 7, 2010, 12:54 p.m.
On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote:
> From: Juha Riihimäki <juha.riihimaki@nokia.com>
>
> add an extra check in "two registers and a shift" to ensure element
> size decoding logic cannot fail.
>
> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
> Signed-off-by: Riku Voipio <riku.voipio@nokia.com>
> ---
>  target-arm/translate.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 743b846..8bba034 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4567,8 +4567,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
>                 size = 3;
>             } else {
>                 size = 2;
> -                while (size && (insn & (1 << (size + 19))) == 0)
> +                while (size && (insn & (1 << (size + 19))) == 0) {
>                     size--;
> +                }
>             }
>             shift = (insn >> 16) & ((1 << (3 + size)) - 1);
>             /* To avoid excessive dumplication of ops we implement shift

I think there's a patch ordering problem that makes
the comment and the change not agree :-)


Laurent
Laurent Desnogues - Feb. 7, 2010, 1:02 p.m.
On Sun, Feb 7, 2010 at 1:54 PM, Laurent Desnogues
<laurent.desnogues@gmail.com> wrote:
> On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote:
>> From: Juha Riihimäki <juha.riihimaki@nokia.com>
>>
>> add an extra check in "two registers and a shift" to ensure element
>> size decoding logic cannot fail.
>>
>> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
>> Signed-off-by: Riku Voipio <riku.voipio@nokia.com>
>> ---
>>  target-arm/translate.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 743b846..8bba034 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -4567,8 +4567,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
>>                 size = 3;
>>             } else {
>>                 size = 2;
>> -                while (size && (insn & (1 << (size + 19))) == 0)
>> +                while (size && (insn & (1 << (size + 19))) == 0) {
>>                     size--;
>> +                }
>>             }
>>             shift = (insn >> 16) & ((1 << (3 + size)) - 1);
>>             /* To avoid excessive dumplication of ops we implement shift
>
> I think there's a patch ordering problem that makes
> the comment and the change not agree :-)

BTW I don't think adding the check for size is needed
here.  The encoding at that point looks like this:

 3322222222221111111111
 10987654321098765432109876543210
 1111001_1___1______________1____
 1111001_1__1_______________1____
 1111001_1_1________________1____

so it will stop for size == 0 given that bit 19 will have to
be set.


Laurent
Riku Voipio - Feb. 8, 2010, 11:47 a.m.
On Sun, Feb 07, 2010 at 02:02:31PM +0100, Laurent Desnogues wrote:
> On Sun, Feb 7, 2010 at 1:54 PM, Laurent Desnogues
> <laurent.desnogues@gmail.com> wrote:
> > On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote:
> >> From: Juha Riihimäki <juha.riihimaki@nokia.com>

> >> add an extra check in "two registers and a shift" to ensure element
> >> size decoding logic cannot fail.

> > I think there's a patch ordering problem that makes
> > the comment and the change not agree :-)

Sorry, apparently messed up while rebasing.
 
> BTW I don't think adding the check for size is needed
> here.  The encoding at that point looks like this:
 
>  3322222222221111111111
>  10987654321098765432109876543210
>  1111001_1___1______________1____
>  1111001_1__1_______________1____
>  1111001_1_1________________1____
 
> so it will stop for size == 0 given that bit 19 will have to
> be set.

Juha agrees so we'll drop this patch (or more precisely get the actual change
out of the previous patch..)
Laurent Desnogues - Feb. 8, 2010, 4:05 p.m.
On Mon, Feb 8, 2010 at 12:47 PM, Riku Voipio <riku.voipio@iki.fi> wrote:
> On Sun, Feb 07, 2010 at 02:02:31PM +0100, Laurent Desnogues wrote:
>> On Sun, Feb 7, 2010 at 1:54 PM, Laurent Desnogues
>> <laurent.desnogues@gmail.com> wrote:
>> > On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote:
>> >> From: Juha Riihimäki <juha.riihimaki@nokia.com>
>
>> >> add an extra check in "two registers and a shift" to ensure element
>> >> size decoding logic cannot fail.
>
>> > I think there's a patch ordering problem that makes
>> > the comment and the change not agree :-)
>
> Sorry, apparently messed up while rebasing.
>
>> BTW I don't think adding the check for size is needed
>> here.  The encoding at that point looks like this:
>
>>  3322222222221111111111
>>  10987654321098765432109876543210
>>  1111001_1___1______________1____
>>  1111001_1__1_______________1____
>>  1111001_1_1________________1____
>
>> so it will stop for size == 0 given that bit 19 will have to
>> be set.
>
> Juha agrees so we'll drop this patch (or more precisely get the actual change
> out of the previous patch..)

Do you intend to resend the patch 3 of this set or should it
be reviewed as is?

Thanks,

Laurent

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 743b846..8bba034 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4567,8 +4567,9 @@  static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 size = 3;
             } else {
                 size = 2;
-                while (size && (insn & (1 << (size + 19))) == 0)
+                while (size && (insn & (1 << (size + 19))) == 0) {
                     size--;
+                }
             }
             shift = (insn >> 16) & ((1 << (3 + size)) - 1);
             /* To avoid excessive dumplication of ops we implement shift