Messages in this thread | | | Date | Fri, 16 Feb 2024 09:47:24 -0800 | Subject | Re: [PATCH] arch/x86/entry_fred: don't set up KVM IRQs if KVM is disabled | From | Xin Li <> |
| |
On 2/16/2024 9:41 AM, Xin Li wrote: > On 2/15/2024 10:31 PM, Paolo Bonzini wrote: >> On 2/16/24 03:10, Xin Li wrote: >>> On 2/15/2024 11:55 AM, Sean Christopherson wrote: >>>> +Paolo and Stephen >>>> >>>> FYI, there's a build failure in -next due to a collision between >>>> kvm/next and >>>> tip/x86/fred. The above makes everything happy. >>>> >>>> On Thu, Feb 15, 2024, Max Kellermann wrote: >>>>> When KVM is disabled, the POSTED_INTR_* macros do not exist, and the >>>>> build fails. >>>>> >>>>> Fixes: 14619d912b65 ("x86/fred: FRED entry/exit and dispatch code") >>>>> Signed-off-by: Max Kellermann <max.kellermann@ionos.com> >>>>> --- >>>>> arch/x86/entry/entry_fred.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c >>>>> index ac120cbdaaf2..660b7f7f9a79 100644 >>>>> --- a/arch/x86/entry/entry_fred.c >>>>> +++ b/arch/x86/entry/entry_fred.c >>>>> @@ -114,9 +114,11 @@ static idtentry_t >>>>> sysvec_table[NR_SYSTEM_VECTORS] __ro_after_init = { >>>>> SYSVEC(IRQ_WORK_VECTOR, irq_work), >>>>> +#if IS_ENABLED(CONFIG_KVM) >>>>> SYSVEC(POSTED_INTR_VECTOR, kvm_posted_intr_ipi), >>>>> SYSVEC(POSTED_INTR_WAKEUP_VECTOR, >>>>> kvm_posted_intr_wakeup_ipi), >>>>> SYSVEC(POSTED_INTR_NESTED_VECTOR, >>>>> kvm_posted_intr_nested_ipi), >>>>> +#endif >>>>> }; >>>>> static bool fred_setup_done __initdata; >>>>> -- >>>>> 2.39.2 >>> >>> We want to minimize #ifdeffery (which is why we didn't add any to >>> sysvec_table[]), would it be better to simply remove "#if >>> IS_ENABLED(CONFIG_KVM)" around the the POSTED_INTR_* macros from the >>> Linux-next tree? >>> >>> BTW, kvm_posted_intr_*() are defined to NULL if !IS_ENABLED(CONFIG_KVM). >> >> It is intentional that KVM-related things are compiled out completely >> if !IS_ENABLED(CONFIG_KVM), > > In arch/x86/include/asm/irq_vectors.h, most vector definitions are not > under any #ifdeffery, e.g., THERMAL_APIC_VECTOR not under > CONFIG_X86_THERMAL_VECTOR and IRQ_WORK_VECTOR not under CONFIG_IRQ_WORK. > > We'd better make all of them consistent, and the question is that should > we add #ifdefs or not. > >> because then it's also not necessary to have >> >> # define fred_sysvec_kvm_posted_intr_ipi NULL >> # define fred_sysvec_kvm_posted_intr_wakeup_ipi NULL >> # define fred_sysvec_kvm_posted_intr_nested_ipi NULL >> >> in arch/x86/include/asm/idtentry.h. The full conflict resultion is >> >> diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c >> index ac120cbdaaf2..660b7f7f9a79 100644 >> --- a/arch/x86/entry/entry_fred.c >> +++ b/arch/x86/entry/entry_fred.c >> @@ -114,9 +114,11 @@ static idtentry_t sysvec_table[NR_SYSTEM_VECTORS] >> __ro_after_init = { >> >> SYSVEC(IRQ_WORK_VECTOR, irq_work), >> >> +#if IS_ENABLED(CONFIG_KVM) >> SYSVEC(POSTED_INTR_VECTOR, kvm_posted_intr_ipi), >> SYSVEC(POSTED_INTR_WAKEUP_VECTOR, kvm_posted_intr_wakeup_ipi), >> SYSVEC(POSTED_INTR_NESTED_VECTOR, kvm_posted_intr_nested_ipi), >> +#endif >> }; >> >> static bool fred_setup_done __initdata; >> diff --git a/arch/x86/include/asm/idtentry.h >> b/arch/x86/include/asm/idtentry.h >> index 749c7411d2f1..758f6a2838a8 100644 >> --- a/arch/x86/include/asm/idtentry.h >> +++ b/arch/x86/include/asm/idtentry.h >> @@ -745,10 +745,6 @@ DECLARE_IDTENTRY_SYSVEC(IRQ_WORK_VECTOR, >> sysvec_irq_work); >> DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_VECTOR, >> sysvec_kvm_posted_intr_ipi); >> DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_WAKEUP_VECTOR, >> sysvec_kvm_posted_intr_wakeup_ipi); >> DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_NESTED_VECTOR, >> sysvec_kvm_posted_intr_nested_ipi); >> -#else >> -# define fred_sysvec_kvm_posted_intr_ipi NULL >> -# define fred_sysvec_kvm_posted_intr_wakeup_ipi NULL >> -# define fred_sysvec_kvm_posted_intr_nested_ipi NULL >> #endif >> >> #if IS_ENABLED(CONFIG_HYPERV) >> >> and it seems to be a net improvement to me. The #ifs match in >> the .h and .c files, and there are no unnecessary initializers >> in the sysvec_table. >> > > I somehow get an impression that the x86 maintainers don't like #ifs in > the .c files, but I could be just wrong. >
Here is an example, but again my interpretation could just be wrong:
#ifdef CONFIG_X86_FRED void fred_install_sysvec(unsigned int vector, const idtentry_t function); #else static inline void fred_install_sysvec(unsigned int vector, const idtentry_t function) { } #endif
#define sysvec_install(vector, function) { \ if (cpu_feature_enabled(X86_FEATURE_FRED)) \ fred_install_sysvec(vector, function); \ else \ idt_install_sysvec(vector, asm_##function); \ }
| |