[v5,08/10] x86: hyperv: Add mshv_handler irq handler and setup function

This will handle SYNIC interrupts such as intercepts, doorbells, and
scheduling messages intended for the mshv driver.

Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
Reviewed-by: Wei Liu <wei.liu@kernel.org>
Reviewed-by: Tianyu Lan <tiala@microsoft.com>
---
 arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
 drivers/hv/hv_common.c         | 5 +++++
 include/asm-generic/mshyperv.h | 1 +
 3 files changed, 15 insertions(+)

On Wed, Feb 26, 2025 at 03:08:02PM -0800, Nuno Das Neves wrote:
> This will handle SYNIC interrupts such as intercepts, doorbells, and
> scheduling messages intended for the mshv driver.
> 
> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> Reviewed-by: Wei Liu <wei.liu@kernel.org>
> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
>  drivers/hv/hv_common.c         | 5 +++++
>  include/asm-generic/mshyperv.h | 1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 0116d0e96ef9..616e9a5d77b4 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -107,6 +107,7 @@ 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);
> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>  	struct pt_regs *old_regs = set_irq_regs(regs);
>  
>  	inc_irq_stat(irq_hv_callback_count);
> +	if (mshv_handler)
> +		mshv_handler();

Can mshv_handler be defined as a weak symbol doing nothing instead
of defining it a null pointer?
This should allow to simplify this code and get rid of
hv_setup_mshv_handler, which looks redundant.

Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>

> +
>  	if (vmbus_handler)
>  		vmbus_handler();
>  
> @@ -126,6 +130,11 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>  	set_irq_regs(old_regs);
>  }
>  
> +void hv_setup_mshv_handler(void (*handler)(void))
> +{
> +	mshv_handler = handler;
> +}
> +
>  void hv_setup_vmbus_handler(void (*handler)(void))
>  {
>  	vmbus_handler = handler;
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 2763cb6d3678..f5a07fd9a03b 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -677,6 +677,11 @@ void __weak hv_remove_vmbus_handler(void)
>  }
>  EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
>  
> +void __weak hv_setup_mshv_handler(void (*handler)(void))
> +{
> +}
> +EXPORT_SYMBOL_GPL(hv_setup_mshv_handler);
> +
>  void __weak hv_setup_kexec_handler(void (*handler)(void))
>  {
>  }
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 1f46d19a16aa..a05f12e63ccd 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -208,6 +208,7 @@ void hv_setup_kexec_handler(void (*handler)(void));
>  void hv_remove_kexec_handler(void);
>  void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
>  void hv_remove_crash_handler(void);
> +void hv_setup_mshv_handler(void (*handler)(void));
>  
>  extern int vmbus_interrupt;
>  extern int vmbus_irq;
> -- 
> 2.34.1
>

On 2/26/2025 3:43 PM, Stanislav Kinsburskii wrote:
> On Wed, Feb 26, 2025 at 03:08:02PM -0800, Nuno Das Neves wrote:
>> This will handle SYNIC interrupts such as intercepts, doorbells, and
>> scheduling messages intended for the mshv driver.
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Reviewed-by: Wei Liu <wei.liu@kernel.org>
>> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
>> ---
>>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
>>  drivers/hv/hv_common.c         | 5 +++++
>>  include/asm-generic/mshyperv.h | 1 +
>>  3 files changed, 15 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
>> index 0116d0e96ef9..616e9a5d77b4 100644
>> --- a/arch/x86/kernel/cpu/mshyperv.c
>> +++ b/arch/x86/kernel/cpu/mshyperv.c
>> @@ -107,6 +107,7 @@ 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);
>> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>>  	struct pt_regs *old_regs = set_irq_regs(regs);
>>  
>>  	inc_irq_stat(irq_hv_callback_count);
>> +	if (mshv_handler)
>> +		mshv_handler();
> 
> Can mshv_handler be defined as a weak symbol doing nothing instead
> of defining it a null pointer?
> This should allow to simplify this code and get rid of
> hv_setup_mshv_handler, which looks redundant.
> 
Interesting, I tested this and it does seems to work! It seems like
a good change, thanks.

> Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> 
>> +
>>  	if (vmbus_handler)
>>  		vmbus_handler();
>>  
>> @@ -126,6 +130,11 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>>  	set_irq_regs(old_regs);
>>  }
>>  
>> +void hv_setup_mshv_handler(void (*handler)(void))
>> +{
>> +	mshv_handler = handler;
>> +}
>> +
>>  void hv_setup_vmbus_handler(void (*handler)(void))
>>  {
>>  	vmbus_handler = handler;
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index 2763cb6d3678..f5a07fd9a03b 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -677,6 +677,11 @@ void __weak hv_remove_vmbus_handler(void)
>>  }
>>  EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
>>  
>> +void __weak hv_setup_mshv_handler(void (*handler)(void))
>> +{
>> +}
>> +EXPORT_SYMBOL_GPL(hv_setup_mshv_handler);
>> +
>>  void __weak hv_setup_kexec_handler(void (*handler)(void))
>>  {
>>  }
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 1f46d19a16aa..a05f12e63ccd 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -208,6 +208,7 @@ void hv_setup_kexec_handler(void (*handler)(void));
>>  void hv_remove_kexec_handler(void);
>>  void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
>>  void hv_remove_crash_handler(void);
>> +void hv_setup_mshv_handler(void (*handler)(void));
>>  
>>  extern int vmbus_interrupt;
>>  extern int vmbus_irq;
>> -- 
>> 2.34.1
>>

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, February 28, 2025 4:38 PM
> 
> On 2/26/2025 3:43 PM, Stanislav Kinsburskii wrote:
> > On Wed, Feb 26, 2025 at 03:08:02PM -0800, Nuno Das Neves wrote:
> >> This will handle SYNIC interrupts such as intercepts, doorbells, and
> >> scheduling messages intended for the mshv driver.
> >>
> >> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> >> Reviewed-by: Wei Liu <wei.liu@kernel.org>
> >> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
> >> ---
> >>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
> >>  drivers/hv/hv_common.c         | 5 +++++
> >>  include/asm-generic/mshyperv.h | 1 +
> >>  3 files changed, 15 insertions(+)
> >>
> >> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> >> index 0116d0e96ef9..616e9a5d77b4 100644
> >> --- a/arch/x86/kernel/cpu/mshyperv.c
> >> +++ b/arch/x86/kernel/cpu/mshyperv.c
> >> @@ -107,6 +107,7 @@ 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);
> >> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
> >>  	struct pt_regs *old_regs = set_irq_regs(regs);
> >>
> >>  	inc_irq_stat(irq_hv_callback_count);
> >> +	if (mshv_handler)
> >> +		mshv_handler();
> >
> > Can mshv_handler be defined as a weak symbol doing nothing instead
> > of defining it a null pointer?
> > This should allow to simplify this code and get rid of
> > hv_setup_mshv_handler, which looks redundant.
> >
> Interesting, I tested this and it does seems to work! It seems like
> a good change, thanks.

Just be a bit careful. When CONFIG_HYPERV=n, mshyperv.c still gets
built even through none of the other Hyper-V related files do.  There
are #ifdef CONFIG_HYPERV in mshyperv.c to eliminate references to
Hyper-V files that wouldn't be built. I'd suggest doing a test build with
that configuration to make sure it's all clean.

Michael

