Page 1 of 2

Can somebody tell me if this ASM is ok ?

Posted: Wed Oct 16, 2013 2:28 pm
by Luca91
I'm trying to implement the hardware interrupts (software interrupts works ok), but I get a double fault just after boot.. I'm starting to think that I've messed up something when trying to convert ASM from intel to AT&T (the guide that I'm using use intel asm and MSVC++, I'm using GCC instead).

-1st CODE-
original:

Code: Select all

unsigned char _cdecl inportb (unsigned short portid) {
#ifdef _MSC_VER
	_asm {
		mov		dx, word ptr [portid]
		in		al, dx
		mov		byte ptr [portid], al
	}
#endif
	return (unsigned char)portid;
}
my attempt:

Code: Select all

unsigned char inportb(unsigned short portid){

	__asm__(
		"movw %[portid],%%dx \n"
		"in %%dx,%%al \n"
		"movb %%al, %[portid] \n"
		: // no output
		: [portid] "m" (portid)  // input
		: "dx","al" //clobbers
	);

	return (unsigned char) portid;
}





2nd CODE:
original:

Code: Select all

void _cdecl outportb (unsigned short portid, unsigned char value) {
#ifdef _MSC_VER
	_asm {
		mov		al, byte ptr [value]
		mov		dx, word ptr [portid]
		out		dx, al
	}
#endif
}
my attempt:

Code: Select all

void outportb (unsigned short portid, unsigned char value){

	__asm__(
		"movb %[value], %%al \n"
		"movw %[portid], %%dx \n"
		"out %%al,%%dx \n"
		: // no output
		:[value] "m" (value), [portid] "m" (portid)
		: "al","dx"
	);
	
}

3rd CODE:
original:

Code: Select all

void _cdecl i86_pit_irq () {

	_asm add esp, 12
	_asm pushad

	//! increment tick count
	_pit_ticks++;

	//! tell hal we are done
	interruptdone(0);

	_asm popad
	_asm iretd
}
my attempt:

Code: Select all

void i86_pit_irq(){

	__asm__(
	"addl $12,%esp \n"
	"pusha \n"
	);
	
	_pit_ticks++;

	interruptdone(0);

	__asm__(
	"popa \n"
	"iret \n"
	);

}

