Re: [PATCH v4 2/2] mshv: add arm64 support for doorbell & intercept SINTs

[Anirudh Rayabharam]

On Wed, Feb 18, 2026 at 04:17:29AM +0000, Michael Kelley wrote:
> From: Anirudh Rayabharam <anirudh@xxxxxxxxxxxxx> Sent: Wednesday, February 11, 2026 9:07 AM
> > 
> > On x86, the HYPERVISOR_CALLBACK_VECTOR is used to receive synthetic
> > interrupts (SINTs) from the hypervisor for doorbells and intercepts.
> > There is no such vector reserved for arm64.
> > 
> > On arm64, the hypervisor exposes a synthetic register that can be read
> > to find the INTID that should be used for SINTs. This INTID is in the
> > PPI range.
> > 
> > To better unify the code paths, introduce mshv_sint_vector_init() that
> > either reads the synthetic register and obtains the INTID (arm64) or
> > just uses HYPERVISOR_CALLBACK_VECTOR as the interrupt vector (x86).
> > 
> > Signed-off-by: Anirudh Rayabharam (Microsoft) <anirudh@xxxxxxxxxxxxx>
> > ---
> >  drivers/hv/mshv_synic.c     | 112 +++++++++++++++++++++++++++++++++---
> >  include/hyperv/hvgdk_mini.h |   2 +
> >  2 files changed, 107 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> > index 074e37c48876..7957ad0328dd 100644
> > --- a/drivers/hv/mshv_synic.c
> > +++ b/drivers/hv/mshv_synic.c
> > @@ -10,17 +10,24 @@
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> >  #include <linux/mm.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/random.h>
> >  #include <linux/cpuhotplug.h>
> >  #include <linux/reboot.h>
> >  #include <asm/mshyperv.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/acpi.h>
> > 
> >  #include "mshv_eventfd.h"
> >  #include "mshv.h"
> > 
> >  static int synic_cpuhp_online;
> >  static struct hv_synic_pages __percpu *synic_pages;
> > +static int mshv_sint_vector = -1; /* hwirq for the SynIC SINTs */
> 
> With the introduction of this variable, the call to add_interrupt_randomness()
> in mshv_isr() should be updated to pass mshv_sint_vector as the argument,
> and the #ifdef HYPERVISOR_CALLBACK_VECTOR can be dropped (yea!).  My
> previous comment about the generic Linux IRQ handling doing the call
> to add_interrupt_randomness() is true for "normal" IRQs but not for per-CPU
> IRQs like these. So the call to add_interrupt_randomness() in mshv_isr() is
> needed on both x86 and ARM64.
> 
> > +#ifndef HYPERVISOR_CALLBACK_VECTOR
> > +static int mshv_sint_irq = -1; /* Linux IRQ for mshv_sint_vector */
> > +#endif
> 
> Documentation/process/coding-style.rst says the following in Section 21:
> 
> If you have a function or variable which may potentially go unused in a
> particular configuration, and the compiler would warn about its definition
> going unused, mark the definition as __maybe_unused rather than wrapping it in
> a preprocessor conditional.
> 
> You could tag mshv_sint_irq with "__maybe_unused" and avoid the #ifndef. But
> see further comments below.
> 
> > 
> >  static u32 synic_event_ring_get_queued_port(u32 sint_index)
> >  {
> > @@ -456,9 +463,7 @@ static int mshv_synic_cpu_init(unsigned int cpu)
> >  	union hv_synic_simp simp;
> >  	union hv_synic_siefp siefp;
> >  	union hv_synic_sirbp sirbp;
> > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> >  	union hv_synic_sint sint;
> > -#endif
> >  	union hv_synic_scontrol sctrl;
> >  	struct hv_synic_pages *spages = this_cpu_ptr(synic_pages);
> >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > @@ -501,10 +506,13 @@ static int mshv_synic_cpu_init(unsigned int cpu)
> > 
> >  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
> > 
> > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> > +#ifndef HYPERVISOR_CALLBACK_VECTOR
> > +	enable_percpu_irq(mshv_sint_irq, 0);
> > +#endif
> > +
> 
> Using IS_ENABLED() would be better than the #ifndef. (See Section 21
> of coding-style.rst about this as well.) You would need to drop the #ifndef
> around mshv_sint_irq, which is fine.
> 
> 	if (!IS_ENABLED(HYPERVISOR_CALLBACK_VECTOR))
> 		enable_percpu_irq(mshv_sint_irq, 0);
> 
> That said, I prefer the approach in v1 of your series where basically
> the code says "if we have a sint irq, enable it". This links the enablement
> most closely to what it directly depends on.
> 
> 	if (mshv_sint_irq != -1)
> 		enable_percpu_irq(mshv_sint_irq, 0);
> 
> But I realize the approach is somewhat a matter of personal preference so either
> way is acceptable.
> 
> >  	/* Enable intercepts */
> >  	sint.as_uint64 = 0;
> > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > +	sint.vector = mshv_sint_vector;
> >  	sint.masked = false;
> >  	sint.auto_eoi = hv_recommend_using_aeoi();
> >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> > @@ -512,13 +520,12 @@ static int mshv_synic_cpu_init(unsigned int cpu)
> > 
> >  	/* Doorbell SINT */
> >  	sint.as_uint64 = 0;
> > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > +	sint.vector = mshv_sint_vector;
> >  	sint.masked = false;
> >  	sint.as_intercept = 1;
> >  	sint.auto_eoi = hv_recommend_using_aeoi();
> >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> >  			      sint.as_uint64);
> > -#endif
> > 
> >  	/* Enable global synic bit */
> >  	sctrl.as_uint64 = hv_get_non_nested_msr(HV_MSR_SCONTROL);
> > @@ -573,6 +580,10 @@ static int mshv_synic_cpu_exit(unsigned int cpu)
> >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> >  			      sint.as_uint64);
> > 
> > +#ifndef HYPERVISOR_CALLBACK_VECTOR
> > +	disable_percpu_irq(mshv_sint_irq);
> > +#endif
> > +
> 
> Same here.
> 
> >  	/* Disable Synic's event ring page */
> >  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
> >  	sirbp.sirbp_enabled = false;
> > @@ -683,14 +694,98 @@ static struct notifier_block mshv_synic_reboot_nb = {
> >  	.notifier_call = mshv_synic_reboot_notify,
> >  };
> > 
> > +#ifndef HYPERVISOR_CALLBACK_VECTOR
> > +#ifdef CONFIG_ACPI
> > +static long __percpu *mshv_evt;
> > +#endif
> 
> Same comment here about the coding-style.rst guidelines.
> 
> Furthermore, mshv_evt could be directly defined here as a per-cpu "long",
> rather than a pointer to a long. Then you don't need to do a runtime
> per-cpu allocation with all the attendant error checking and cleanup, which
> saves about 10 lines of code. So
> 
> static DEFINE_PER_CPU(long, mshv_evt);
> 
> drivers/clocksource/hyperv_timer.c does the definition for stimer0_evt this
> way. I looked through all kernel code and found several other places doing
> the direct definition. I don't remember why I didn't do the direct method for
> vmbus_evt, but I'm planning to submit a patch to change it, which will drop
> a few lines of code.
> 
> > +
> > +static irqreturn_t mshv_percpu_isr(int irq, void *dev_id)
> > +{
> > +	mshv_isr();
> > +	return IRQ_HANDLED;
> > +}
> 
> This function generates a warning about being unused when !CONFIG_ACPI.
> But see further comments below.
> 
> > +
> > +static int __init mshv_sint_vector_init(void)
> > +{
> > +#ifdef CONFIG_ACPI
> > +	int ret;
> > +	struct hv_register_assoc reg = {
> > +		.name = HV_ARM64_REGISTER_SINT_RESERVED_INTERRUPT_ID,
> > +	};
> > +	union hv_input_vtl input_vtl = { 0 };
> > +
> > +	ret = hv_call_get_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
> > +				1, input_vtl, &reg);
> > +	if (ret || !reg.value.reg64)
> > +		return -ENODEV;
> > +
> > +	mshv_sint_vector = reg.value.reg64;
> > +	ret  = acpi_register_gsi(NULL, mshv_sint_vector, ACPI_EDGE_SENSITIVE,
> > +					ACPI_ACTIVE_HIGH);
> > +	if (ret < 0)
> > +		goto out_fail;
> > +
> > +	mshv_sint_irq = ret;
> > +
> > +	mshv_evt = alloc_percpu(long);
> > +	if (!mshv_evt) {
> > +		ret = -ENOMEM;
> > +		goto out_unregister;
> > +	}
> > +
> > +	ret = request_percpu_irq(mshv_sint_irq, mshv_percpu_isr, "MSHV",
> > +		mshv_evt);
> > +	if (ret)
> > +		goto free_evt;
> > +
> > +	return 0;
> > +
> > +free_evt:
> > +	free_percpu(mshv_evt);
> > +out_unregister:
> > +	acpi_unregister_gsi(mshv_sint_vector);
> > +out_fail:
> > +	return ret;
> > +#else
> > +	return -ENODEV;
> > +#endif
> > +}
> 
> I have several thoughts about the #ifdef CONFIG_ACPI.
> 
> The coding-style.rst guidelines in Section 21 also say:
> 
> Prefer to compile out entire functions, rather than portions of functions or
> portions of expressions.  Rather than putting an ifdef in an expression, factor
> out part or all of the expression into a separate helper function and apply the
> conditional to that function.
> 
> But more fundamentally, it looks like the #ifdef CONFIG_ACPI is there
> solely because acpi_register_gsi() exists only when CONFIG_ACPI is set.
> The rest of the code doesn't depend on ACPI. In the !CONFIG_ACPI case,
> your stub code returns -ENODEV, so doorbell & intercept SINTs just don't
> work, and pretty much everything is non-functional.
> 
> This patch doesn't allude to any future DeviceTree case that parallels ACPI,
> so I'm unsure what's expected in the future.  If such a future DT case is
> murky, perhaps drivers/hv/Kconfig should give MSHV_ROOT a dependency
> on ACPI. Then the #ifdef CONFIG_ACPI could be dropped, along with the
> #else stub code. When/if the DT use case comes along, the dependency
> can be removed and the code structured to handle both ACPI and DT.
> The code to fetch the INTID via the hypervisor synthetic register, and the
> request_percpu_irq() would be applicable to both. It's only the GSI
> registration that would be different, and that could be pulled out into a
> helper function that handles the difference in ACPI and DT. I haven't looked
> to see how DT does the equivalent of GSI registration.

The DT case will materialize in the future. Making MSHV_ROOT depend on
ACPI seems a bit drastic to me when all we want to do is follow the
coding style guideline that says "prefer to compile out entire
functions...".

> 
> Another approach would be to add stubs for acpi_register_gsi() and
> acpi_unregister_gsi() in include/linux/acpi.h.  A number of such stubs
> have been added over the years. Saurabh got one added in 2023
> (commit 1f6277bf716cc). Then the above code would compile even
> with !CONFIG_ACPI.  acpi_register_gsi() would fail, and you would get
> an error return. This approach produces cleaner code and is consistent
> with similar use cases that depend on stubs provided by include/linux/acpi.h
> rather than #ifdefs.

I'll send out a v5 which takes a simpler approach to conform to the
coding guidelines. I'll also address all the other comments from above.

Thanks,
Anirudh.