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

[Michael Kelley]

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.

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.

And either of these approaches avoids the unused mshv_percpu_isr()
function and mshv_evt variable.

> +
> +static void mshv_sint_vector_cleanup(void)
> +{
> +#ifdef CONFIG_ACPI
> +	free_percpu_irq(mshv_sint_irq, mshv_evt);
> +	free_percpu(mshv_evt);
> +	acpi_unregister_gsi(mshv_sint_vector);
> +#endif
> +}
> +#else /* !HYPERVISOR_CALLBACK_VECTOR */
> +static int __init mshv_sint_vector_init(void)
> +{
> +	mshv_sint_vector = HYPERVISOR_CALLBACK_VECTOR;
> +	return 0;
> +}
> +
> +static void mshv_sint_vector_cleanup(void)
> +{
> +}
> +#endif /* HYPERVISOR_CALLBACK_VECTOR */
> +
>  int __init mshv_synic_init(struct device *dev)
>  {
>  	int ret = 0;
> 
> +	ret = mshv_sint_vector_init();
> +	if (ret) {
> +		dev_err(dev, "Failed to get MSHV SINT vector: %i\n", ret);
> +		return ret;
> +	}
> +
>  	synic_pages = alloc_percpu(struct hv_synic_pages);
>  	if (!synic_pages) {
>  		dev_err(dev, "Failed to allocate percpu synic page\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto sint_vector_cleanup;
>  	}
> 
>  	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mshv_synic",
> @@ -713,6 +808,8 @@ int __init mshv_synic_init(struct device *dev)
>  	cpuhp_remove_state(synic_cpuhp_online);
>  free_synic_pages:
>  	free_percpu(synic_pages);
> +sint_vector_cleanup:
> +	mshv_sint_vector_cleanup();
>  	return ret;
>  }
> 
> @@ -721,4 +818,5 @@ void mshv_synic_cleanup(void)
>  	unregister_reboot_notifier(&mshv_synic_reboot_nb);
>  	cpuhp_remove_state(synic_cpuhp_online);
>  	free_percpu(synic_pages);
> +	mshv_sint_vector_cleanup();
>  }
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index 30fbbde81c5c..7676f78e0766 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -1117,6 +1117,8 @@ enum hv_register_name {
>  	HV_X64_REGISTER_MSR_MTRR_FIX4KF8000	= 0x0008007A,
> 
>  	HV_X64_REGISTER_REG_PAGE	= 0x0009001C,
> +#elif defined(CONFIG_ARM64)
> +	HV_ARM64_REGISTER_SINT_RESERVED_INTERRUPT_ID	= 0x00070001,
>  #endif
>  };
> 
> --
> 2.34.1
>