diff mbox series

arch/x86/entry_fred: don't set up KVM IRQs if KVM is disabled

Message ID 20240215133631.136538-1-max.kellermann@ionos.com
State New
Headers
Series arch/x86/entry_fred: don't set up KVM IRQs if KVM is disabled |

Commit Message

Max Kellermann Feb. 15, 2024, 1:36 p.m. UTC 
  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(+)

Comments

Xin Li (Intel) Feb. 15, 2024, 6:23 p.m. UTC | #1 
On 2/15/2024 5:36 AM, 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;

Hmm, we did test against !CONFIG_KVM.

The POSTED_INTR_* macros are under CONFIG_HAVE_KVM, which is 'selected'
under CONFIG_X86.

Can you please send me your kernel config file?
Max Kellermann Feb. 15, 2024, 7:30 p.m. UTC | #2 
On Thu, Feb 15, 2024 at 7:23 PM Xin Li <xin@zytor.com> wrote:
> The POSTED_INTR_* macros are under CONFIG_HAVE_KVM, which is 'selected'
> under CONFIG_X86.

Sorry, I should have said that I found this bug on linux-next master,
which has this commit:

https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/dcf0926e9b899eca754a07c4064de69815b85a38%5E%21/

. which changes CONFIG_HAVE_KVM to CONFIG_KVM. I was not aware that
this commit hadn't been merged upstream yet.

You can easily reproduce with with "defconfig" plus CONFIG_X86_FRED
(on linux-next/master).

Max
Sean Christopherson Feb. 15, 2024, 7:55 p.m. UTC | #3 
+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
Xin Li (Intel) Feb. 16, 2024, 2:10 a.m. UTC | #4 
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).

diff --git a/arch/x86/include/asm/irq_vectors.h 
b/arch/x86/include/asm/irq_vectors.h
index 3a19904c2db6..d18bfb238f66 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -84,11 +84,9 @@
  #define HYPERVISOR_CALLBACK_VECTOR	0xf3

  /* Vector for KVM to deliver posted interrupt IPI */
-#if IS_ENABLED(CONFIG_KVM)
  #define POSTED_INTR_VECTOR		0xf2
  #define POSTED_INTR_WAKEUP_VECTOR	0xf1
  #define POSTED_INTR_NESTED_VECTOR	0xf0
-#endif

  #define MANAGED_IRQ_SHUTDOWN_VECTOR	0xef
Paolo Bonzini Feb. 16, 2024, 6:31 a.m. UTC | #5 
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), 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.

Paolo
Xin Li (Intel) Feb. 16, 2024, 5:41 p.m. UTC | #6 
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.

Thanks!
     Xin
Xin Li (Intel) Feb. 16, 2024, 5:47 p.m. UTC | #7 
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);             \
}
Thomas Gleixner Feb. 16, 2024, 9:45 p.m. UTC | #8 
On Fri, Feb 16 2024 at 07:31, Paolo Bonzini wrote:
> On 2/16/24 03:10, Xin Li wrote:
>
> It is intentional that KVM-related things are compiled out completely
> if !IS_ENABLED(CONFIG_KVM), because then it's also not necessary to
> have

That's a matter of taste. In both cases _ALL_ KVM related things are
compiled out.

#ifdeffing out the vector numbers is silly to begin with because these
vector numbers stay assigned to KVM whether KVM is enabled or not.

And no, I don't think it's a net win to have the #ifdeffery in that
table. Look at apic_idts[] in arch/x86/kernel/idt.c how this ends up
looking. It's unreadable gunk.

The few NULL defines in a header file next to the real stuff

#if IS_ENABLED(CONFIG_KVM)
....
#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

are not hurting at all and they are at a place where #ifdeffery is
required anyway. That's a very common pattern all over the kernel and it
limits the #ifdef horror to _ONE_ place.

With your change you propagate the #ifdefffery to the multiple and the
very wrong places for absolutely zero practical value. The resulting
binary code is exactly the same for the price of tasteless #ifdeffery in
places where it matters.

Please get rid of this #ifdef in the vector header and don't inflict
bad taste on everyone.

Thanks,

        tglx
Borislav Petkov Feb. 16, 2024, 9:46 p.m. UTC | #9 
+ Arnd for

https://lore.kernel.org/r/20240216202527.2493264-1-arnd@kernel.org

On Fri, Feb 16, 2024 at 07:31:46AM +0100, 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), 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.

