asm (lidt) question

asm (lidt) question

[Randy.Dunlap]

In arch/i386/kernel, inline asm for loading IDT (lidt) is used a few
times, but with slightly different constraints and output/input
definitions.  Are these OK, equivalent, or what?

[rddunlap@dragon kernel]$ findc lidt
./cpu/common.c:484: __asm__ __volatile__("lidt %0": "=m" (idt_descr));
./traps.c:783:	__asm__ __volatile__("lidt %0": "=m" (idt_descr));

vs.

./reboot.c:186:	__asm__ __volatile__ ("lidt %0" : : "m" (real_mode_idt));
./reboot.c:261:	__asm__ __volatile__("lidt %0": :"m" (no_idt));
./suspend.c:95:	asm volatile ("lidt %0" :: "m" (saved_context.idt_limit));


Thanks,
--
~Randy  [yes, i've looked at the inline asm docs]
For Linux-2.6:
http://www.codemonkey.org.uk/post-halloween-2.5.txt
  or http://lwn.net/Articles/39901/
http://www.kernel.org/pub/linux/kernel/people/rusty/modules/

Re: asm (lidt) question

[Davide Libenzi]

On Thu, 17 Jul 2003, Randy.Dunlap wrote:

>
> In arch/i386/kernel, inline asm for loading IDT (lidt) is used a few
> times, but with slightly different constraints and output/input
> definitions.  Are these OK, equivalent, or what?
>
> [rddunlap@dragon kernel]$ findc lidt
> ./cpu/common.c:484: __asm__ __volatile__("lidt %0": "=m" (idt_descr));
> ./traps.c:783:	__asm__ __volatile__("lidt %0": "=m" (idt_descr));
>
> vs.
>
> ./reboot.c:186:	__asm__ __volatile__ ("lidt %0" : : "m" (real_mode_idt));
> ./reboot.c:261:	__asm__ __volatile__("lidt %0": :"m" (no_idt));
> ./suspend.c:95:	asm volatile ("lidt %0" :: "m" (saved_context.idt_limit));

I'd have said no looking at the syntax (input/output), but they indeed
generate the same code (just checked).



- Davide

Re: asm (lidt) question

[Davide Libenzi]

On Thu, 17 Jul 2003, Davide Libenzi wrote:

> On Thu, 17 Jul 2003, Randy.Dunlap wrote:
>
> >
> > In arch/i386/kernel, inline asm for loading IDT (lidt) is used a few
> > times, but with slightly different constraints and output/input
> > definitions.  Are these OK, equivalent, or what?
> >
> > [rddunlap@dragon kernel]$ findc lidt
> > ./cpu/common.c:484: __asm__ __volatile__("lidt %0": "=m" (idt_descr));
> > ./traps.c:783:	__asm__ __volatile__("lidt %0": "=m" (idt_descr));
> >
> > vs.
> >
> > ./reboot.c:186:	__asm__ __volatile__ ("lidt %0" : : "m" (real_mode_idt));
> > ./reboot.c:261:	__asm__ __volatile__("lidt %0": :"m" (no_idt));
> > ./suspend.c:95:	asm volatile ("lidt %0" :: "m" (saved_context.idt_limit));
>
> I'd have said no looking at the syntax (input/output), but they indeed
> generate the same code (just checked).

Randy, I'd say that this :

__asm__ __volatile__("lidt %0": "=m" (var));

is better then :

__asm__ __volatile__("lidt %0": :"m" (var));

IMHO, since "var" is really an output parameter.



- Davide

Re: asm (lidt) question

[Randy.Dunlap]

On Thu, 17 Jul 2003 16:18:06 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

| On Thu, 17 Jul 2003, Davide Libenzi wrote:
| 
| > On Thu, 17 Jul 2003, Randy.Dunlap wrote:
| >
| > >
| > > In arch/i386/kernel, inline asm for loading IDT (lidt) is used a few
| > > times, but with slightly different constraints and output/input
| > > definitions.  Are these OK, equivalent, or what?
| > >
| > > [rddunlap@dragon kernel]$ findc lidt
| > > ./cpu/common.c:484: __asm__ __volatile__("lidt %0": "=m" (idt_descr));
| > > ./traps.c:783:	__asm__ __volatile__("lidt %0": "=m" (idt_descr));
| > >
| > > vs.
| > >
| > > ./reboot.c:186:	__asm__ __volatile__ ("lidt %0" : : "m" (real_mode_idt));
| > > ./reboot.c:261:	__asm__ __volatile__("lidt %0": :"m" (no_idt));
| > > ./suspend.c:95:	asm volatile ("lidt %0" :: "m" (saved_context.idt_limit));
| >
| > I'd have said no looking at the syntax (input/output), but they indeed
| > generate the same code (just checked).
| 
| Randy, I'd say that this :
| 
| __asm__ __volatile__("lidt %0": "=m" (var));
| 
| is better then :
| 
| __asm__ __volatile__("lidt %0": :"m" (var));
| 
| IMHO, since "var" is really an output parameter.

Yes, I agree, var is output and should be listed after the first ':'
IMHO also.

Thanks for checking that they generate the same code, though.

I'll buy you a beer...  No, wait, I already did that.  :)

--
~Randy

Re: asm (lidt) question

[Davide Libenzi]

On Thu, 17 Jul 2003, Randy.Dunlap wrote:

> | Randy, I'd say that this :
> |
> | __asm__ __volatile__("lidt %0": "=m" (var));
> |
> | is better then :
> |
> | __asm__ __volatile__("lidt %0": :"m" (var));
> |
> | IMHO, since "var" is really an output parameter.
>
> Yes, I agree, var is output and should be listed after the first ':'
> IMHO also.

BF (Brain Farting) is contagious ;) I need a vacation, a long one (and you
too so maybe OSDL will hire someone else meanwhile :)