> 
> > Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> >
> >> +
> >>  	if (vmbus_handler)
> >>  		vmbus_handler();
> >>
> >> @@ -126,6 +130,11 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
> >>  	set_irq_regs(old_regs);
> >>  }
> >>
> >> +void hv_setup_mshv_handler(void (*handler)(void))
> >> +{
> >> +	mshv_handler = handler;
> >> +}
> >> +
> >>  void hv_setup_vmbus_handler(void (*handler)(void))
> >>  {
> >>  	vmbus_handler = handler;
> >> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> >> index 2763cb6d3678..f5a07fd9a03b 100644
> >> --- a/drivers/hv/hv_common.c
> >> +++ b/drivers/hv/hv_common.c
> >> @@ -677,6 +677,11 @@ void __weak hv_remove_vmbus_handler(void)
> >>  }
> >>  EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
> >>
> >> +void __weak hv_setup_mshv_handler(void (*handler)(void))
> >> +{
> >> +}
> >> +EXPORT_SYMBOL_GPL(hv_setup_mshv_handler);
> >> +
> >>  void __weak hv_setup_kexec_handler(void (*handler)(void))
> >>  {
> >>  }
> >> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> >> index 1f46d19a16aa..a05f12e63ccd 100644
> >> --- a/include/asm-generic/mshyperv.h
> >> +++ b/include/asm-generic/mshyperv.h
> >> @@ -208,6 +208,7 @@ void hv_setup_kexec_handler(void (*handler)(void));
> >>  void hv_remove_kexec_handler(void);
> >>  void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
> >>  void hv_remove_crash_handler(void);
> >> +void hv_setup_mshv_handler(void (*handler)(void));
> >>
> >>  extern int vmbus_interrupt;
> >>  extern int vmbus_irq;
> >> --
> >> 2.34.1
> >>

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Wednesday, February 26, 2025 3:08 PM

> 
> This will handle SYNIC interrupts such as intercepts, doorbells, and
> scheduling messages intended for the mshv driver.

Could you provide a bit more detailed commit message? How does
the mshv_handler() relate to the vmbus_handler()? From the code
mshv_handler() goes first, and I'm assuming it processes what it
knows about (intercepts, doorbells, scheduling messages?) and
then hands off control to the vmbus_handler() to handle the usual
VMbus-related message and channel interrupts. But it would be
nice to have the commit message or code comments describe the
overall intent and any obscure aspects of the relationship.

And avoid references to "This" or "This patch". :-)

> 
> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> Reviewed-by: Wei Liu <wei.liu@kernel.org>
> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
>  drivers/hv/hv_common.c         | 5 +++++
>  include/asm-generic/mshyperv.h | 1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 0116d0e96ef9..616e9a5d77b4 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -107,6 +107,7 @@ 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);
> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>  	struct pt_regs *old_regs = set_irq_regs(regs);
> 
>  	inc_irq_stat(irq_hv_callback_count);
> +	if (mshv_handler)
> +		mshv_handler();
> +
>  	if (vmbus_handler)
>  		vmbus_handler();
> 
> @@ -126,6 +130,11 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>  	set_irq_regs(old_regs);
>  }
> 
> +void hv_setup_mshv_handler(void (*handler)(void))
> +{
> +	mshv_handler = handler;
> +}
> +
>  void hv_setup_vmbus_handler(void (*handler)(void))
>  {
>  	vmbus_handler = handler;
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 2763cb6d3678..f5a07fd9a03b 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -677,6 +677,11 @@ void __weak hv_remove_vmbus_handler(void)
>  }
>  EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
> 
> +void __weak hv_setup_mshv_handler(void (*handler)(void))
> +{
> +}
> +EXPORT_SYMBOL_GPL(hv_setup_mshv_handler);
> +
>  void __weak hv_setup_kexec_handler(void (*handler)(void))
>  {
>  }
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 1f46d19a16aa..a05f12e63ccd 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -208,6 +208,7 @@ void hv_setup_kexec_handler(void (*handler)(void));
>  void hv_remove_kexec_handler(void);
>  void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
>  void hv_remove_crash_handler(void);
> +void hv_setup_mshv_handler(void (*handler)(void));
> 
>  extern int vmbus_interrupt;
>  extern int vmbus_irq;
> --
> 2.34.1

On 3/7/2025 9:44 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Wednesday, February 26, 2025 3:08 PM
> 
>>
>> This will handle SYNIC interrupts such as intercepts, doorbells, and
>> scheduling messages intended for the mshv driver.
> 
> Could you provide a bit more detailed commit message? How does
> the mshv_handler() relate to the vmbus_handler()? From the code
> mshv_handler() goes first, and I'm assuming it processes what it
> knows about (intercepts, doorbells, scheduling messages?) and
> then hands off control to the vmbus_handler() to handle the usual
> VMbus-related message and channel interrupts. But it would be
> nice to have the commit message or code comments describe the
> overall intent and any obscure aspects of the relationship.
> 
> And avoid references to "This" or "This patch". :-)
> 

