Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

On Sat,  8 Apr 2017 20:53:18 +0100
Sergei Trofimovich  wrote:

> Starting from gcc-5.4+ gcc generates MLX
> instructions in more cases to refer local
> symbols:
> https://gcc.gnu.org/PR60465
> 
> That caused ia64 module loader to choke
> on such instructions:
> fuse: invalid slot number 1 for IMM64
> 
> Linux kernel used to handle only case where
> relocation pointed to slot=2 instruction in
> the bundle. That limitation was fixed in linux by
> commit 9c184a073bfd ("[IA64] Fix 2.6 kernel for the new ia64 assembler")
> See http://sources.redhat.com/bugzilla/show_bug.cgi?id=1433
> 
> This change lifts the slot=2 restriction from
> linux kernel module loader.
> 
> Tested on 'fuse' and 'btrfs' kernel modules.
> 
> Cc: Markus Elfring 
> Cc: H. J. Lu 
> Cc: Tony Luck 
> Cc: Fenghua Yu 
> Cc: linux-i...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Andrew Morton 
> Bug: https://bugs.gentoo.org/601014
> Tested-by: Émeric MASCHINO 
> Signed-off-by: Sergei Trofimovich 
> ---
> Change since v1: added 'Tested-by'
> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Ping :)

-- 

  Sergei


pgpCNIfSoxdHg.pgp
Description: Цифровая подпись OpenPGP

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

On Sat,  8 Apr 2017 20:53:18 +0100
Sergei Trofimovich  wrote:

> Starting from gcc-5.4+ gcc generates MLX
> instructions in more cases to refer local
> symbols:
> https://gcc.gnu.org/PR60465
> 
> That caused ia64 module loader to choke
> on such instructions:
> fuse: invalid slot number 1 for IMM64
> 
> Linux kernel used to handle only case where
> relocation pointed to slot=2 instruction in
> the bundle. That limitation was fixed in linux by
> commit 9c184a073bfd ("[IA64] Fix 2.6 kernel for the new ia64 assembler")
> See http://sources.redhat.com/bugzilla/show_bug.cgi?id=1433
> 
> This change lifts the slot=2 restriction from
> linux kernel module loader.
> 
> Tested on 'fuse' and 'btrfs' kernel modules.
> 
> Cc: Markus Elfring 
> Cc: H. J. Lu 
> Cc: Tony Luck 
> Cc: Fenghua Yu 
> Cc: linux-i...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Andrew Morton 
> Bug: https://bugs.gentoo.org/601014
> Tested-by: Émeric MASCHINO 
> Signed-off-by: Sergei Trofimovich 
> ---
> Change since v1: added 'Tested-by'
> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Ping :)

-- 

  Sergei


pgpCNIfSoxdHg.pgp
Description: Цифровая подпись OpenPGP

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[Sergei Trofimovich]

On Mon, 10 Apr 2017 19:23:28 +0200
SF Markus Elfring  wrote:

