| Message ID | 20230410081438.1750-6-xin3.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: enable FRED for x86-64 | expand |
On Mon, Apr 10 2023 at 01:14, Xin Li wrote: > Add external_interrupt() to dispatch external interrupts to their handlers. > > If an external interrupt is a system interrupt, dipatch it through > system_interrupt_handlers table, otherwise to > dispatch_common_interrupt(). This naming convention sucks. external interrupts which can be system interrupts. Come on. > +/* > + * External interrupt dispatch function. > + * > + * Until/unless dispatch_common_interrupt() can be taught to deal with the > + * special system vectors, split the dispatch. More gibberish. It's not useful to add your sloppy personal notes which are not understandable for anyone else. That comment might eventually make some sense right in the code where the condition is. > + * Note: dispatch_common_interrupt() already deals with IRQ_MOVE_CLEANUP_VECTOR. > + */ > +int external_interrupt(struct pt_regs *regs) > +{ > + unsigned int vector = regs->vector; > + unsigned int sysvec = vector - FIRST_SYSTEM_VECTOR; > + > + if (vector < FIRST_EXTERNAL_VECTOR) { unlikely(...) > + pr_err("invalid external interrupt vector %d\n", vector); > + return -EINVAL; > + } > + > + if (sysvec < NR_SYSTEM_VECTORS) > + system_interrupt_handlers[sysvec](regs); > + else > + dispatch_common_interrupt(regs, vector); How is this supposed to work once the vector space gets extended in a later version of FRED? Can we please think about this _now_ and not rewrite all of this two years down the road? Even if that's not fully specified yet, here is the obvious question: What are we going to do with the system vectors. Are they going to stay just in the middle of the expanded vector space? That would be completely non-sensical as we'd end up with yet another segmentation of the vector space. So the obvious solution is to segment the vector space in the following way: 0 - 31 Exceptions/traps - Cannot be moved 32 IRQ_MOVE_CLEANUP_VECTOR 33 - X System vectors including APIC_SPURIOUS X+1 - MAX External interrupts This spares the IRQ_MOVE_CLEANUP_VECTOR hackery. It requires to move the ISA vectors, but that's not rocket science. That makes the external interrupt vector space trivially expandable, no? Coming back to that comment: > + * Until/unless dispatch_common_interrupt() can be taught to deal with the > + * special system vectors, split the dispatch. That's simply wishful thinking. Because all what this would achieve is moving the condition and table lookup into dispatch_common_interrupt(). What's the win aside of convoluted code? There are only two options to deal with that: 1) Have the condition in external_interrupt() if (unlikely(vector < LEGACY_MAX_EXCEPTION_VECTORS)) yell_and_bail(); if (vector < FIRST_DEVICE_VECTOR) sysvec_handler[vector](regs) else fred_common_interrupt(regs, vector); 2) Make the sysvec_handler[] fred wrapper functions take the vector argument and allocate the sysvec_handler[] array dynamically sized based on the maximum number of supported vectors in a particular [FRED]APIC implementation. Not sure whether that's worth it as FRED allows up to 64k vectors if I understand the reserved space layout in the spec correctly and that would cost 512k memory just for the table. Why has all of the above not been thought through and addressed _before_ posting this pile? <Kernel development educational template #11> 1) Implementing a Prove of Concept for early validation is perfectly fine. 2) Trying to sell that PoC upstream by polishing it a bit is doomed. 3) The proper tools for upstream development are brain and taste, not duct tape and stapler. </Kernel development educational template #11> Thanks, tglx
On Mon, Jun 05 2023 at 13:56, Thomas Gleixner wrote: > On Mon, Apr 10 2023 at 01:14, Xin Li wrote: > How is this supposed to work once the vector space gets extended in a > later version of FRED? > > Can we please think about this _now_ and not rewrite all of this two > years down the road? > > Even if that's not fully specified yet, here is the obvious question: > > What are we going to do with the system vectors. Are they going to > stay just in the middle of the expanded vector space? > > That would be completely non-sensical as we'd end up with yet another > segmentation of the vector space. > > So the obvious solution is to segment the vector space in the following > way: > > 0 - 31 Exceptions/traps - Cannot be moved > 32 IRQ_MOVE_CLEANUP_VECTOR > 33 - X System vectors including APIC_SPURIOUS > X+1 - MAX External interrupts > > This spares the IRQ_MOVE_CLEANUP_VECTOR hackery. It requires to move the > ISA vectors, but that's not rocket science. Which we just discussed completely away. :) > That makes the external interrupt vector space trivially expandable, no? So there is a theoretical problem here that device interrupts could starve system vectors due to the priority scheme. Needs some thought. That whole APIC priority muck is pretty useless as long as CR8 writes are slower than sti/cli. When I tested that last (Broadwell) they were significantly slower. Also it's unclear how that expansion vector space is handled vs. priorities. Ideally event delivery would be FIFO because that's the only guarantee for preventing starvation without having to configure priorities (which is mostly a wrong guess anyway). Thanks, tglx
> > Add external_interrupt() to dispatch external interrupts to their handlers. > > > > If an external interrupt is a system interrupt, dipatch it through > > system_interrupt_handlers table, otherwise to > > dispatch_common_interrupt(). > > This naming convention sucks. external interrupts which can be system > interrupts. Come on. This name dispatch_common_interrupt() comes from arch/x86/kernel/irq.c: /* * common_interrupt() handles all normal device IRQ's (the special SMP * cross-CPU interrupts have their own entry points). */ DEFINE_IDTENTRY_IRQ(common_interrupt) Should we rename it to device_intertupt()? Thanks! Xin
On Mon, Jun 19 2023 at 19:16, Li, Xin3 wrote: >> > Add external_interrupt() to dispatch external interrupts to their handlers. >> > >> > If an external interrupt is a system interrupt, dipatch it through >> > system_interrupt_handlers table, otherwise to >> > dispatch_common_interrupt(). >> >> This naming convention sucks. external interrupts which can be system >> interrupts. Come on. > > This name dispatch_common_interrupt() comes from arch/x86/kernel/irq.c: That's not the point. Your changelog says: If an external interrupt is a system interrupt... It's either an external interrupt which goes through common_interrupt() or it is a system interrupt which goes through it's very own handler, no? Thanks, tglx
> That's not the point. Your changelog says: > > If an external interrupt is a system interrupt... > > It's either an external interrupt which goes through common_interrupt() > or it is a system interrupt which goes through it's very own handler, > no? Ah, then it looks more of a problem in the way how I described it. What I wanted to describe is the dispatch logic _inside_ the new function external_interrupt(), what about: external_interrupt() dispatches all external interrupts: it checks if an external interrupt is a system interrupt, if yes it dipatches it through the system_interrupt_handlers table, otherwise to dispatch_common_interrupt(). Thanks! Xin
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 12072e2af4a6..f86cd233b00b 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -1511,6 +1511,32 @@ static system_interrupt_handler system_interrupt_handlers[NR_SYSTEM_VECTORS] = { #undef SYSV +/* + * External interrupt dispatch function. + * + * Until/unless dispatch_common_interrupt() can be taught to deal with the + * special system vectors, split the dispatch. + * + * Note: dispatch_common_interrupt() already deals with IRQ_MOVE_CLEANUP_VECTOR. + */ +int external_interrupt(struct pt_regs *regs) +{ + unsigned int vector = regs->vector; + unsigned int sysvec = vector - FIRST_SYSTEM_VECTOR; + + if (vector < FIRST_EXTERNAL_VECTOR) { + pr_err("invalid external interrupt vector %d\n", vector); + return -EINVAL; + } + + if (sysvec < NR_SYSTEM_VECTORS) + system_interrupt_handlers[sysvec](regs); + else + dispatch_common_interrupt(regs, vector); + + return 0; +} + #endif /* CONFIG_X86_64 */ void __init install_system_interrupt_handler(unsigned int n, const void *asm_addr, const void *addr)