You've described it pretty well, I don't know if there's too much
more to say given this patch doesn't introduce the handler code.

I can try to improve it, something like:
"
mshv_handler() will be used to process messages related to managing
guest partitions such as intercepts, doorbells, and scheduling messages.

In a (non-nested) root partition, the same interrupt vector is shared
between the vmbus and mshv_root drivers.

Introduce a stub for mshv_handler() and call it in
sysvec_hyperv_callback alongside vmbus_handler().

Even though both handlers will be called for every Hyper-V interrupt,
the messages for each driver are delivered to different offsets within
the SYNIC message page, so they won't step on each other.
"

Does that work?

Nuno
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Reviewed-by: Wei Liu <wei.liu@kernel.org>
>> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
>> ---
>>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
>>  drivers/hv/hv_common.c         | 5 +++++
>>  include/asm-generic/mshyperv.h | 1 +
>>  3 files changed, 15 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
>> index 0116d0e96ef9..616e9a5d77b4 100644
>> --- a/arch/x86/kernel/cpu/mshyperv.c
>> +++ b/arch/x86/kernel/cpu/mshyperv.c
>> @@ -107,6 +107,7 @@ 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);
>> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>>  	struct pt_regs *old_regs = set_irq_regs(regs);
>>
>>  	inc_irq_stat(irq_hv_callback_count);
>> +	if (mshv_handler)
>> +		mshv_handler();
>> +
>>  	if (vmbus_handler)
>>  		vmbus_handler();
>>
>> @@ -126,6 +130,11 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>>  	set_irq_regs(old_regs);
>>  }
>>
>> +void hv_setup_mshv_handler(void (*handler)(void))
>> +{
>> +	mshv_handler = handler;
>> +}
>> +
>>  void hv_setup_vmbus_handler(void (*handler)(void))
>>  {
>>  	vmbus_handler = handler;
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index 2763cb6d3678..f5a07fd9a03b 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -677,6 +677,11 @@ void __weak hv_remove_vmbus_handler(void)
>>  }
>>  EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
>>
>> +void __weak hv_setup_mshv_handler(void (*handler)(void))
>> +{
>> +}
>> +EXPORT_SYMBOL_GPL(hv_setup_mshv_handler);
>> +
>>  void __weak hv_setup_kexec_handler(void (*handler)(void))
>>  {
>>  }
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 1f46d19a16aa..a05f12e63ccd 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -208,6 +208,7 @@ void hv_setup_kexec_handler(void (*handler)(void));
>>  void hv_remove_kexec_handler(void);
>>  void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
>>  void hv_remove_crash_handler(void);
>> +void hv_setup_mshv_handler(void (*handler)(void));
>>
>>  extern int vmbus_interrupt;
>>  extern int vmbus_irq;
>> --
>> 2.34.1

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, March 7, 2025 3:30 PM
> 
> On 3/7/2025 9:44 AM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Wednesday, February 26, 2025 3:08 PM
> >
> >>
> >> This will handle SYNIC interrupts such as intercepts, doorbells, and
> >> scheduling messages intended for the mshv driver.
> >
> > Could you provide a bit more detailed commit message? How does
> > the mshv_handler() relate to the vmbus_handler()? From the code
> > mshv_handler() goes first, and I'm assuming it processes what it
> > knows about (intercepts, doorbells, scheduling messages?) and
> > then hands off control to the vmbus_handler() to handle the usual
> > VMbus-related message and channel interrupts. But it would be
> > nice to have the commit message or code comments describe the
> > overall intent and any obscure aspects of the relationship.
> >
> > And avoid references to "This" or "This patch". :-)
> >
> 
> You've described it pretty well, I don't know if there's too much
> more to say given this patch doesn't introduce the handler code.
> 
> I can try to improve it, something like:
> "
> mshv_handler() will be used to process messages related to managing
> guest partitions such as intercepts, doorbells, and scheduling messages.
> 
> In a (non-nested) root partition, the same interrupt vector is shared
> between the vmbus and mshv_root drivers.
> 
> Introduce a stub for mshv_handler() and call it in
> sysvec_hyperv_callback alongside vmbus_handler().
> 
> Even though both handlers will be called for every Hyper-V interrupt,
> the messages for each driver are delivered to different offsets within
> the SYNIC message page, so they won't step on each other.
> "
> 
> Does that work?