- Davide

Re: asm (lidt) question

[Jamie Lokier]

Davide Libenzi wrote:
> IMHO, since "var" is really an output parameter.

"var" is read, not written.
I think you are confusing "lidt" with "sidt".

Therefore

> __asm__ __volatile__("lidt %0": :"m" (var));

is the better choice, and that's why I wrote it in reboot.c :)

-- Jamie

Re: asm (lidt) question

[Davide Libenzi]

On Tue, 22 Jul 2003, Jamie Lokier wrote:

> Davide Libenzi wrote:
> > IMHO, since "var" is really an output parameter.
>
> "var" is read, not written.
> I think you are confusing "lidt" with "sidt".

Actually I don't even know what I was confusing, since L and S are not
there for nothing ;) And yes, the form with =m as input parameter should
be corrected, even if it generates the same code.



- Davide

Re: asm (lidt) question

[Richard B. Johnson]

On Tue, 22 Jul 2003, Davide Libenzi wrote:

> On Tue, 22 Jul 2003, Jamie Lokier wrote:
>
> > Davide Libenzi wrote:
> > > IMHO, since "var" is really an output parameter.
> >
> > "var" is read, not written.
> > I think you are confusing "lidt" with "sidt".
>
> Actually I don't even know what I was confusing, since L and S are not
> there for nothing ;) And yes, the form with =m as input parameter should
> be corrected, even if it generates the same code.
>
>
>
> - Davide

LIDT is "load interrupt descriptor table". SIDT is "store interrupt
descriptor table". Only SIDT modifies memory. LIDT reads from memory
and puts the result into a special CPU register, therefore doesn't
modify memory.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.20 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: asm (lidt) question

[Davide Libenzi]

On Tue, 22 Jul 2003, Richard B. Johnson wrote:

> LIDT is "load interrupt descriptor table". SIDT is "store interrupt
> descriptor table". Only SIDT modifies memory. LIDT reads from memory
> and puts the result into a special CPU register, therefore doesn't
> modify memory.

Indeed, that why this is not really correct :

__asm__ __volatile__("lidt %0": "=m" (var));

even if it generates the same code of :

__asm__ __volatile__("lidt %0": : "m" (var));



- Davide

Re: asm (lidt) question