> > -   if (slot(insn) != 2) {
> > +   if (slot(insn) != 1 && slot(insn) != 2) {  
> 
> + int const s = slot(insn);
> + if (s < 1 || s > 2) {
> 
> Do run time characteristics matter for such a condition check here?

It's done once at kernel module load time. My guess would be
"not critical at all".

slot() is a pure arithmetic static inline function. You can compare
assembly output before and after your change.

You can measure the difference yourself using 'ski' emulator.
That's for example how I debugged and tested the patch:
 http://trofi.github.io/posts/199-ia64-machine-emulation.html

-- 

  Sergei


pgps9ql5YvoLF.pgp
Description: Цифровая подпись OpenPGP

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[Sergei Trofimovich]

On Mon, 10 Apr 2017 19:23:28 +0200
SF Markus Elfring  wrote:

> > -   if (slot(insn) != 2) {
> > +   if (slot(insn) != 1 && slot(insn) != 2) {  
> 
> + int const s = slot(insn);
> + if (s < 1 || s > 2) {
> 
> Do run time characteristics matter for such a condition check here?

It's done once at kernel module load time. My guess would be
"not critical at all".

slot() is a pure arithmetic static inline function. You can compare
assembly output before and after your change.

You can measure the difference yourself using 'ski' emulator.
That's for example how I debugged and tested the patch:
 http://trofi.github.io/posts/199-ia64-machine-emulation.html

-- 

  Sergei


pgps9ql5YvoLF.pgp
Description: Цифровая подпись OpenPGP

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[SF Markus Elfring]

> +++ b/arch/ia64/kernel/module.c
> @@ -153,7 +153,7 @@ slot (const struct insn *insn)
>  static int
>  apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
>  {

I have got another idea (after your clarification) for the suggested change.


> - if (slot(insn) != 2) {
> + if (slot(insn) != 1 && slot(insn) != 2) {

+   int const s = slot(insn);
+   if (s < 1 || s > 2) {

Do run time characteristics matter for such a condition check here?


>   printk(KERN_ERR "%s: invalid slot number %d for IMM64\n",

-  mod->name, slot(insn));
+  mod->name, s);


>   return 0;


How do you think about my update suggestion for this function implementation?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[SF Markus Elfring]

> +++ b/arch/ia64/kernel/module.c
> @@ -153,7 +153,7 @@ slot (const struct insn *insn)
>  static int
>  apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
>  {

I have got another idea (after your clarification) for the suggested change.


> - if (slot(insn) != 2) {
> + if (slot(insn) != 1 && slot(insn) != 2) {

+   int const s = slot(insn);
+   if (s < 1 || s > 2) {

Do run time characteristics matter for such a condition check here?


>   printk(KERN_ERR "%s: invalid slot number %d for IMM64\n",

-  mod->name, slot(insn));
+  mod->name, s);


>   return 0;


How do you think about my update suggestion for this function implementation?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

On Sun, 9 Apr 2017 10:27:52 +0200
SF Markus Elfring  wrote:

> > That caused ia64 module loader to choke
> > on such instructions:
> > fuse: invalid slot number 1 for IMM64  
> 
> Why does it matter to check such a value?

I'm not sure I follow the question. Is your question about
linux kernel relocation code handler, gcc or ia64 instruction
format?

-- 

  Sergei


pgpsbo9tMNJCe.pgp
Description: Цифровая подпись OpenPGP

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

On Sun, 9 Apr 2017 10:27:52 +0200
SF Markus Elfring  wrote:

> > That caused ia64 module loader to choke
> > on such instructions:
> > fuse: invalid slot number 1 for IMM64  
> 
> Why does it matter to check such a value?

I'm not sure I follow the question. Is your question about
linux kernel relocation code handler, gcc or ia64 instruction
format?

-- 

  Sergei


pgpsbo9tMNJCe.pgp
Description: Цифровая подпись OpenPGP

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[SF Markus Elfring]

> Starting from gcc-5.4+ gcc generates MLX

How do you think about to omit the plus character?


> instructions in more cases to refer local
> symbols:

I wonder about your choice of a line length limit here.


> That caused ia64 module loader to choke
> on such instructions:
> fuse: invalid slot number 1 for IMM64

Why does it matter to check such a value?


> … That limitation was fixed in linux by

Would it be nicer to write “in corresponding source code by the”?


> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Does this version information contain an unwanted word repetition?
How does it fit to the identifier “v3” in the commit subject?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[SF Markus Elfring]

> Starting from gcc-5.4+ gcc generates MLX

How do you think about to omit the plus character?


> instructions in more cases to refer local
> symbols:

I wonder about your choice of a line length limit here.


> That caused ia64 module loader to choke
> on such instructions:
> fuse: invalid slot number 1 for IMM64

Why does it matter to check such a value?


> … That limitation was fixed in linux by

Would it be nicer to write “in corresponding source code by the”?


> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Does this version information contain an unwanted word repetition?
How does it fit to the identifier “v3” in the commit subject?

Regards,
Markus