Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comments for "https://os.phil-opp.com/hardware-interrupts/" #480

Closed
utterances-bot opened this issue Oct 22, 2018 · 60 comments
Closed

Comments for "https://os.phil-opp.com/hardware-interrupts/" #480

utterances-bot opened this issue Oct 22, 2018 · 60 comments

Comments

@utterances-bot
Copy link

utterances-bot commented Oct 22, 2018

This is a general purpose comment thread for the “Hardware Interrupts” post.

Copy link

Hi, I'd be interested in writing the small scancode_set1 crate! You can reach me at trinioler at gmail.com if you want to continue the discussion out of band. :)

@phil-opp
Copy link
Owner

@ZerothLaw Awesome, I'll write you an email!

@phil-opp phil-opp changed the title https://os.phil-opp.com/hardware-interrupts/ Comments for "https://os.phil-opp.com/hardware-interrupts/" Oct 22, 2018
Copy link
Contributor

I’m following your tutorials, and (am currently) creating an API for device drivers. Thanks for the amazing work!

@phil-opp
Copy link
Owner

@rybot666 You're welcome! What are your plans for the API design?

Copy link
Contributor

I'm curious if you have any plans for how to do APIC, as it doesn't look like there is a crate for it.

@katietheqt-old
Copy link
Contributor

katietheqt-old commented Oct 25, 2018

@phil-opp I’m planning on using traits to implement it (I’ve got a Driver trait that holds info, and an InterruptHandler trait that allows drivers to register interrupt handlers). You can have a look, but the source code is a bit of a mess. The API for drivers is in master, because I forgot to make a PR. ;)

EDIT: Forgot the link

Copy link

thejpster commented Oct 26, 2018

I wrote a crate called pc-keyboard which handles raw access to a UK 105 key PS/2 keyboard generating scan code set 2. Happy for patches to make it work with set 1 and other keyboard layouts.

@phil-opp
Copy link
Owner

@thejpster Awesome! cc @ZerothLaw

@phil-opp
Copy link
Owner

@rybot666 Ah, so that devices can register and unregister for certain interrupts at runtime?

@phil-opp
Copy link
Owner

@donald-pinckney Nothing specific yet, just some vague ideas.

@katietheqt-old
Copy link
Contributor

@phil-opp Exactly. Currently (as in right now) trying to get rid of errors about not being able to send Fn(...) types across threads

@phil-opp
Copy link
Owner

@rybot666 I think you need to add the Send bound to your register function to allow only function types that can be safely sent across threads.

@katietheqt-old
Copy link
Contributor

@phil-opp Yea, I have and it seems to have fixed that. Although my current device isn't very good (an iPad and the Github editor).

@ZerothLaw
Copy link

I wrote a crate called pc-keyboard which handles raw access to a UK 105 key PS/2 keyboard generating scan code set 2. Happy for patches to make it work with set 1 and other keyboard layouts.

Hey @thejpster I'll see about sending you a PR with scancode set 1 sometime soon.

Copy link

npip99 commented Oct 29, 2018

"As already mentioned, the 8259 APIC has been superseded by the APIC"

Perhaps "8259 PIC" was intended here?

@phil-opp
Copy link
Owner

@npip99 Thanks, fixed in 68f8d23.

Copy link

Isn't bad that the pic8259_simple crate hasn't been updated in 2 years, considering that the owner itself said "I am not qualified to have written this crate"?

@phil-opp
Copy link
Owner

@GrayJack I understand your concerns, but the crate is rather small and I don't see anything problematic in the code. Also, I contacted the owner before I used the crate for this post, and they said that they're still willing to maintain this crate.

@JoshMcguigan
Copy link

There is a small mistake in the code examples in the implementation and enabling interrupts sections. The last code example under Implementation uses the PIC in main.rs, but does not show the import statement. The first code example under Enabling Interrupts shows the import statement as a new line.

phil-opp added a commit that referenced this issue Jan 20, 2019
@phil-opp
Copy link
Owner

@JoshMcguigan Thanks! Fixed in 2e16cc4.