[Davide Libenzi]

On Tue, 22 Jul 2003, Randy.Dunlap wrote:

> On Tue, 22 Jul 2003 10:31:37 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:
>
> | On Tue, 22 Jul 2003, Jamie Lokier wrote:
> |
> | > Davide Libenzi wrote:
> | > > IMHO, since "var" is really an output parameter.
> | >
> | > "var" is read, not written.
> | > I think you are confusing "lidt" with "sidt".
> |
> | Actually I don't even know what I was confusing, since L and S are not
> | there for nothing ;) And yes, the form with =m as input parameter should
> | be corrected, even if it generates the same code.
>
> Yes, less confusion is better, so here's a patch to use the
> same reasonable syntax in all places.
>
> Look OK?  Generates the same code, as Davide pointed out.

Yep, that's the right syntax (pls push to Andrew or Linus) ;)



- Davide

Re: asm (lidt) question

[Randy.Dunlap]

On Tue, 22 Jul 2003 10:31:37 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

| On Tue, 22 Jul 2003, Jamie Lokier wrote:
| 
| > Davide Libenzi wrote:
| > > IMHO, since "var" is really an output parameter.
| >
| > "var" is read, not written.
| > I think you are confusing "lidt" with "sidt".
| 
| Actually I don't even know what I was confusing, since L and S are not
| there for nothing ;) And yes, the form with =m as input parameter should
| be corrected, even if it generates the same code.

Yes, less confusion is better, so here's a patch to use the
same reasonable syntax in all places.

Look OK?  Generates the same code, as Davide pointed out.

--
~Randy


patch_name:	lidt_norm.patch
patch_version:	2003-07-22.11:47:21
author:		Randy.Dunlap <rddunlap@osdl.org>
description:	normalize lidt/lgdt syntax usage
product:	Linux
product_versions: 2.6.0-test1
diffstat:	=
 arch/i386/kernel/cpu/common.c |    4 ++--
 arch/i386/kernel/traps.c      |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)


diff -Naur ./arch/i386/kernel/cpu/common.c~lidt ./arch/i386/kernel/cpu/common.c
--- ./arch/i386/kernel/cpu/common.c~lidt	2003-07-13 20:29:29.000000000 -0700
+++ ./arch/i386/kernel/cpu/common.c	2003-07-22 11:41:05.000000000 -0700
@@ -480,8 +480,8 @@
 	 */
 	memcpy(thread->tls_array, cpu_gdt_table[cpu], GDT_ENTRY_TLS_ENTRIES * 8);
 
-	__asm__ __volatile__("lgdt %0": "=m" (cpu_gdt_descr[cpu]));
-	__asm__ __volatile__("lidt %0": "=m" (idt_descr));
+	__asm__ __volatile__("lgdt %0": :"m" (cpu_gdt_descr[cpu]));
+	__asm__ __volatile__("lidt %0": :"m" (idt_descr));
 
 	/*
 	 * Delete NT
diff -Naur ./arch/i386/kernel/traps.c~lidt ./arch/i386/kernel/traps.c
--- ./arch/i386/kernel/traps.c~lidt	2003-07-13 20:31:20.000000000 -0700
+++ ./arch/i386/kernel/traps.c	2003-07-22 11:39:31.000000000 -0700
@@ -780,7 +780,7 @@
 	 * it uses the read-only mapped virtual address.
 	 */
 	idt_descr.address = fix_to_virt(FIX_F00F_IDT);
-	__asm__ __volatile__("lidt %0": "=m" (idt_descr));
+	__asm__ __volatile__("lidt %0": :"m" (idt_descr));
 }
 #endif
 

Re: asm (lidt) question

[Randy.Dunlap]

On Tue, 22 Jul 2003 11:58:16 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

| > Yes, less confusion is better, so here's a patch to use the
| > same reasonable syntax in all places.
| >
| > Look OK?  Generates the same code, as Davide pointed out.
| 
| Yep, that's the right syntax (pls push to Andrew or Linus) ;)
| 

Yes, planning to.

~Randy