search

dreamos82 / Dreamos64

My experiments with osdev... again
165 stars 8 forks source link

Possible error in ioapic.c #223

Open AntoninRuan opened 5 days ago

AntoninRuan commented 5 days ago

Hi man, i am currently reading through your notes on osdev (it is really well done thank you for that).

And i was actually reading the keyboard interrupt chapter in order to implement it myself also using ioapic. And after reading the ioapic redirection table documentation. I was having a bit of trouble finding how to determine how some bit were supposed to be set, so i decided to read through your implentation hoping to find more informations. Sadly I didn't find what I was searching for but while searching I think I found an error in your code.

If I am not mistaken here your are reading bit 2..4 of an Interrupt Override Structure from the MADT, that are the trigger mode flags but you are writing it the the polarity flag of your entry (flag which you already set a few line above) https://github.com/dreamos82/Dreamos64/blob/137159a7baace4947239ba05474afaf3a81e9f8c/src/kernel/arch/x86_64/cpu/ioapic.c#L136-L140

I don't know if you really expect issues from other people in this repo, but I thought I'd let you know, if it is indeed an error it may avoid you strange bug in the future.

dreamos82 commented 4 days ago

Hey @AntoninRuan first of all thanks for reading the notes, and secondly thanks for this issue .

And i was actually reading the keyboard interrupt chapter in order to implement it myself also using ioapic. And after reading the ioapic redirection table documentation. I was having a bit of trouble finding how to determine how some bit were supposed to be set,

So do you think that this part of the notes should be improved? (i need to read it again, do you mind to link the part that is not clear here?) What are the bits that are not clear?

If I am not mistaken here your are reading bit 2..4 of an Interrupt Override Structure from the MADT, that are the trigger mode flags but you are writing it the the polarity flag of your entry (flag which you already set a few line above)

That is entirely possible that i made a mistake (i recently found out a function that was returning the opposite of what it was supposed to return! :) ).

It took me nearly one hour to figure out what i did more than 2 years ago lol, i also had to go again through the IOApic documentation, because i mostly forgot how it works. So yeah apparently that code is a bug, i will double check, and fix it (you can create a PR eventually, and I can merge it once i confirmed everything is working).

Thanks for reporting it.

And Yeah I'm happy if other people create issues and even contribute if they want, so don't worry. For me it means that someone is interested in my code (although probably there are much better and elegant solution than mine! :) )

AntoninRuan commented 4 days ago

So do you think that this part of the notes should be improved? (i need to read it again, do you mind to link the part that is not clear here?) What are the bits that are not clear?

I guess it depends on what you want to provides with notes. For me the chapter was a good introduction to APIC and IOAPIC and it allowed me to then search for precise documentation and specifications (mainly the IOAPIC datasheet in this case). I then had trouble understanding the meaning of some of things in the datasheet and used your code to search for more informations.

Actually, I just reread the part on the apic and saw I missed the description of a LVT entry, so I guess here I may have benefited from more informations on what the Pin Polarity bit really do but considering the lack of informations given in the datasheet

This bit specifies the polarity of the interrupt signal. 0=High active, 1=Low active. You probably didn't have much to add.

As for the PR, I am still not sure to really understand what really happens and I rather test a bit more on my side before proposing any change.

dreamos82 commented 3 days ago

Actually, I just reread the part on the apic and saw I missed the description of a LVT entry, so I guess here I may have benefited from more informations on what the Pin Polarity bit really do but considering the lack of informations given in the datasheet

Yeah, i noticed that too, on a quick read, the fact tht part of a IOREDTBL entry is an LVT entry is totally missable (i did miss it too, when reading the docuemntation, only on a second read i found it out) i will try to find a way to make it more clear.

Thanks again for reporting this, and i will fix the bug soon.