Yes, that works. I was going to ask how the two handlers could
Both be used on the same interrupt, and you've provided the
explanation, which is perfect. ;-)

Minor tweak: Start the first sentence as an imperative:

   Add mshv_handler() to process messages related .....

Michael

On 3/7/2025 9:38 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, February 28, 2025 4:38 PM
>>
>> On 2/26/2025 3:43 PM, Stanislav Kinsburskii wrote:
>>> On Wed, Feb 26, 2025 at 03:08:02PM -0800, Nuno Das Neves wrote:
>>>> This will handle SYNIC interrupts such as intercepts, doorbells, and
>>>> scheduling messages intended for the mshv driver.
>>>>
>>>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>>>> Reviewed-by: Wei Liu <wei.liu@kernel.org>
>>>> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
>>>> ---
>>>>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
>>>>  drivers/hv/hv_common.c         | 5 +++++
>>>>  include/asm-generic/mshyperv.h | 1 +
>>>>  3 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
>>>> index 0116d0e96ef9..616e9a5d77b4 100644
>>>> --- a/arch/x86/kernel/cpu/mshyperv.c
>>>> +++ b/arch/x86/kernel/cpu/mshyperv.c
>>>> @@ -107,6 +107,7 @@ 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);
>>>> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>>>>  	struct pt_regs *old_regs = set_irq_regs(regs);
>>>>
>>>>  	inc_irq_stat(irq_hv_callback_count);
>>>> +	if (mshv_handler)
>>>> +		mshv_handler();
>>>
>>> Can mshv_handler be defined as a weak symbol doing nothing instead
>>> of defining it a null pointer?
>>> This should allow to simplify this code and get rid of
>>> hv_setup_mshv_handler, which looks redundant.
>>>
>> Interesting, I tested this and it does seems to work! It seems like
>> a good change, thanks.
> 
> Just be a bit careful. When CONFIG_HYPERV=n, mshyperv.c still gets
> built even through none of the other Hyper-V related files do.  There
> are #ifdef CONFIG_HYPERV in mshyperv.c to eliminate references to
> Hyper-V files that wouldn't be built. I'd suggest doing a test build with
> that configuration to make sure it's all clean.
> 
Thanks Michael - I don't think it would be an issue since the __weak version
would be defined in mshyperv.c itself, replacing the function pointer.

