Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH] x86/idt: Keep spurious entries unset in system_vectors | Date | Tue, 28 Apr 2020 08:43:23 +0200 |
| |
Thomas Gleixner <tglx@linutronix.de> writes:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes: >> Commit dc20b2d52653 ("x86/idt: Move interrupt gate initialization to IDT >> code") had a side-effect of 'set_bit(i, used_vectors)' for unused entries > > That was not a side effect. That was intentional. > >> which are being mapped to spurious entries. (user_vectors were later > > user_vectors?
*used*
> >> renamed to system_vectors). >> >> Previously, we used to count on system_vectors in arch_show_interrupts() > > We used to? Maybe you used to. I did not.
I was referring to
commit 9d87cd61a6b71ee00b7576a3ebc10208becdbea1 Author: Vitaly Kuznetsov <vkuznets@redhat.com> Date: Tue Jul 7 18:26:13 2015 +0200
x86/irq: Hide 'HYP:' line in /proc/interrupts when not on Xen/Hyper-V
and right you are, it lacks your Signed-off-by :-)
> >> to not print unexisting entries in /proc/interrupts. E.g. 'Hypervisor >> callback interrupts' should not be printed on bare metal. This is >> currently broken. > > That was definitely unintended, but that's a purely cosmetic issue. > >> Setting bits in system_vectors for all unused entries also makes >> alloc_intr_gate() fail in case someone decides to do it later. It seems >> this is not currently an issue because all alloc_intr_gate() users are >> calling it early, before we call idt_setup_apic_and_irq_gates() but >> this also seems wrong. > > No, that's not wrong. There is absolutely no reason to allocate an > interrupt gate later. > > The only thing what's wrong is that alloc_intr_gate() lacks an __init > annotation and a sanity check to reject attempts which come in late > after idt_setup_apic_and_irq_gates() was invoked. > > With that addressed we can remove the set_bit() for unused vectors to > cure the /proc/interrupts cosmetics. > > Talking about side effects. This also reflects the actual number of > system vectors which are assigned in the debugfs interface as that was > 'broken' as well.
Ok, I was only aiming at fixing the cosmetics but making alloc_intr_gate() __init seems to make sense too.
> > Thanks, > > tglx > > 8<-------------------- > arch/x86/kernel/idt.c | 20 ++++++++++++++++---- > drivers/xen/events/events_base.c | 24 +++++++++++++++--------- > 2 files changed, 31 insertions(+), 13 deletions(-) > > --- a/arch/x86/kernel/idt.c > +++ b/arch/x86/kernel/idt.c > @@ -51,6 +51,9 @@ struct idt_data { > #define TSKG(_vector, _gdt) \ > G(_vector, NULL, DEFAULT_STACK, GATE_TASK, DPL0, _gdt << 3) > > + > +static __initdata bool idt_setup_done; > + > /* > * Early traps running on the DEFAULT_STACK because the other interrupt > * stacks work only after cpu_init(). > @@ -318,11 +321,16 @@ void __init idt_setup_apic_and_irq_gates > > #ifdef CONFIG_X86_LOCAL_APIC > for_each_clear_bit_from(i, system_vectors, NR_VECTORS) { > - set_bit(i, system_vectors); > + /* > + * Don't set the non assigned system vectors in the > + * system_vectors bitmap. Otherwise they show up in > + * /proc/interrupts. > + */ > entry = spurious_entries_start + 8 * (i - FIRST_SYSTEM_VECTOR); > set_intr_gate(i, entry); > } > #endif > + idt_setup_done = true; > } > > /** > @@ -352,6 +360,7 @@ void idt_invalidate(void *addr) > load_idt(&idt); > } > > +/* This goes away once ASYNC_PF is sanitized */
Rumor has it that the work is ongoing...
> void __init update_intr_gate(unsigned int n, const void *addr) > { > if (WARN_ON_ONCE(!test_bit(n, system_vectors))) > @@ -359,9 +368,12 @@ void __init update_intr_gate(unsigned in > set_intr_gate(n, addr); > } > > -void alloc_intr_gate(unsigned int n, const void *addr) > +void __init alloc_intr_gate(unsigned int n, const void *addr) > { > - BUG_ON(n < FIRST_SYSTEM_VECTOR); > - if (!test_and_set_bit(n, system_vectors)) > + if (WARN_ON(n < FIRST_SYSTEM_VECTOR)) > + return; > + if (WARN_ON(idt_setup_done)) > + return; > + if (!WARN_ON(test_and_set_bit(n, system_vectors))) > set_intr_gate(n, addr); > } > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -1639,26 +1639,32 @@ EXPORT_SYMBOL_GPL(xen_set_callback_via); > /* Vector callbacks are better than PCI interrupts to receive event > * channel notifications because we can receive vector callbacks on any > * vcpu and we don't need PCI support or APIC interactions. */ > -void xen_callback_vector(void) > +static void xen_callback_vector(void) > { > - int rc; > uint64_t callback_via; > > if (xen_have_vector_callback) { > callback_via = HVM_CALLBACK_VECTOR(HYPERVISOR_CALLBACK_VECTOR); > - rc = xen_set_callback_via(callback_via); > - if (rc) { > + if (xen_set_callback_via(callback_via)) { > pr_err("Request for Xen HVM callback vector failed\n"); > xen_have_vector_callback = 0; > - return; > } > - pr_info_once("Xen HVM callback vector for event delivery is enabled\n"); > - alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, > - xen_hvm_callback_vector); > } > } > + > +static __init void xen_init_callback_vector(void) > +{ > + xen_callback_vector();
I don't quite like the 'xen_callback_vector()' name, it sounds like it is the irq handler but it's not. If we are to split the alloc_intr_gate() part I'd suggest the following:
xen_alloc_callback_vector() -> alloc_intr_gate() xen_setup_callback_vector() -> hypercall
Juergen, what do you think?
> + if (!xen_have_vector_callback) > + return; > + > + alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, xen_hvm_callback_vector); > + pr_info("Xen HVM callback vector for event delivery is enabled\n"); > +} > + > #else > void xen_callback_vector(void) {} > +static inline void xen_init_callback_vector(void) {} > #endif > > #undef MODULE_PARAM_PREFIX > @@ -1693,7 +1699,7 @@ void __init xen_init_IRQ(void) > pci_xen_initial_domain(); > } > if (xen_feature(XENFEAT_hvm_callback_vector)) > - xen_callback_vector(); > + xen_init_callback_vector(); > > if (xen_hvm_domain()) { > native_init_IRQ(); >
Ok, I'll split this into several patches, write 'to-be-massaged' changelogs and send v2 out. Thanks!
-- Vitaly
| |