feat(x86_64): real abstraction for ISA IRQs, unfuck I/O APIC #490
Conversation
|
ugh CI breakage is totally unrelated: https://github.com/hawkw/mycelium/actions/runs/12529896150/job/34945925042?pr=490 |
hal-x86_64/src/interrupt/apic/io.rs
Outdated
| match src_override.trigger_mode { | ||
| AcpiTriggerMode::Edge => trigger = TriggerMode::Edge, | ||
| AcpiTriggerMode::Level => trigger = TriggerMode::Level, | ||
| _ => {} |
There was a problem hiding this comment.
this should be an unreachable arm right? i think it's
mycelium/hal-x86_64/src/interrupt/apic.rs
Lines 27 to 30 in ba56bb4
There was a problem hiding this comment.
this is also a case of it being SameAsBus and me just picking some bullshit.
| match src_override.polarity { | ||
| AcpiPolarity::ActiveHigh => polarity = PinPolarity::High, | ||
| AcpiPolarity::ActiveLow => polarity = PinPolarity::Low, | ||
| AcpiPolarity::SameAsBus => {} |
There was a problem hiding this comment.
again very fuzzy on APIC configuration but this essentially reads to me as "if there is an override for this IRQ, its polarity is the same as the bus, which we've assumed to be high". but i guess, why would we assume active high by default, or at least why is that assumption right to hold here?
There was a problem hiding this comment.
you're going to hate my rationale for this, but it's "the previous code assumed all ISA interrupts are always active high and it worked okay."
there's probably a correct thing to do here where some ACPI table tells us what the native bus polarity is supposed to be, but i need to go figure it out.
There was a problem hiding this comment.
probably there should at least be a "TODO(eliza): when it says SameAsBus actually make it be the same as the bus"
There was a problem hiding this comment.
lmao i love computer. a todo would be good
There was a problem hiding this comment.
i added some comments in f2d5045 that i hope sufficiently communicate my shame about this :)
hal-x86_64/src/interrupt/apic/io.rs
Outdated
| let entry = RedirectionEntry::new() | ||
| .with(RedirectionEntry::DELIVERY, DeliveryMode::Normal) | ||
| .with(RedirectionEntry::POLARITY, polarity) | ||
| .with(RedirectionEntry::REMOTE_IRR, false) | ||
| .with(RedirectionEntry::TRIGGER, trigger) | ||
| .with(RedirectionEntry::MASKED, true) | ||
| .with(RedirectionEntry::DESTINATION, 0xff) | ||
| .with(RedirectionEntry::VECTOR, isa_base + irq as u8); |
There was a problem hiding this comment.
i kinda wonder if it'd be nice to move this out of the loop and just after contemplating MADT overrides, just to make the loop a bit more concise. then you'd just have mostly_finished_redirection_entry.with(RedirectionEntry::VECTOR, isa_base + irq as u8); here before apic.set_entry(...)
There was a problem hiding this comment.
yeah...and in fact, i specifically designed the bitfield builder API to let you do this, and then i didn't do it here because ???? reasons ?????
| let mut global_system_interrupt = irq as u8; | ||
| let mut polarity = PinPolarity::High; | ||
| let mut trigger = TriggerMode::Edge; | ||
| // Is there an override for this IRQ? If there is, clobber the |
There was a problem hiding this comment.
i'll admit that part of my thinking was hoping this didn't need to have the shape
// set some defaults
//
// put the defaults through an extruder that maybe messes with them
//
// now use whatever's left
or at least could do so in a more structured way, but with there being a SameAsBus polarity idk
| const END_INTERRUPT: u8 = 0x20; // from osdev wiki | ||
| if num >= self.sisters.little.address && num < self.sisters.little.address + 8 { |
Co-authored-by: iximeow <[email protected]>
Co-authored-by: iximeow <[email protected]>
Basically, the current way you interact with ISA standard PC interrupts
in the Mycelium x86_64 HAL is nonsensical, in several ways:
For some reason, we were mapping the I/O APIC ISA IRQs and the PIC
ISA IRQs to separate regions in the IDT, even though you can only ever
have either an I/O APIC or a PIC. Because of this, we were creating
separate entries for each ISA interrupt's ISR in the PIC region and the
I/O APIC region of the IDT. This doesn't actually break anything, but
it's stupid and dumb and I don't know why I did that.
I made an attempt to have an interface for controlling ISA interrupts
that abstracts over the I/O APIC and PIC interrupt models. But,
currently, you can't actually use this thing to do anything you might
want to do in an abstract way (mask/unmask/acknowledge an interrupt).
Instead, you have to actually inspect which interrupt model you have.
So, it's pointless.
Apparently the motherboard manufacturer gets to route ISA interrupts
to I/O APIC pins in "whatever random order they want to". Currently, we
hard-code the PS/2 keyboard and PIT timer interrupts to the vectors they
happen to be on when I run
qemu-system-x86_64 -cpu qemu64on mycomputer, but in real life, they could be anything. There's a thing in
the ACPI tables that tells you how these are routed, but we're not using
it. This is bad.
Also there's a bug where apparently when we unmask the PIT interrupt,
it might fire spuriously, even if we haven't actually done an
sti. Idon't get this and it seems wrong, but it results in the IRQ being
raised before configuring the static that tells you whether you have
the APIC or PIC interrupt model so when you try to actually send an EOI,
you dereference uninitialized memory and the kernel just hangs. This
happens occasionally when running in QEMU with the default settings, and
seems to happen consistently with
-machine accel=kvm, so that suggestsit is maybe worth worrying about.
This branch, uh, fixes all of that stuff. I made the abstraction for ISA
interrupts actually usable, and you can now mask/unmask/acknowledge them
from ISRs or anywhere else by interrupt name, instead of having to know what
interrupt model we have. Also, we now do the correct thing with regard to
MADT interrupt source overrides (read: we actually know about them), and I
fixed the spooky spurious interrupt behavior with the PIT by not unmasking it
until after the interrupt controller static is initialized.
Please clap.