However, I went and tested this __weak version again with CONFIG_MSHV_ROOT=m
and it does not actually work. Everything seems ok at first (it compiles,
can insert the module), but upon starting a guest, the interrupts don't get
delivered to the root (or rather, they don't get handled by mshv_hander()).

This seems to match with what the ld docs say - There's an option
LD_DYNAMIC_LINK to allow __weak symbols to be overridden by the dynamic
linker, but this is not enabled in the kernel.

So I will stick with the current implementation.

Nuno

> Michael
> 
>>
>>> Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>>>
>>>> +
>>>>  	if (vmbus_handler)
>>>>  		vmbus_handler();
>>>>
>>>> @@ -126,6 +130,11 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
>>>>  	set_irq_regs(old_regs);
>>>>  }
>>>>
>>>> +void hv_setup_mshv_handler(void (*handler)(void))
>>>> +{
>>>> +	mshv_handler = handler;
>>>> +}
>>>> +
>>>>  void hv_setup_vmbus_handler(void (*handler)(void))
>>>>  {
>>>>  	vmbus_handler = handler;
>>>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>>>> index 2763cb6d3678..f5a07fd9a03b 100644
>>>> --- a/drivers/hv/hv_common.c
>>>> +++ b/drivers/hv/hv_common.c
>>>> @@ -677,6 +677,11 @@ void __weak hv_remove_vmbus_handler(void)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(hv_remove_vmbus_handler);
>>>>
>>>> +void __weak hv_setup_mshv_handler(void (*handler)(void))
>>>> +{
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(hv_setup_mshv_handler);
>>>> +
>>>>  void __weak hv_setup_kexec_handler(void (*handler)(void))
>>>>  {
>>>>  }
>>>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>>>> index 1f46d19a16aa..a05f12e63ccd 100644
>>>> --- a/include/asm-generic/mshyperv.h
>>>> +++ b/include/asm-generic/mshyperv.h
>>>> @@ -208,6 +208,7 @@ void hv_setup_kexec_handler(void (*handler)(void));
>>>>  void hv_remove_kexec_handler(void);
>>>>  void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
>>>>  void hv_remove_crash_handler(void);
>>>> +void hv_setup_mshv_handler(void (*handler)(void));
>>>>
>>>>  extern int vmbus_interrupt;
>>>>  extern int vmbus_irq;
>>>> --
>>>> 2.34.1
>>>>
>

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Monday, March 10, 2025 2:47 PM
> 
> On 3/7/2025 9:38 AM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, February 28,
> 2025 4:38 PM
> >>
> >> On 2/26/2025 3:43 PM, Stanislav Kinsburskii wrote:
> >>> On Wed, Feb 26, 2025 at 03:08:02PM -0800, Nuno Das Neves wrote:
> >>>> This will handle SYNIC interrupts such as intercepts, doorbells, and
> >>>> scheduling messages intended for the mshv driver.
> >>>>
> >>>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> >>>> Reviewed-by: Wei Liu <wei.liu@kernel.org>
> >>>> Reviewed-by: Tianyu Lan <tiala@microsoft.com>
> >>>> ---
> >>>>  arch/x86/kernel/cpu/mshyperv.c | 9 +++++++++
> >>>>  drivers/hv/hv_common.c         | 5 +++++
> >>>>  include/asm-generic/mshyperv.h | 1 +
> >>>>  3 files changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> >>>> index 0116d0e96ef9..616e9a5d77b4 100644
> >>>> --- a/arch/x86/kernel/cpu/mshyperv.c
> >>>> +++ b/arch/x86/kernel/cpu/mshyperv.c
> >>>> @@ -107,6 +107,7 @@ 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);
> >>>> @@ -117,6 +118,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
> >>>>  	struct pt_regs *old_regs = set_irq_regs(regs);
> >>>>
> >>>>  	inc_irq_stat(irq_hv_callback_count);
> >>>> +	if (mshv_handler)
> >>>> +		mshv_handler();
> >>>
> >>> Can mshv_handler be defined as a weak symbol doing nothing instead
> >>> of defining it a null pointer?
> >>> This should allow to simplify this code and get rid of
> >>> hv_setup_mshv_handler, which looks redundant.
> >>>
> >> Interesting, I tested this and it does seems to work! It seems like
> >> a good change, thanks.
> >
> > Just be a bit careful. When CONFIG_HYPERV=n, mshyperv.c still gets
> > built even through none of the other Hyper-V related files do.  There
> > are #ifdef CONFIG_HYPERV in mshyperv.c to eliminate references to
> > Hyper-V files that wouldn't be built. I'd suggest doing a test build with
> > that configuration to make sure it's all clean.
> >
> Thanks Michael - I don't think it would be an issue since the __weak version
> would be defined in mshyperv.c itself, replacing the function pointer.

Yes, sounds right to me.

> 
> However, I went and tested this __weak version again with CONFIG_MSHV_ROOT=m
> and it does not actually work. Everything seems ok at first (it compiles,
> can insert the module), but upon starting a guest, the interrupts don't get
> delivered to the root (or rather, they don't get handled by mshv_hander()).
> 
> This seems to match with what the ld docs say - There's an option
> LD_DYNAMIC_LINK to allow __weak symbols to be overridden by the dynamic
> linker, but this is not enabled in the kernel.
> 

Yeah, I recall learning the hard way that a symbol defined in a module doesn't
override a __weak symbol in the kernel image. At the time, I gave up and took
a different path, and didn't get as far as looking at 'ld' options like you did. :-)

Michael