Ok, I'll pick up Max' patch tomorrow and we must remember to tell Linus
during the merge window about this.

Thx.
Thomas Gleixner Feb. 16, 2024, 10:29 p.m. UTC | #10 
On Fri, Feb 16 2024 at 22:46, Borislav Petkov wrote:
> On Fri, Feb 16, 2024 at 07:31:46AM +0100, Paolo Bonzini wrote:
>> 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.
>
> Ok, I'll pick up Max' patch tomorrow and we must remember to tell Linus
> during the merge window about this.

No. Don't.

This pointless #ifdeffery in the vector header needs to vanish from the
KVM tree.

Why would you take the #ifdef mess into tasteful code just because
someone decided that #ifdeffing out constants in a header which is
maintained by other people is a brilliant idea?

The #ifdeffery in the idtentry header is unavoidable and the extra NULL
defines are at the right place and not making the actual code
unreadable.

Thanks,

        tglx
Max Kellermann Feb. 16, 2024, 11 p.m. UTC | #11 
On Fri, Feb 16, 2024 at 10:45 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> #ifdeffing out the vector numbers is silly to begin with because these
> vector numbers stay assigned to KVM whether KVM is enabled or not.

There could be one non-silly use of this: if the macros are not
defined in absence of the feature, any use of it will lead to a
compiler error, which is good, because it may reveal certain kinds of
bugs.
(Though I agree that this isn't worth the code ugliness. I prefer to
avoid the preprocessor whenever possible. I hate how much the kernel
uses macros instead of inline functions.)
Thomas Gleixner Feb. 17, 2024, 12:11 a.m. UTC | #12 
On Sat, Feb 17 2024 at 00:00, Max Kellermann wrote:
> On Fri, Feb 16, 2024 at 10:45 PM Thomas Gleixner <tglx@linutronixde> wrote:
>> #ifdeffing out the vector numbers is silly to begin with because these
>> vector numbers stay assigned to KVM whether KVM is enabled or not.
>
> There could be one non-silly use of this: if the macros are not
> defined in absence of the feature, any use of it will lead to a
> compiler error, which is good, because it may reveal certain kinds of
> bugs.

I generally agree with this sentiment, but for constants like those in
the case at hand I really draw the line.

> (Though I agree that this isn't worth the code ugliness. I prefer to
> avoid the preprocessor whenever possible. I hate how much the kernel
> uses macros instead of inline functions.)

No argument about that. I'm urging people to use inlines instead of
macros where ever possible, but there are things which can only solved
by macros.

I'm well aware that I wrote some of the more ugly ones myself. Though
the end justifies the means. If the ugly macro from hell which you
verify once safes you from the horrors of copy & pasta error hell then
they are making the code better and there are plenty of options to make
them reasonably (type) safe if done right.

Thanks,

        tglx
Paolo Bonzini Feb. 17, 2024, 9:52 a.m. UTC | #13 
On Fri, Feb 16, 2024 at 10:45 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Fri, Feb 16 2024 at 07:31, Paolo Bonzini wrote:
> > On 2/16/24 03:10, Xin Li wrote:
> >
> > It is intentional that KVM-related things are compiled out completely
> > if !IS_ENABLED(CONFIG_KVM), because then it's also not necessary to
> > have
>
> That's a matter of taste. In both cases _ALL_ KVM related things are
> compiled out.
>
> #ifdeffing out the vector numbers is silly to begin with because these
> vector numbers stay assigned to KVM whether KVM is enabled or not.

No problem---it seems that I misunderstood or read too much into the
usage of CONFIG_HAVE_KVM up to 6.8, so I'm happy to follow whatever
FRED support did for thermal vector and the like, and remove the
#ifdef for the vector numbers.

Paolo
Thomas Gleixner Feb. 17, 2024, 10:25 p.m. UTC | #14 
On Sat, Feb 17 2024 at 10:52, Paolo Bonzini wrote:
> On Fri, Feb 16, 2024 at 10:45 PM Thomas Gleixner <tglx@linutronixde> wrote:
>> #ifdeffing out the vector numbers is silly to begin with because these
>> vector numbers stay assigned to KVM whether KVM is enabled or not.
>
> No problem---it seems that I misunderstood or read too much into the
> usage of CONFIG_HAVE_KVM up to 6.8, so I'm happy to follow whatever
> FRED support did for thermal vector and the like, and remove the
> #ifdef for the vector numbers.

Thank you! Appreciated!

      tglx
diff mbox series

Patch

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;