Re: [PATCH 03/11] Drivers: hv: Add support to setup percpu vmbus handler

[Naman Jain]

On 4/1/2026 10:25 PM, Michael Kelley wrote:
> From: Naman Jain <namjain@xxxxxxxxxxxxxxxxxxx> Sent: Monday, March 16, 2026 5:13 AM
> > Add a wrapper function - hv_setup_percpu_vmbus_handler(), similar to
> > hv_setup_vmbus_handler() to allow setting up custom per-cpu VMBus
> > interrupt handler. This is required for arm64 support, to be added
> > in MSHV_VTL driver, where per-cpu VMBus interrupt handler will be
> > set to mshv_vtl_vmbus_isr() for VTL2 (Virtual Trust Level 2).
>
> Needing both hv_setup_vmbus_handler() and
> hv_setup_percpu_vmbus_handler() seems unfortunate. Here's an
> alternate approach to consider:
>
> 1. I think the x86 VMBus sysvec handler and the vmbus_percpu_isr()
> functions could both use the same vmbus_handler global variable.
> Looking at your changes in this patch set, hv_setup_vmbus_handler()
> and hv_setup_percpu_vmbus_handler() are used together and always
> set the same value.
>
> 2. So move the global variable vmbus_handler out from arch/x86
> and into hv_common.c, and export it. The x86 sysvec handler can
> still reference it, and vmbus_percpu_isr() in vmbus_drv.c can
> also reference it.  No need to have vmbus_percpu_isr() under
> arch/arm64 or have a stub in hv_common.c.
>
> 3. hv_setup_vmbus_handler() and hv_remove_vmbus_handler()
> also move to hv_common.c.  The __weak stubs go away.
>
> With these changes, only hv_setup_vmbus_handler() needs to
> be called, and it works for both x86 with the sysvec handler and
> for arm64 with vmbus_percpu_isr().
>
> I haven't coded this up, so maybe there's some problematic detail,
> but the idea seems like it would work. If it does work, some of my
> comments below are no longer applicable.

This is a great suggestion. Current implementation looked complex in 
design and it was becoming more complex with the changes I was making 
while addressing Sashiko's AI review comments. However your suggestion 
looks much better. I'll implement it. Thanks for suggesting.

> > Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Naman Jain <namjain@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   arch/arm64/hyperv/mshyperv.c   | 13 +++++++++++++
> >   drivers/hv/hv_common.c         | 11 +++++++++++
> >   drivers/hv/vmbus_drv.c         |  7 +------
> >   include/asm-generic/mshyperv.h |  3 +++
> >   4 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> > index 4fdc26ade1d7..d4494ceeaad0 100644
> > --- a/arch/arm64/hyperv/mshyperv.c
> > +++ b/arch/arm64/hyperv/mshyperv.c
> > @@ -134,3 +134,16 @@ bool hv_is_hyperv_initialized(void)
> >   	return hyperv_initialized;
> >   }
> >   EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
> > +
> > +static void (*vmbus_percpu_handler)(void);
> > +void hv_setup_percpu_vmbus_handler(void (*handler)(void))
> > +{
> > +	vmbus_percpu_handler = handler;
> > +}
> > +
> > +irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> > +{
> > +	if (vmbus_percpu_handler)
> > +		vmbus_percpu_handler();
> > +	return IRQ_HANDLED;
> > +}
> > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > index d1ebc0ebd08f..a5064f558bf6 100644
> > --- a/drivers/hv/hv_common.c
> > +++ b/drivers/hv/hv_common.c
> > @@ -759,6 +759,17 @@ void __weak hv_setup_vmbus_handler(void (*handler)(void))
> >   }
> >   EXPORT_SYMBOL_GPL(hv_setup_vmbus_handler);
> >
> > +irqreturn_t __weak vmbus_percpu_isr(int irq, void *dev_id)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> > +EXPORT_SYMBOL_GPL(vmbus_percpu_isr);
> > +
> > +void __weak hv_setup_percpu_vmbus_handler(void (*handler)(void))
> > +{
> > +}
> > +EXPORT_SYMBOL_GPL(hv_setup_percpu_vmbus_handler);
>
> You've implemented hv_setup_percpu_vmbus_handler() following
> the pattern of hv_setup_vmbus_handler(), which is reasonable.
> But that turns out to be unnecessarily complicated. The existing
> hv_setup_vmbus_handler() has a portion in
> arch/x86/kernel/cpu/mshyperv.c as a special case because it uses a
> hard-coded interrupt vector on x86/x64, and has its own custom
> sysvec code. And there's a need for a __weak stub in hv_common.c
> so that vmbus_drv.c will compile on arm64.
>
> But hv_setup_percpu_vmbus_handler() does not have the same
> requirements. It could be implemented entirely in vmbus_drv.c,
> with no code under arch/x86 or arch/arm64, and no __weak stub
> in hv_common.c.  vmbus_drv.c would just need to
> EXPORT_SYMBOL_FOR_MODULES, like it already does with vmbus_isr.
> I didn't code it up, but I think that approach would be simpler with
> fewer piece-parts scattered all over. If so, it would be worth
> breaking the symmetry with hv_setup_vmbus_handler().

No longer applicable.

Regards,
Naman