4nd CODE: (ATTENTION: my attempt doesn't compile- luckily this is not a required function)
original:

Code: Select all

char* i86_cpu_get_vender () {

	static char	vender[32] = {0};

#ifdef _MSC_VER
	_asm {
		mov		eax, 0
		cpuid
		mov		dword ptr [vender], ebx
		mov		dword ptr [vender+4], edx
		mov		dword ptr [vender+8], ecx
	}
#endif

	return vender;
}
my attempt:

Code: Select all

char* i86_cpu_get_vender(){

	static char vendername[50] = {0};

 	__asm__(
		"movl $0,%%eax \n"
		"cpuid \n"
		"movl %%ebx, %[vendername] \n"
		"movl %%edx, %[vendername+$4] \n"
		"movl %%ecx, %[vendername+$8] \n"
		:"=r"(vendername) //user vendername as output
		:[vendername]"m"(vendername) //use vendername as input
		:"eax","ebx","edx","ecx" // clobbers those regs
	);

	return vendername;

}



Thank you very much for the support !

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Oct 17, 2013 6:25 am
by bluemoon
A quick scan spot two issue
1. in the function i86_pit_irq(), you have 2 separated block of assembly without side effect, the compiler is free to re-order them or even eliminate them. Furthermore, you should use the attribute hack to tell gcc that you are using iret in inline assembly.

2. vender should be spelled vendor.

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Oct 17, 2013 6:50 am
by Luca91

Code: Select all

 in the function i86_pit_irq(), you have 2 separated block of assembly without side effect, the compiler is free to re-order them or even eliminate them. Furthermore, you should use the attribute hack to tell gcc that you are using iret in inline assembly.
Thanks for this hint, can you tell me how can I do that ? or at least put me on the right direction ? Many thanks

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Oct 17, 2013 2:44 pm
by neon
Hello,

Please note that the MSVC implementation for i86_pit_irq is hackish and guaranteed to break on GCC. It is highly recommended to implement interrupt handlers in assembly language rather then C. Please reference this article for an example of this using GCC. It also includes example code that implements support for the PIT.

If you must only use GCC, perhaps this article might help. It introduces the GCC extensions __attribute__ ( ( signal, naked ) ). For example,

Code: Select all

/* your interrupt handler */

void i86_pit_irq (void) __attribute__ ( ( signal, naked ) );
void i86_pit_irq (void) {
   _pit_ticks++;
   interruptdone(0);
   asm("reti");
}

/* inportb and outportb */

unsigned char _cdecl inportb (unsigned short portid) {
    unsigned char ret;
    asm volatile( "inb %1, %0" : "=a"(ret) : "Nd"(portid) );
    return ret;
}

void _cdecl outportb (unsigned short portid, unsigned char value) {
    asm volatile( "outb %0, %1" : : "a"(value), "Nd"(portid) );
}

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Oct 17, 2013 5:17 pm
by Luca91
Thanks Mike for the reply.

Uhmmm it looks like that __attribute__ ( ( signal, naked ) ) is not supported by x86 GCC. Are you sure ?

Many thanks

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Oct 17, 2013 5:42 pm
by neon
Hello,

It would appear that you are correct. That is unfortunate; they should look into implementing support for it on x86. Unfortunately this means we have two options for ISR's:

1. Repair the stack. In the C function, you would need to view the disassembly and see how much is pushed on the stack in the generated epilog code of that function and pop them off before returning (this is what the "add esp, 12" does in the MSVC version of the routine; it pops 12 bytes off the stack that was pushed by the epilog code. For GCC, the number will be different. This is what results in triple fault.)

2. Assembly language. Most portable method and the recommended approach in the long run.

This might be an interesting article. It provides an alternative for writing ISR's in GCC. We do still recommend option (2) above though.

Perhaps someone here knows of an alternative way to write ISR's in C without using those attributes.

Re: Can somebody tell me if this ASM is ok ?

Posted: Fri Oct 18, 2013 2:08 am
by bluemoon
Check the wiki Interrupt_Service_Routines.
Yes I double check that attribute(IRQ) and interrupt handler related stuff for gcc does not support x86.
You either doing hack as inspired by the wiki, or do proper assembly.

In any case, gcc attribute extension, if it were supported, would not provide any better performance over "asm by hand" since there must be "glue code" anyway.

Re: Can somebody tell me if this ASM is ok ?

Posted: Fri Oct 18, 2013 5:17 am
by Luca91
Thanks to both for the reply.

Neon, you have some pm :wink:

Re: Can somebody tell me if this ASM is ok ?

Posted: Sat Oct 19, 2013 10:37 am
by sortie
Don't write interrupt handlers in C. It's that simple. You are begging for trouble otherwise - indeed the reason naked isn't supported on x86 is that a lot of people would abuse it. Indeed, if you wanted to emit assembly from C, why not just use a top-level assembly statement?

Code: Select all

#include <stdfoo.h>

asm (".global foo\n"
       ".type foo, @function\n"
       "foo:\n"
       "	ret\n"
       ".size foo, . - foo");

#if defined(__cplusplus)
extern "C"
#endif
void foo(void);

void bar()
{
    foo();
}

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Nov 07, 2013 12:27 pm
by Luca91
Sorry mates if I'm resurecting this old post, but I still haven't fixed this issue :(

According to Sortie's example I've written this code:

Code: Select all

asm (".global i86_pit_irq\n"
       ".type i86_pit_irq, @function\n"
       "i86_pit_irq:\n"
       "   pusha\n"                        
       "call  i86_pit_irq_from_c\n"
       "popa\n"
       "iret\n"
       ".size i86_pit_irq, . - i86_pit_irq");

void i86_pit_irq();

void i86_pit_irq_from_c(){
	_pit_ticks++;
	interruptdone(0);
}
Okay, now when an interrupt occurs, i86_pit_irq is called (the asm function), then this asm function, call the i86_pit_irq_from_c() function, and the it do popa and iret.
I've still the same error.. when I boot the Os I still receive the default handler exception..
Maybe I misunderstood Sortie's example..
Any help please ? I'm stuck ..

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Nov 07, 2013 5:12 pm
by Combuster
This is one of the reasons I avoid AT&T syntax - I can't see if PUSHA and IRET are actually encoded as PUSHADs and IRETDs, rather than their 16-bit variants. In particular because the latter case would break pretty much everything.


That said, the exact error which you're getting is not clear. Which exception (number) occurs at which exact instruction? What is the error code? Does Bochs print any error message (get it if you don't have it yet)? It might be just as well that the error is not actually caused by your code.

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Nov 07, 2013 5:18 pm
by Luca91
I dunno :(
I'm really confused at this point...
I've also tried to write the asm in another file like suggested by the wiki, but I still get the default handler exception..
I feel a bit lost ..
I think that I've to try to add a debug string..

EDIT: this is the pure asm file that I've written:

Code: Select all

global   i86_pit_irq
align 4

extern  i86_pit_irq_from_c

i86_pit_irq:
    pusha
    call     i86_pit_irq_from_c
    popa
    iret
compiled with: nasm -f elf ./Hal/i86_pit_irq.asm -o $build/i86_pit_irq.o
Can anybody confirm at least that this is right ?? thanks [-o<

EDIT2: Tomorrow I'll print mike's (Neon) kernel example from his guide and I'll check 1:1 with my code, in the hope that I can figure out what's wrong :x :( Also I found that alot of guides use to add a number to esp to fix the stack even if the use asm for irq handlers, why ?? :|

Re: Can somebody tell me if this ASM is ok ?

Posted: Thu Nov 07, 2013 9:56 pm
by neon
Hello,

The assembly file should be like this,

Code: Select all

global _i86_pit_irq
align 4

extern  _i86_pit_irq_from_c

_i86_pit_irq:
    pusha
    call     _i86_pit_irq_from_c
    popa
    iret
Then just link the resulting i86_pit_irq.o using your linker in the same way the object files produced by the C compiler are linked. The driver source would look something like this,

Code: Select all

#ifdef __cplusplus
extern "C" { /* don't need if using C. */
#endif

#define CDECL __cdecl

/* define as uint32_t to discourage direct calling. */
extern uint32_t i86_pit_irq;

void PitInitialize (void) {
   /* register interrupt handler. */
   IdtRegisterHandler (i86_pit_irq, 0);
}

void CDECL i86_pit_irq_from_c (void) {
   /* real interrupt handler. */
}

#ifdef __cplusplus
}
#endif

Re: Can somebody tell me if this ASM is ok ?

Posted: Fri Nov 08, 2013 9:26 am
by Luca91
I've used that code but it still give me default handler exception #-o
Any other clue ?

also accourding to your tutorial, this code:

Code: Select all

void PitInitialize (void) {
   /* register interrupt handler. */
   IdtRegisterHandler (i86_pit_irq, 0);
}
is:

Code: Select all

void i86_pit_initialize(){

	setvect(32,i86_pit_irq);
	_pit_isinit = 1;

}

Re: Can somebody tell me if this ASM is ok ?

Posted: Fri Nov 08, 2013 12:34 pm
by neon
Hello,

Sorry, the provided code was not tested. You are correct, for the series use the provided i86_pit_initialize. Also make sure it is stored at IDT entry 32 like it is in i86_pit_initialize. After this call, enabling hardware interrupts cannot result in a call to the default handler unless the IDT is not being properly updated or an invalid interrupt is actually being triggered.

We would typically use the Bochs debugger here. That is, you could just run the debugger with the c command until the handler is called; ctrl+c to break out and use the info idt command to validate that the IDT was properly updated and the entry is correct.

If you run it in Bochs, do you get any errors or warnings in the output log? How you are linking i86_pit_irq.o? Also, make sure to use the provided asm file - that is, keeping those underscore prefixes. Make sure the project is being built without errors or warnings.