@bIgBV
Copy link

bIgBV commented Jan 31, 2019

I am really enjoying this series. I'm currently working on adding a simply network layer utilizing virtio to xv6 and it's been really cool comparing the two implementations. I'm learning a ton from the in depth explanations as well since there in't a lot of information regarding this except for the osdev wiki and the intel manuals.

@phil-opp
Copy link
Owner

@bIgBV Great to hear that!

Copy link

As Rust beginner, this line if let Some(key) = key { in 'Interpreting the Scancodes' section is confusing to me, Some(key) seems like Some(Some('1') if scancode is 0x02. Could a different variable name be used in place of first key like if let Some(n) = key {? Or is it common to use same variable while pattern matching?

Copy link

a note for anyone using the latest version (0.5.0) of pc-keyboard, you'll need to make a change.

use pc_keyboard::{Keyboard, ScancodeSet1, DecodedKey, layouts, HandleControl }; // new

...

Mutex::new(Keyboard::new(layouts::Us104Key, ScancodeSet1, HandleControl::Ignore)); //new

which causes some odd behavior in the number pad (might be bug in pc-keyboard.

or

Mutex::new(Keyboard::new(layouts::Us104Key, ScancodeSet1, HandleControl::MapLettersToUnicode));

which works better, but now the control key is a modifier to be used with another key and returns unicode. which means you lose direct access to the control key.

@phil-opp
Copy link
Owner

@krsoninikhil

As Rust beginner, this line if let Some(key) = key { in 'Interpreting the Scancodes' section is confusing to me,

This feature is called pattern matching and is normally used in combination with a match expression and an enum. However, Rust also supports patterns any many other places, including the if let clause. The if let statement does the following:

  • Check whether the right side of the = sign matches the pattern on the left side
  • If not, skip the block (or execute the else clause if present)
  • If yes, destructure the right side and assign the variables used in the pattern, then execute the if block

You can use any variable name in the pattern, including the same variable name as used on the right side. If you use the same name, the original variable is shadowed and only the new variable can be used inside the block. For more information see the official documentation which includes a shadowed variable in the example (Ok(age) = age).

So to answer the second part of your question: Yes, a different variable name like n would work too (if let Some(n) = key). And yes, shadowing in such situations is somewhat common in Rust because it provides a nice way to keep using the original variable name without needing to unwrap it. But this is entirely personal preference, so if you prefer using a different variable name that's perfectly fine too.

I hope this answers your question! I opened issue #572 to add an explanation like the above to the post.

@krsoninikhil
Copy link

krsoninikhil commented Mar 28, 2019

I read about pattern matching and if let from official docs (here) and didn't notice about shadowing. Thanks for taking time to explain it. Adding one line about shadowing in post would definitely help.

@ethindp
Copy link

ethindp commented Jul 22, 2019 via email

@phil-opp
Copy link
Owner

@aufflick The notify_end_of_interrupt function only signals to the interrupt controller that we're finished handling the interrupt, so that it can send the next one when possible. However, interrupts stay disabled on the CPU by default until the interrupt handler returns. So even if the interrupt controller raises an interrupt right after the notify_end_of_interrupt call, the CPU would not start to handle it until the handler returns.

@ethindp We currently use only a single core and the legacy PIC, so no two interrupts can run at the same time. So a deadlock could occur if we the "keep interrupts enabled" flag for the IDT entry of the keyboard handler.

@ethindp
Copy link

ethindp commented Jul 22, 2019 via email

@phil-opp
Copy link
Owner

As long as we don't make the keyboard handler nested by reenabling interrupts before it is finished, I don't think it will cause issues. And there is no reason to do so because we want to handle keyboard input sequentially anyway.

@64
Copy link

64 commented Jul 22, 2019

There shouldn’t be a deadlock here even with SMP. If two interrupts happen on different CPUs at the same time, one will take the keyboard lock and the other will be forced to wait until it’s finished.

APIC has a similar EOI mechanism but uses MMIO instead of in/out, as the 8259 PIC does - it shouldn’t be that hard to replace. Basically the main thing you need to avoid is locking something twice from the same CPU.

As a side note, there’s really no reason for the PIC to take a lock here as far as I see it, it’s not mutating anything.

@phil-opp
Copy link
Owner

As a side note, there’s really no reason for the PIC to take a lock here as far as I see it, it’s not mutating anything.

It's mutating the internal state of the interrupt controller, isn't it?

@64
Copy link

64 commented Jul 22, 2019

Yes but it doesn’t need to - it treats writing to a port as a mutation. It’s not modifying any actual data.

@phil-opp
Copy link
Owner

I guess that a question of the mental model. What I meant is that the port write has a side effect, even without modifying any data. Before it, the internal state of the interrupt controller is that the interrupt is still processed. Afterwards, the interrupt controller is in a different internal state, namely that the handling of the interrupt has finished. For me this sounds like some internal register of the interrupt controller changes its value as a result of that port write.

@ethindp
Copy link

ethindp commented Jul 22, 2019 via email

@64
Copy link

64 commented Jul 22, 2019

There is no easy walkthrough that I’m aware of. OSDev wiki explains what you need to do to get the ACPI tables. Here’s a useful reference that helped me with SMP before: https://nemez.net/osdev/lapic.txt

Copy link

stuaxo commented Oct 29, 2019

This is great, maybe I can finally output a keyboard handler that handles chords.. ie when you hold down DOWN and RIGHT, it will continuously repeat DOWN then RIGHT so diagonal scrolling comes for free.

Copy link

Is there any reason to implement InterruptIndex::as_usize as usize::from(self.as_u8()) as opposed to self as usize?

@ethindp
Copy link

ethindp commented Jun 22, 2020

Getting the APIC and all of that working shouldn't be too difficult (the x86 crate lets us set up the XAPIC or X2APIC, as well as sending IPIs. I don't know how to tell the processors where to start execution though. And it doesn't help with ACPI either.

@phil-opp
Copy link
Owner

@JohnDowson Casting with as is more dangerous because it is also possible to accidentally truncate values. For example, consider 500u32 as u8, which will silently change the number. For this reason, it is generally recommend to use the From/Into traits if possible and the TryFrom/TryInto traits otherwise. This way, it is clear from the code whether a truncation is desired or not. Also, if you switch the integer types at some, the compiler will throw errors instead of silently introducing a potential truncation.

@phil-opp
Copy link
Owner

@ethindp I plan to look into the APIC more closely soon as part of adding UEFI support. I'll update the blog with some instructions then.

Copy link

I'm having an issue that as soon as I add the PIC initialize code and enable interrupts, the double fault message doesn't print fully. Instead, it seems to hang in the middle of the InterruptStackFrame structure. If I don't include the structure in the output, it prints the whole thing.

I cloned your blog_os repo and checkout out the post-07 branch, and if I induce a double fault there (by causing a stack overflow in _start), the system apparently triple faults (reboots endlessly), with again only a partial double fault message printed. There too not including the stack frame in the output causes the reboot loop to go away.

Any idea what could be causing this?

Copy link

SvenGroot commented Aug 14, 2020

Ok, I had a thought after noticing that this didn't happen when running the release profile. I tried increasing the size of the double fault handler's stack, and that solved the issue. It seems that when compiling without optimizations, the Debug formatter for InterruptStackFrame overflows the 4kb stack. What that ends up doing (hang vs. reboot loop) seems to depend on what's at the address it ends up accessing after that.

@phil-opp
Copy link
Owner

@SvenGroot Thanks for reporting! You're completely right that this is caused by a stack overflow on the double fault stack. See #449 (comment) for more information. I now pushed 0425bd3 and 817e36c to increase the stack size to 4096 * 5 bytes, which should prevent this issue.

Copy link

pitust commented Sep 3, 2020

I made a switch from ubuntu to arch linux, which gave me a much newer version of QEMU. and suddenly my code double-faults whenever i press a key. If I mask out the interrupt on the PIC the issue goes away although then I am unable to type anything (except in my serial console which still doesn't fire interrupts). This issue was present ever since I updated from the QEMU on ubuntu to the version of QEMU available on Arch Linux.

@phil-opp
Copy link
Owner

phil-opp commented Sep 4, 2020

Could you tell us what's your QEMU version now, so that we can try to reproduce the problem?

@ethindp
Copy link

ethindp commented Sep 4, 2020

The Qemu version that Arch Linux has is the latest version (5.1.0-1)

Copy link

dvjn commented Sep 4, 2020

Hey, in the Fixing a Race Condition section, by removing the println macro from the println macro's output test we are effectively not testing the println macro, if that makes any sense.

Also, according to what I have understood, we don't need to get the writer's lock manually, we only need to disable interrupts during the test.

So this should be enough:

#[test_case]
fn test_println_output() {
-    use core::fmt::Write;
    use x86_64::instructions::interrupts;

    let s = "Some test string that fits on a single line";
    interrupts::without_interrupts(|| {
-        let mut writer = WRITER.lock();
-        writeln!(writer, "\n{}", s).expect("writeln failed");
+        println!("\n{}", s);
        for (i, c) in s.chars().enumerate() {
            let screen_char = writer.buffer.chars[BUFFER_HEIGHT - 2][i].read();
            assert_eq!(char::from(screen_char.ascii_character), c);
        }
    });
}

By the way, great tutorials!
It's my first time writing any OS related code, but you made it easier!

@phil-opp
Copy link
Owner

@divykj Thanks for your comment (and sorry for the late reply!). You're completely right that we are no longer testing the println macro itself. However, leaving the WRITER temporarily unlocked between the println and the result checking is not a good idea either since this can result in race conditions with multi core processing. While we're only using a single CPU core right now, we will add support for running tests parallel on multi-core CPUs at some point, at which point this test would sometimes fail.

I'm currently in the process of rewriting the screen output code (pixel-based framebuffer instead of VGA text buffer), so the println implementation will change too at some point. I'll try to find a better solution for this test then.

Copy link

Hi. Thank you for this great tutorial series, your explanations are detailed and well written.
I did stumble upon something I don't quite understand though, it would be great to have an answer to what is happening even if I suspect it is a beginner rust question.

When trying to reproduce the deadlock by having the interrupt try to acquire the already locked Writer, I ran into something that confused me.
At first when I first ran it, it didn't deadlock, instead they would both write just fine.
I found the issue was that in main I had

mod vga_buffer;

rather that using

blog_os::print;

After changing this, the kernel deadlocks as expected.
But how is my first result possible? what is going on here, isn't main acquiring the same lock in both these cases?

@Qubasa
Copy link

Qubasa commented Oct 24, 2020

@phil-opp
Hi there,

I am trying to setup serial interrupts too. As far as I can see the uart_16550 crate already enables serial port interrupts and they should arrive at IRQ4 or IDT index 36, but none of it happens. Have you an idea why?
I run qemu with -serial pty and connected a serial console with baud rate 38400 to make sure the input really gets send to the kernel, but I still don't seem to get any irq4 interrupts :(
I also send more then 14 characters because of the FIFO

@phil-opp
Copy link
Owner

@krummelur I'm not quite sure if that's what you mean, but if you have mod vga_buffer in both your lib.rs and main.rs, you create the module twice. This means that two separate WRITER statics are created that can be both locked individually. They still access the same memory region, so this is a form of mutable aliasing (two &mut references to the same memory), which can lead to bugs and undefined behavior.

@phil-opp
Copy link
Owner

@luis-hebendanz I never set up serial interrupts so I don't have any idea what could be the problem, sorry. But please let us know if you figure it out!

@Qubasa
Copy link

Qubasa commented Nov 15, 2020

@phil-opp I got it to work turns out I had to unmask the interrupt in the pic controller by writing a 0 to port 0x21 at bit index 2.
This is very cool because if you start qemu with the -nographic flag you get a terminal with output and keyboard input without having to open another window :) (quit the qemu session with ctrl-a+x)

Repository owner locked and limited conversation to collaborators Jun 12, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests