Patchwork Sparc-32 doesn't work in 3.1.

login
register
mail settings
Submitter Sergei Trofimovich
Date Dec. 14, 2011, 5:42 p.m.
Message ID <20111214204225.7c8ec86c@sf.home>
Download mbox | patch
Permalink /patch/131448/
State RFC
Delegated to: David Miller
Headers show

Comments

Sergei Trofimovich - Dec. 14, 2011, 5:42 p.m.
[ CCed Jakub ]

>>>   Boot time fixup v1.6. 4/Mar/98 Jakub Jelinek (jj@ultra.linux.cz).
>>>   Patching kernel for srmmu[Fujitsu TurboSparc]/iommu
>>>   Fixup i f029ddfc doesn't refer to a valid instruction at
>>>   f00de648[95eea000]
>>>   halt, power off

> I put the broken image up at http://landley.net/sparc-image for the
> moment, but if you build 3.1 with the attached .config and the toolchain
> mentioned last time, it should reproduce for you.  It's 100% reliable
> for me...

Nice! With this config it breaks for me on your and mine toolchains.
The offending function is ext4_kvmalloc (and similar ext4_kvzalloc).

The usual relocation in sparc looks like a pair of instructions loading
two pats of address in 2 instructions:

Like that:
>        sethi   %hi(ext4_fill_super), %o4       !, tmp113
>        or      %o4, %lo(ext4_fill_super), %o4  ! tmp113,, tmp28

In our case relocatable symbol sits in tail call, so %lo part is in "unusual"
RESTORE instruction:

> ext4_kvmalloc:
...
>         sethi  %hi(___i_page_kernel), %i2      !, tmp112
>        call    __vmalloc, 0    !
>         restore %i2, %lo(___i_page_kernel), %o2        ! tmp112,,
...

David: is this code correct? Or it's a compiler bug? I am sparc32 newbie.
(C source and asm sources of function are in [1])

I think this kind of code is generated only in -Os.
So to workaround it I tried this hack:

> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
>         ret = kmalloc(size, flags);
>         if (!ret)
>                 ret = __vmalloc(size, flags, PAGE_KERNEL);
> +
> +       asm __volatile__("nop":::"memory");
> +
>         return ret;
>  }
(for both ext4_kvmalloc / ext4_kvzalloc. Attached workaround as a patch.)

It forces compiler to geterate "usual" pattern for relocation.

I think of 2 solutions:

1. trying to fix sparc/boot/btfixupprep.c and arch/sparc/mm/btfixup.c
   to distinct HI22 and LO10 relocations as different ones.
   Right now they are merged into one 'i' type and rely on instruction heuristics to fix it.
2. Add a hack to arch/sparc/mm/btfixup.c to recognize restore instruction as well

Any others?

Hope that helps.

[1]:

void *ext4_kvmalloc(size_t size, gfp_t flags)
{
        void *ret;

        ret = kmalloc(size, flags);
        if (!ret)
                ret = __vmalloc(size, flags, PAGE_KERNEL);
        return ret;
}

        .global ext4_kvmalloc
        .type   ext4_kvmalloc, #function
        .proc   0120
ext4_kvmalloc:
        save    %sp, -96, %sp   !
        mov     %i0, %o0        ! size,
        call    __kmalloc, 0    !,
         mov    %i1, %o1        ! flags,
        cmp     %o0, 0  ! ret,
        bne     .LL274  !
         sethi  %hi(___i_page_kernel), %i2      !, tmp112
        call    __vmalloc, 0    !
         restore %i2, %lo(___i_page_kernel), %o2        ! tmp112,,
.LL274:
        jmp     %i7+8
         restore %g0, %o0, %o0  ! ret,
        .size   ext4_kvmalloc, .-ext4_kvmalloc
Eric Dumazet - Dec. 14, 2011, 5:49 p.m.
Le mercredi 14 décembre 2011 à 20:42 +0300, Sergei Trofimovich a écrit :
> [ CCed Jakub ]
> 
> >>>   Boot time fixup v1.6. 4/Mar/98 Jakub Jelinek (jj@ultra.linux.cz).
> >>>   Patching kernel for srmmu[Fujitsu TurboSparc]/iommu
> >>>   Fixup i f029ddfc doesn't refer to a valid instruction at
> >>>   f00de648[95eea000]
> >>>   halt, power off
> 
> > I put the broken image up at http://landley.net/sparc-image for the
> > moment, but if you build 3.1 with the attached .config and the toolchain
> > mentioned last time, it should reproduce for you.  It's 100% reliable
> > for me...
> 
> Nice! With this config it breaks for me on your and mine toolchains.
> The offending function is ext4_kvmalloc (and similar ext4_kvzalloc).
> 
> The usual relocation in sparc looks like a pair of instructions loading
> two pats of address in 2 instructions:
> 
> Like that:
> >        sethi   %hi(ext4_fill_super), %o4       !, tmp113
> >        or      %o4, %lo(ext4_fill_super), %o4  ! tmp113,, tmp28
> 
> In our case relocatable symbol sits in tail call, so %lo part is in "unusual"
> RESTORE instruction:
> 
> > ext4_kvmalloc:
> ...
> >         sethi  %hi(___i_page_kernel), %i2      !, tmp112
> >        call    __vmalloc, 0    !
> >         restore %i2, %lo(___i_page_kernel), %o2        ! tmp112,,
> ...
> 
> David: is this code correct? Or it's a compiler bug? I am sparc32 newbie.
> (C source and asm sources of function are in [1])
> 
> I think this kind of code is generated only in -Os.
> So to workaround it I tried this hack:
> 
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> >         ret = kmalloc(size, flags);
> >         if (!ret)
> >                 ret = __vmalloc(size, flags, PAGE_KERNEL);
> > +
> > +       asm __volatile__("nop":::"memory");
> > +
> >         return ret;
> >  }
> (for both ext4_kvmalloc / ext4_kvzalloc. Attached workaround as a patch.)
> 
> It forces compiler to geterate "usual" pattern for relocation.
> 
> I think of 2 solutions:
> 
> 1. trying to fix sparc/boot/btfixupprep.c and arch/sparc/mm/btfixup.c
>    to distinct HI22 and LO10 relocations as different ones.
>    Right now they are merged into one 'i' type and rely on instruction heuristics to fix it.
> 2. Add a hack to arch/sparc/mm/btfixup.c to recognize restore instruction as well
> 
> Any others?

3)  Adding a memset() in ext4_kvmalloc() and ext4_kvzalloc() to prefault
pages ?

4) (Unrelated) : add __GFP_HIGHMEM to __vmalloc() flags, so that high
memory pages can be used for large allocations.



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 14, 2011, 5:53 p.m.
From: Sergei Trofimovich <slyich@gmail.com>
Date: Wed, 14 Dec 2011 20:42:25 +0300

>> ext4_kvmalloc:
> ...
>>         sethi  %hi(___i_page_kernel), %i2      !, tmp112
>>        call    __vmalloc, 0    !
>>         restore %i2, %lo(___i_page_kernel), %o2        ! tmp112,,
> ...
> 
> David: is this code correct? Or it's a compiler bug? I am sparc32 newbie.
> (C source and asm sources of function are in [1])

It is correct, it's a tail call.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 14, 2011, 5:54 p.m.
From: Sergei Trofimovich <slyich@gmail.com>
Date: Wed, 14 Dec 2011 20:42:25 +0300

> It forces compiler to geterate "usual" pattern for relocation.

No, this is wrong.

The right fix is to simply teach btfixup to be able to patch
the 'restore' just as equally as it would patch an 'or'.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 44d0c8d..570d45e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -117,6 +117,7 @@  void *ext4_kvmalloc(size_t size, gfp_t flags)
 	ret = kmalloc(size, flags);
 	if (!ret)
 		ret = __vmalloc(size, flags, PAGE_KERNEL);
+	mb(); /* hack to pessimize code */
 	return ret;
 }
 
@@ -127,6 +128,7 @@  void *ext4_kvzalloc(size_t size, gfp_t flags)
 	ret = kzalloc(size, flags);
 	if (!ret)
 		ret = __vmalloc(size, flags | __GFP_ZERO, PAGE_KERNEL);
+	mb(); /* hack to pessimize code */
 	return ret;
 }