Re: [PATCH v2 03/15] Drivers: hv: Move vmbus_handler to common code

[Naman Jain <namjain@linux.microsoft.com>]

On 4/27/2026 11:08 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, April 23, 2026 5:42 AM
>>
>> Move the vmbus_handler global variable and hv_setup_vmbus_handler()/
>> hv_remove_vmbus_handler() from arch/x86 to drivers/hv/hv_common.c.
>>
>> hv_setup_vmbus_handler() is called unconditionally in vmbus_bus_init()
>> and works for both x86 (sysvec handler) and arm64 (vmbus_percpu_isr).
>>
>> This eliminates the need for separate percpu vmbus handler setup
>> functions and __weak stubs, that are needed for adding ARM64 support
>> in MSHV_VTL driver where we need to set a custom per-cpu vmbus handler.
>>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   arch/x86/kernel/cpu/mshyperv.c | 12 ------------
>>   drivers/hv/hv_common.c         |  9 +++++++--
>>   drivers/hv/vmbus_drv.c         | 17 +++++++++--------
>>   include/asm-generic/mshyperv.h |  1 +
>>   4 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
>> index 89a2eb8a0722..68706ff5880e 100644
>> --- a/arch/x86/kernel/cpu/mshyperv.c
>> +++ b/arch/x86/kernel/cpu/mshyperv.c
>> @@ -145,7 +145,6 @@ void hv_set_msr(unsigned int reg, u64 value)
>>   EXPORT_SYMBOL_GPL(hv_set_msr);
>>
>>   static void (*mshv_handler)(void);
>> -static void (*vmbus_handler)(void);
>>   static void (*hv_stimer0_handler)(void);
>>   static void (*hv_kexec_handler)(void);
>>   static void (*hv_crash_handler)(struct pt_regs *regs);
>> @@ -172,17 +171,6 @@ void hv_setup_mshv_handler(void (*handler)(void))
>>   	mshv_handler = handler;
>>   }
>>
>> -void hv_setup_vmbus_handler(void (*handler)(void))
>> -{
>> -	vmbus_handler = handler;
>> -}
>> -
>> -void hv_remove_vmbus_handler(void)
>> -{
>> -	/* We have no way to deallocate the interrupt gate */
>> -	vmbus_handler = NULL;
>> -}
>> -
>>   /*
>>    * Routines to do per-architecture handling of stimer0
>>    * interrupts when in Direct Mode
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index e8633bc51d56..eb7b0028b45d 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -758,13 +758,18 @@ bool __weak hv_isolation_type_tdx(void)
>>   }
>>   EXPORT_SYMBOL_GPL(hv_isolation_type_tdx);
>>
>> -void __weak hv_setup_vmbus_handler(void (*handler)(void))
>> +void (*vmbus_handler)(void);
>> +EXPORT_SYMBOL_GPL(vmbus_handler);
>> +
>> +void hv_setup_vmbus_handler(void (*handler)(void))
>>   {
>> +	vmbus_handler = handler;
>>   }
>>   EXPORT_SYMBOL_GPL(hv_setup_vmbus_handler);
>>
>> -void __weak hv_remove_vmbus_handler(void)
>> +void hv_remove_vmbus_handler(void)
>>   {
>> +	vmbus_handler = NULL;
>>   }
>>   EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
> 
> I'd suggest moving hv_setup_vmbus_handler() and
> hv_remove_vmbus_handler() above or below the group
> of __weak stubs in this source code file. There's a comment
> describing the purpose of these __weak functions, and
> intermixing these two functions that are no longer __weak
> produces something of a jumble.
> 

Acked.

>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index bc4fc1951ae1..052ca8b11cee 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -1415,7 +1415,8 @@ EXPORT_SYMBOL_FOR_MODULES(vmbus_isr, "mshv_vtl");
>>
>>   static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
>>   {
>> -	vmbus_isr();
>> +	if (vmbus_handler)
>> +		vmbus_handler();
> 
> Is it necessary to test vmbus_handler first? From what I can
> see, it is always set before the per-cpu interrupt is setup.

After the shuffle of hv_remove_vmbus_handler() and freeing the irq, it 
can be safely removed. When I was setting the vmbus_handler to NULL 
first, before freeing the IRQ, this was required.

> 
>>   	return IRQ_HANDLED;
>>   }
>>
>> @@ -1517,8 +1518,10 @@ static int vmbus_bus_init(void)
>>   		vmbus_irq_initialized = true;
>>   	}
>>
>> +	hv_setup_vmbus_handler(vmbus_isr);
>> +
>>   	if (vmbus_irq == -1) {
>> -		hv_setup_vmbus_handler(vmbus_isr);
>> +		/* x86: sysvec handler uses vmbus_handler directly */
>>   	} else {
>>   		ret = request_percpu_irq(vmbus_irq, vmbus_percpu_isr,
>>   				"Hyper-V VMbus", &vmbus_evt);
>> @@ -1553,9 +1556,8 @@ static int vmbus_bus_init(void)
>>   	return 0;
>>
>>   err_connect:
>> -	if (vmbus_irq == -1)
>> -		hv_remove_vmbus_handler();
>> -	else
>> +	hv_remove_vmbus_handler();
>> +	if (vmbus_irq != -1)
>>   		free_percpu_irq(vmbus_irq, &vmbus_evt);
> 
> These operations should be reordered so they are the inverse
> of how they are setup.  I.e., free_percpu_irq() first, then remove
> the VMBus handler. That's just good standard practice unless
> there's a specific reason to do the cleanup ordering differently. In
> fact, hv_remove_vmbus_handler() needs to be moved down
> to the err_setup label so it's done if request_percpu_irq()
> fails.


Acked. I will do the same for other hv_remove_vmbus_handler() as well.

> 
>>   err_setup:
>>   	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
>> @@ -3026,9 +3028,8 @@ static void __exit vmbus_exit(void)
>>   	vmbus_connection.conn_state = DISCONNECTED;
>>   	hv_stimer_global_cleanup();
>>   	vmbus_disconnect();
>> -	if (vmbus_irq == -1)
>> -		hv_remove_vmbus_handler();
>> -	else
>> +	hv_remove_vmbus_handler();
>> +	if (vmbus_irq != -1)
>>   		free_percpu_irq(vmbus_irq, &vmbus_evt);
> 
> Ordering should be changed here as well so it is the inverse
> of how things are set up.
> 
>>   	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
>>   		smpboot_unregister_percpu_thread(&vmbus_irq_threads);
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 2810aa05dc73..db183c8cfb95 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -179,6 +179,7 @@ static inline u64 hv_generate_guest_id(u64 kernel_version)
>>
>>   int hv_get_hypervisor_version(union hv_hypervisor_version_info *info);
>>
>> +extern void (*vmbus_handler)(void);
>>   void hv_setup_vmbus_handler(void (*handler)(void));
>>   void hv_remove_vmbus_handler(void);
>>   void hv_setup_stimer0_handler(void (*handler)(void));
>> --
>> 2.43.0
>>


Regards,
Naman