From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>

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 INTID for SINTs should be in the SGI or PPI range. The
hypervisor exposes a virtual device in the ACPI that reserves a
PPI for this use. Introduce a platform_driver that binds to this ACPI
device and obtains the interrupt vector that can be used for SINTs.

To better unify x86 and arm64 paths, introduce mshv_sint_irq_init() that
either registers the platform_driver and obtains the INTID (arm64) or
just uses HYPERVISOR_CALLBACK_VECTOR as the interrupt vector (x86).

Signed-off-by: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
---
 drivers/hv/mshv_root.h      |   2 +
 drivers/hv/mshv_root_main.c |  11 ++-
 drivers/hv/mshv_synic.c     | 152 ++++++++++++++++++++++++++++++++++--
 3 files changed, 158 insertions(+), 7 deletions(-)

diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
index c02513f75429..c2d1e8d7452c 100644
--- a/drivers/hv/mshv_root.h
+++ b/drivers/hv/mshv_root.h
@@ -332,5 +332,7 @@ int mshv_region_get(struct mshv_mem_region *region);
 bool mshv_region_handle_gfn_fault(struct mshv_mem_region *region, u64 gfn);
 void mshv_region_movable_fini(struct mshv_mem_region *region);
 bool mshv_region_movable_init(struct mshv_mem_region *region);
+int mshv_synic_init(void);
+void mshv_synic_cleanup(void);
 
 #endif /* _MSHV_ROOT_H_ */
diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
index abb34b37d552..6c2d4a80dbe3 100644
--- a/drivers/hv/mshv_root_main.c
+++ b/drivers/hv/mshv_root_main.c
@@ -2276,11 +2276,17 @@ static int __init mshv_parent_partition_init(void)
 			MSHV_HV_MAX_VERSION);
 	}
 
+	ret = mshv_synic_init();
+	if (ret) {
+		dev_err(dev, "Failed to initialize synic: %i\n", ret);
+		goto device_deregister;
+	}
+
 	mshv_root.synic_pages = alloc_percpu(struct hv_synic_pages);
 	if (!mshv_root.synic_pages) {
 		dev_err(dev, "Failed to allocate percpu synic page\n");
 		ret = -ENOMEM;
-		goto device_deregister;
+		goto synic_cleanup;
 	}
 
 	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mshv_synic",
@@ -2322,6 +2328,8 @@ static int __init mshv_parent_partition_init(void)
 	cpuhp_remove_state(mshv_cpuhp_online);
 free_synic_pages:
 	free_percpu(mshv_root.synic_pages);
+synic_cleanup:
+	mshv_synic_cleanup();
 device_deregister:
 	misc_deregister(&mshv_dev);
 	return ret;
@@ -2337,6 +2345,7 @@ static void __exit mshv_parent_partition_exit(void)
 		mshv_root_partition_exit();
 	cpuhp_remove_state(mshv_cpuhp_online);
 	free_percpu(mshv_root.synic_pages);
+	mshv_synic_cleanup();
 }
 
 module_init(mshv_parent_partition_init);
diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
index ba89655b0910..b7860a75b97e 100644
--- a/drivers/hv/mshv_synic.c
+++ b/drivers/hv/mshv_synic.c
@@ -10,13 +10,19 @@
 #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 <asm/mshyperv.h>
+#include <linux/platform_device.h>
+#include <linux/acpi.h>
 
 #include "mshv_eventfd.h"
 #include "mshv.h"
 
+static int mshv_interrupt = -1;
+static int mshv_irq = -1;
+
 static u32 synic_event_ring_get_queued_port(u32 sint_index)
 {
 	struct hv_synic_event_ring_page **event_ring_page;
@@ -446,14 +452,144 @@ void mshv_isr(void)
 	}
 }
 
+#ifndef HYPERVISOR_CALLBACK_VECTOR
+#ifdef CONFIG_ACPI
+static long __percpu *mshv_evt;
+
+static acpi_status mshv_walk_resources(struct acpi_resource *res, void *ctx)
+{
+	struct resource r;
+
+	switch (res->type) {
+	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+		if (!acpi_dev_resource_interrupt(res, 0, &r)) {
+			pr_err("Unable to parse MSHV ACPI interrupt\n");
+			return AE_ERROR;
+		}
+		/* ARM64 INTID */
+		mshv_interrupt = res->data.extended_irq.interrupts[0];
+		/* Linux IRQ number */
+		mshv_irq = r.start;
+		pr_info("MSHV SINT INTID %d, IRQ %d\n",
+			mshv_interrupt, mshv_irq);
+		return AE_OK;
+	default:
+		/* Unused resource type */
+		return AE_OK;
+	}
+
+	return AE_OK;
+}
+
+static irqreturn_t mshv_percpu_isr(int irq, void *dev_id)
+{
+	mshv_isr();
+	add_interrupt_randomness(irq);
+	return IRQ_HANDLED;
+}
+
+static int mshv_sint_probe(struct platform_device *pdev)
+{
+	acpi_status result;
+	int ret = 0;
+	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
+
+	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+					mshv_walk_resources, NULL);
+
+	if (ACPI_FAILURE(result)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	mshv_evt = alloc_percpu(long);
+	if (!mshv_evt) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = request_percpu_irq(mshv_irq, mshv_percpu_isr, "MSHV", mshv_evt);
+out:
+	return ret;
+}
+
+static void mshv_sint_remove(struct platform_device *pdev)
+{
+	free_percpu_irq(mshv_irq, mshv_evt);
+	free_percpu(mshv_evt);
+}
+#else
+static int mshv_sint_probe(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+
+static void mshv_sint_remove(struct platform_device *pdev)
+{
+	return;
+}
+#endif
+
+
+static const __maybe_unused struct acpi_device_id mshv_sint_device_ids[] = {
+	{"MSFT1003", 0},
+	{"", 0},
+};
+
+static struct platform_driver mshv_sint_drv = {
+	.probe = mshv_sint_probe,
+	.remove = mshv_sint_remove,
+	.driver = {
+		.name = "mshv_sint",
+		.acpi_match_table = ACPI_PTR(mshv_sint_device_ids),
+		.probe_type = PROBE_FORCE_SYNCHRONOUS,
+	},
+};
+#endif /* HYPERVISOR_CALLBACK_VECTOR */
+
+int mshv_synic_init(void)
+{
+#ifdef HYPERVISOR_CALLBACK_VECTOR
+	mshv_interrupt = HYPERVISOR_CALLBACK_VECTOR;
+	mshv_irq = -1;
+	return 0;
+#else
+	int ret;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	ret = platform_driver_register(&mshv_sint_drv);
+	if (ret)
+		return ret;
+
+	if (mshv_interrupt == -1 || mshv_irq == -1) {
+		ret = -ENODEV;
+		goto out_unregister;
+	}
+
+	return 0;
+
+out_unregister:
+	platform_driver_unregister(&mshv_sint_drv);
+	return ret;
+#endif
+}
+
+void mshv_synic_cleanup(void)
+{
+#ifndef HYPERVISOR_CALLBACK_VECTOR
+	if (!acpi_disabled)
+		platform_driver_unregister(&mshv_sint_drv);
+#endif
+}
+
 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(mshv_root.synic_pages);
 	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
@@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
 
 	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
 
-#ifdef HYPERVISOR_CALLBACK_VECTOR
+	if (mshv_irq != -1)
+		enable_percpu_irq(mshv_irq, 0);
+
 	/* Enable intercepts */
 	sint.as_uint64 = 0;
-	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
+	sint.vector = mshv_interrupt;
 	sint.masked = false;
 	sint.auto_eoi = hv_recommend_using_aeoi();
 	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
@@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
 
 	/* Doorbell SINT */
 	sint.as_uint64 = 0;
-	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
+	sint.vector = mshv_interrupt;
 	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);
@@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
 	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
 			      sint.as_uint64);
 
+	if (mshv_irq != -1)
+		disable_percpu_irq(mshv_irq);
+
 	/* Disable Synic's event ring page */
 	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
 	sirbp.sirbp_enabled = false;
-- 
2.34.1

On Wed, Jan 28, 2026 at 04:04:37PM +0000, Anirudh Rayabharam wrote:
> From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> 
> 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 INTID for SINTs should be in the SGI or PPI range. The
> hypervisor exposes a virtual device in the ACPI that reserves a
> PPI for this use. Introduce a platform_driver that binds to this ACPI
> device and obtains the interrupt vector that can be used for SINTs.
> 
> To better unify x86 and arm64 paths, introduce mshv_sint_irq_init() that

Where is mshv_sint_irq_init?

> either registers the platform_driver and obtains the INTID (arm64) or
> just uses HYPERVISOR_CALLBACK_VECTOR as the interrupt vector (x86).
> 
> Signed-off-by: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> ---
>  drivers/hv/mshv_root.h      |   2 +
>  drivers/hv/mshv_root_main.c |  11 ++-
>  drivers/hv/mshv_synic.c     | 152 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 158 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
> index c02513f75429..c2d1e8d7452c 100644
> --- a/drivers/hv/mshv_root.h
> +++ b/drivers/hv/mshv_root.h
> @@ -332,5 +332,7 @@ int mshv_region_get(struct mshv_mem_region *region);
>  bool mshv_region_handle_gfn_fault(struct mshv_mem_region *region, u64 gfn);
>  void mshv_region_movable_fini(struct mshv_mem_region *region);
>  bool mshv_region_movable_init(struct mshv_mem_region *region);
> +int mshv_synic_init(void);
> +void mshv_synic_cleanup(void);
>  
>  #endif /* _MSHV_ROOT_H_ */
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index abb34b37d552..6c2d4a80dbe3 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -2276,11 +2276,17 @@ static int __init mshv_parent_partition_init(void)
>  			MSHV_HV_MAX_VERSION);
>  	}
>  
> +	ret = mshv_synic_init();
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize synic: %i\n", ret);
> +		goto device_deregister;
> +	}
> +
>  	mshv_root.synic_pages = alloc_percpu(struct hv_synic_pages);
>  	if (!mshv_root.synic_pages) {
>  		dev_err(dev, "Failed to allocate percpu synic page\n");
>  		ret = -ENOMEM;
> -		goto device_deregister;
> +		goto synic_cleanup;
>  	}

Should this become a part of mshv_synic_init()?

>  
>  	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mshv_synic",
> @@ -2322,6 +2328,8 @@ static int __init mshv_parent_partition_init(void)
>  	cpuhp_remove_state(mshv_cpuhp_online);
>  free_synic_pages:
>  	free_percpu(mshv_root.synic_pages);
> +synic_cleanup:
> +	mshv_synic_cleanup();
>  device_deregister:
>  	misc_deregister(&mshv_dev);
>  	return ret;
> @@ -2337,6 +2345,7 @@ static void __exit mshv_parent_partition_exit(void)
>  		mshv_root_partition_exit();
>  	cpuhp_remove_state(mshv_cpuhp_online);
>  	free_percpu(mshv_root.synic_pages);
> +	mshv_synic_cleanup();

Please, follow the common convention where cleaup path is the reverse of
init path.

>  }
>  
>  module_init(mshv_parent_partition_init);
> diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> index ba89655b0910..b7860a75b97e 100644
> --- a/drivers/hv/mshv_synic.c
> +++ b/drivers/hv/mshv_synic.c
> @@ -10,13 +10,19 @@
>  #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 <asm/mshyperv.h>
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
>  
>  #include "mshv_eventfd.h"
>  #include "mshv.h"
>  
> +static int mshv_interrupt = -1;

The name is a bit too short. What about mshv_callback_vector or
mshv_irq_vector?

> +static int mshv_irq = -1;
> +

Should this be a path of mshv_root structure?

>  static u32 synic_event_ring_get_queued_port(u32 sint_index)
>  {
>  	struct hv_synic_event_ring_page **event_ring_page;
> @@ -446,14 +452,144 @@ void mshv_isr(void)
>  	}
>  }
>  
> +#ifndef HYPERVISOR_CALLBACK_VECTOR
> +#ifdef CONFIG_ACPI
> +static long __percpu *mshv_evt;
> +
> +static acpi_status mshv_walk_resources(struct acpi_resource *res, void *ctx)
> +{
> +	struct resource r;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> +		if (!acpi_dev_resource_interrupt(res, 0, &r)) {
> +			pr_err("Unable to parse MSHV ACPI interrupt\n");
> +			return AE_ERROR;
> +		}
> +		/* ARM64 INTID */
> +		mshv_interrupt = res->data.extended_irq.interrupts[0];
> +		/* Linux IRQ number */
> +		mshv_irq = r.start;
> +		pr_info("MSHV SINT INTID %d, IRQ %d\n",
> +			mshv_interrupt, mshv_irq);
> +		return AE_OK;
> +	default:
> +		/* Unused resource type */
> +		return AE_OK;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static irqreturn_t mshv_percpu_isr(int irq, void *dev_id)
> +{
> +	mshv_isr();
> +	add_interrupt_randomness(irq);
> +	return IRQ_HANDLED;
> +}
> +
> +static int mshv_sint_probe(struct platform_device *pdev)
> +{
> +	acpi_status result;
> +	int ret = 0;
> +	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
> +
> +	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +					mshv_walk_resources, NULL);
> +
> +	if (ACPI_FAILURE(result)) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	mshv_evt = alloc_percpu(long);
> +	if (!mshv_evt) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = request_percpu_irq(mshv_irq, mshv_percpu_isr, "MSHV", mshv_evt);
> +out:
> +	return ret;
> +}
> +
> +static void mshv_sint_remove(struct platform_device *pdev)
> +{
> +	free_percpu_irq(mshv_irq, mshv_evt);
> +	free_percpu(mshv_evt);
> +}
> +#else
> +static int mshv_sint_probe(struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +
> +static void mshv_sint_remove(struct platform_device *pdev)
> +{
> +	return;
> +}
> +#endif
> +

Is this all x86-compatible?
The commit message says it's introduced for arm64.
If it's incompatible, please, wrap it into #ifdefs and compile out for
x86_64.

> +
> +static const __maybe_unused struct acpi_device_id mshv_sint_device_ids[] = {
> +	{"MSFT1003", 0},
> +	{"", 0},
> +};
> +
> +static struct platform_driver mshv_sint_drv = {
> +	.probe = mshv_sint_probe,
> +	.remove = mshv_sint_remove,
> +	.driver = {
> +		.name = "mshv_sint",
> +		.acpi_match_table = ACPI_PTR(mshv_sint_device_ids),
> +		.probe_type = PROBE_FORCE_SYNCHRONOUS,
> +	},
> +};
> +#endif /* HYPERVISOR_CALLBACK_VECTOR */
> +
> +int mshv_synic_init(void)
> +{
> +#ifdef HYPERVISOR_CALLBACK_VECTOR
> +	mshv_interrupt = HYPERVISOR_CALLBACK_VECTOR;
> +	mshv_irq = -1;
> +	return 0;
> +#else
> +	int ret;
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	ret = platform_driver_register(&mshv_sint_drv);
> +	if (ret)
> +		return ret;
> +
> +	if (mshv_interrupt == -1 || mshv_irq == -1) {
> +		ret = -ENODEV;
> +		goto out_unregister;
> +	}
> +
> +	return 0;
> +
> +out_unregister:
> +	platform_driver_unregister(&mshv_sint_drv);
> +	return ret;
> +#endif
> +}
> +
> +void mshv_synic_cleanup(void)
> +{
> +#ifndef HYPERVISOR_CALLBACK_VECTOR
> +	if (!acpi_disabled)
> +		platform_driver_unregister(&mshv_sint_drv);
> +#endif
> +}
> +
>  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(mshv_root.synic_pages);
>  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> @@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
>  
>  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
>  
> -#ifdef HYPERVISOR_CALLBACK_VECTOR
> +	if (mshv_irq != -1)
> +		enable_percpu_irq(mshv_irq, 0);
> +

It's better to explicitly separate x86 and arm64 paths with #ifdefs.
For example:

#ifdef CONFIG_X86_64
int setup_cpu_sint() {
  	/* Enable intercepts */
  	sint.as_uint64 = 0;
	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
	....
}
#endif
#ifdef CONFIG_ARM64
int setup_cpu_sint() {
	enable_percpu_irq(mshv_irq, 0);

  	/* Enable intercepts */
  	sint.as_uint64 = 0;
	sint.vector = mshv_interrupt;
	....
}
#endif

Thanks,
Stanislav

>  	/* Enable intercepts */
>  	sint.as_uint64 = 0;
> -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> +	sint.vector = mshv_interrupt;
>  	sint.masked = false;
>  	sint.auto_eoi = hv_recommend_using_aeoi();
>  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> @@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
>  
>  	/* Doorbell SINT */
>  	sint.as_uint64 = 0;
> -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> +	sint.vector = mshv_interrupt;
>  	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);
> @@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
>  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
>  			      sint.as_uint64);
>  
> +	if (mshv_irq != -1)
> +		disable_percpu_irq(mshv_irq);
> +
>  	/* Disable Synic's event ring page */
>  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
>  	sirbp.sirbp_enabled = false;
> -- 
> 2.34.1
>

On Wed, Jan 28, 2026 at 03:03:51PM -0800, Stanislav Kinsburskii wrote:
> On Wed, Jan 28, 2026 at 04:04:37PM +0000, Anirudh Rayabharam wrote:
> > From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> > 
> > 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 INTID for SINTs should be in the SGI or PPI range. The
> > hypervisor exposes a virtual device in the ACPI that reserves a
> > PPI for this use. Introduce a platform_driver that binds to this ACPI
> > device and obtains the interrupt vector that can be used for SINTs.
> > 
> > To better unify x86 and arm64 paths, introduce mshv_sint_irq_init() that
> 
> Where is mshv_sint_irq_init?

Oops, this should be mshv_synic_init(). Leftover from previous
development version of this patch :)

Will fix in the next version.

> 
> > either registers the platform_driver and obtains the INTID (arm64) or
> > just uses HYPERVISOR_CALLBACK_VECTOR as the interrupt vector (x86).
> > 
> > Signed-off-by: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> > ---
> >  drivers/hv/mshv_root.h      |   2 +
> >  drivers/hv/mshv_root_main.c |  11 ++-
> >  drivers/hv/mshv_synic.c     | 152 ++++++++++++++++++++++++++++++++++--
> >  3 files changed, 158 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
> > index c02513f75429..c2d1e8d7452c 100644
> > --- a/drivers/hv/mshv_root.h
> > +++ b/drivers/hv/mshv_root.h
> > @@ -332,5 +332,7 @@ int mshv_region_get(struct mshv_mem_region *region);
> >  bool mshv_region_handle_gfn_fault(struct mshv_mem_region *region, u64 gfn);
> >  void mshv_region_movable_fini(struct mshv_mem_region *region);
> >  bool mshv_region_movable_init(struct mshv_mem_region *region);
> > +int mshv_synic_init(void);
> > +void mshv_synic_cleanup(void);
> >  
> >  #endif /* _MSHV_ROOT_H_ */
> > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> > index abb34b37d552..6c2d4a80dbe3 100644
> > --- a/drivers/hv/mshv_root_main.c
> > +++ b/drivers/hv/mshv_root_main.c
> > @@ -2276,11 +2276,17 @@ static int __init mshv_parent_partition_init(void)
> >  			MSHV_HV_MAX_VERSION);
> >  	}
> >  
> > +	ret = mshv_synic_init();
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize synic: %i\n", ret);
> > +		goto device_deregister;
> > +	}
> > +
> >  	mshv_root.synic_pages = alloc_percpu(struct hv_synic_pages);
> >  	if (!mshv_root.synic_pages) {
> >  		dev_err(dev, "Failed to allocate percpu synic page\n");
> >  		ret = -ENOMEM;
> > -		goto device_deregister;
> > +		goto synic_cleanup;
> >  	}
> 
> Should this become a part of mshv_synic_init()?

Yeah, good idea. Maybe even the below cpuhp_setup_state can be moved.

> 
> >  
> >  	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mshv_synic",
> > @@ -2322,6 +2328,8 @@ static int __init mshv_parent_partition_init(void)
> >  	cpuhp_remove_state(mshv_cpuhp_online);
> >  free_synic_pages:
> >  	free_percpu(mshv_root.synic_pages);
> > +synic_cleanup:
> > +	mshv_synic_cleanup();
> >  device_deregister:
> >  	misc_deregister(&mshv_dev);
> >  	return ret;
> > @@ -2337,6 +2345,7 @@ static void __exit mshv_parent_partition_exit(void)
> >  		mshv_root_partition_exit();
> >  	cpuhp_remove_state(mshv_cpuhp_online);
> >  	free_percpu(mshv_root.synic_pages);
> > +	mshv_synic_cleanup();
> 
> Please, follow the common convention where cleaup path is the reverse of
> init path.

Right, will fix this.

> 
> >  }
> >  
> >  module_init(mshv_parent_partition_init);
> > diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> > index ba89655b0910..b7860a75b97e 100644
> > --- a/drivers/hv/mshv_synic.c
> > +++ b/drivers/hv/mshv_synic.c
> > @@ -10,13 +10,19 @@
> >  #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 <asm/mshyperv.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/acpi.h>
> >  
> >  #include "mshv_eventfd.h"
> >  #include "mshv.h"
> >  
> > +static int mshv_interrupt = -1;
> 
> The name is a bit too short. What about mshv_callback_vector or
> mshv_irq_vector?

I like mshv_callback_vector. I'll change to that in the next version
unless someone else comes up with a better suggestion.

> 
> > +static int mshv_irq = -1;
> > +
> 
> Should this be a path of mshv_root structure?

This doesn't need to be globally accessible. It is only used in this file.
So I guess it doesn't need to be in mshv_root. What do you think?

> 
> >  static u32 synic_event_ring_get_queued_port(u32 sint_index)
> >  {
> >  	struct hv_synic_event_ring_page **event_ring_page;
> > @@ -446,14 +452,144 @@ void mshv_isr(void)
> >  	}
> >  }
> >  
> > +#ifndef HYPERVISOR_CALLBACK_VECTOR
> > +#ifdef CONFIG_ACPI
> > +static long __percpu *mshv_evt;
> > +
> > +static acpi_status mshv_walk_resources(struct acpi_resource *res, void *ctx)
> > +{
> > +	struct resource r;
> > +
> > +	switch (res->type) {
> > +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> > +		if (!acpi_dev_resource_interrupt(res, 0, &r)) {
> > +			pr_err("Unable to parse MSHV ACPI interrupt\n");
> > +			return AE_ERROR;
> > +		}
> > +		/* ARM64 INTID */
> > +		mshv_interrupt = res->data.extended_irq.interrupts[0];
> > +		/* Linux IRQ number */
> > +		mshv_irq = r.start;
> > +		pr_info("MSHV SINT INTID %d, IRQ %d\n",
> > +			mshv_interrupt, mshv_irq);
> > +		return AE_OK;
> > +	default:
> > +		/* Unused resource type */
> > +		return AE_OK;
> > +	}
> > +
> > +	return AE_OK;
> > +}
> > +
> > +static irqreturn_t mshv_percpu_isr(int irq, void *dev_id)
> > +{
> > +	mshv_isr();
> > +	add_interrupt_randomness(irq);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mshv_sint_probe(struct platform_device *pdev)
> > +{
> > +	acpi_status result;
> > +	int ret = 0;
> > +	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
> > +
> > +	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> > +					mshv_walk_resources, NULL);
> > +
> > +	if (ACPI_FAILURE(result)) {
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
> > +
> > +	mshv_evt = alloc_percpu(long);
> > +	if (!mshv_evt) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	ret = request_percpu_irq(mshv_irq, mshv_percpu_isr, "MSHV", mshv_evt);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static void mshv_sint_remove(struct platform_device *pdev)
> > +{
> > +	free_percpu_irq(mshv_irq, mshv_evt);
> > +	free_percpu(mshv_evt);
> > +}
> > +#else
> > +static int mshv_sint_probe(struct platform_device *pdev)
> > +{
> > +	return -ENODEV;
> > +}
> > +
> > +static void mshv_sint_remove(struct platform_device *pdev)
> > +{
> > +	return;
> > +}
> > +#endif
> > +
> 
> Is this all x86-compatible?
> The commit message says it's introduced for arm64.
> If it's incompatible, please, wrap it into #ifdefs and compile out for
> x86_64.

They are wrapped in #ifndef HYPERVISOR_CALLBACK_VECTOR.

If that is defined we use the hardcoded vector. It is currently
only defined for x86 so HYPERVISOR_CALLBACK_VECTOR is effectively a proxy
for "x86 enabled". This approach is better because we're not concerned
about whether it is x86 or arm, what we really want to figure out
is whether we have a pre-defined vector or not.

The VMBus driver follows this pattern too.

> 
> > +
> > +static const __maybe_unused struct acpi_device_id mshv_sint_device_ids[] = {
> > +	{"MSFT1003", 0},
> > +	{"", 0},
> > +};
> > +
> > +static struct platform_driver mshv_sint_drv = {
> > +	.probe = mshv_sint_probe,
> > +	.remove = mshv_sint_remove,
> > +	.driver = {
> > +		.name = "mshv_sint",
> > +		.acpi_match_table = ACPI_PTR(mshv_sint_device_ids),
> > +		.probe_type = PROBE_FORCE_SYNCHRONOUS,
> > +	},
> > +};
> > +#endif /* HYPERVISOR_CALLBACK_VECTOR */
> > +
> > +int mshv_synic_init(void)
> > +{
> > +#ifdef HYPERVISOR_CALLBACK_VECTOR
> > +	mshv_interrupt = HYPERVISOR_CALLBACK_VECTOR;
> > +	mshv_irq = -1;
> > +	return 0;
> > +#else
> > +	int ret;
> > +
> > +	if (acpi_disabled)
> > +		return -ENODEV;
> > +
> > +	ret = platform_driver_register(&mshv_sint_drv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (mshv_interrupt == -1 || mshv_irq == -1) {
> > +		ret = -ENODEV;
> > +		goto out_unregister;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_unregister:
> > +	platform_driver_unregister(&mshv_sint_drv);
> > +	return ret;
> > +#endif
> > +}
> > +
> > +void mshv_synic_cleanup(void)
> > +{
> > +#ifndef HYPERVISOR_CALLBACK_VECTOR
> > +	if (!acpi_disabled)
> > +		platform_driver_unregister(&mshv_sint_drv);
> > +#endif
> > +}
> > +
> >  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(mshv_root.synic_pages);
> >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > @@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> >  
> >  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
> >  
> > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> > +	if (mshv_irq != -1)
> > +		enable_percpu_irq(mshv_irq, 0);
> > +
> 
> It's better to explicitly separate x86 and arm64 paths with #ifdefs.
> For example:
> 
> #ifdef CONFIG_X86_64
> int setup_cpu_sint() {
>   	/* Enable intercepts */
>   	sint.as_uint64 = 0;
> 	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> 	....
> }
> #endif
> #ifdef CONFIG_ARM64
> int setup_cpu_sint() {
> 	enable_percpu_irq(mshv_irq, 0);
> 
>   	/* Enable intercepts */
>   	sint.as_uint64 = 0;
> 	sint.vector = mshv_interrupt;
> 	....
> }
> #endif

This seems unnecessary. We've made the paths that determine
mshv_interrupt separate. Now we can just use that here.

There is no need to write two copies of 

	...
   	sint.as_uint64 = 0;
 	sint.vector = <whatever>;
	...

I could do the enable_percpu_irq() inside an ifdef. But do we gain
anything from it? Won't the compiler optimize the current code as well
since mshv_irq will always be -1 whenever HYPERVISOR_CALLBACK_VECTOR is
defined?

Thanks,
Anirudh.

> 
> Thanks,
> Stanislav
> 
> >  	/* Enable intercepts */
> >  	sint.as_uint64 = 0;
> > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > +	sint.vector = mshv_interrupt;
> >  	sint.masked = false;
> >  	sint.auto_eoi = hv_recommend_using_aeoi();
> >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> > @@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> >  
> >  	/* Doorbell SINT */
> >  	sint.as_uint64 = 0;
> > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > +	sint.vector = mshv_interrupt;
> >  	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);
> > @@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
> >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> >  			      sint.as_uint64);
> >  
> > +	if (mshv_irq != -1)
> > +		disable_percpu_irq(mshv_irq);
> > +
> >  	/* Disable Synic's event ring page */
> >  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
> >  	sirbp.sirbp_enabled = false;
> > -- 
> > 2.34.1
> >

On Thu, Jan 29, 2026 at 04:36:51AM +0000, Anirudh Rayabharam wrote:
> On Wed, Jan 28, 2026 at 03:03:51PM -0800, Stanislav Kinsburskii wrote:
> > On Wed, Jan 28, 2026 at 04:04:37PM +0000, Anirudh Rayabharam wrote:
> > > From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>

<snip>

> > 
> > > +static int mshv_irq = -1;
> > > +
> > 
> > Should this be a path of mshv_root structure?
> 
> This doesn't need to be globally accessible. It is only used in this file.
> So I guess it doesn't need to be in mshv_root. What do you think?
> 

Please, see below.

<snip>

> > >  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(mshv_root.synic_pages);
> > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > > @@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > >  
> > >  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
> > >  
> > > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> > > +	if (mshv_irq != -1)
> > > +		enable_percpu_irq(mshv_irq, 0);
> > > +
> > 
> > It's better to explicitly separate x86 and arm64 paths with #ifdefs.
> > For example:
> > 
> > #ifdef CONFIG_X86_64
> > int setup_cpu_sint() {
> >   	/* Enable intercepts */
> >   	sint.as_uint64 = 0;
> > 	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > 	....
> > }
> > #endif
> > #ifdef CONFIG_ARM64
> > int setup_cpu_sint() {
> > 	enable_percpu_irq(mshv_irq, 0);
> > 
> >   	/* Enable intercepts */
> >   	sint.as_uint64 = 0;
> > 	sint.vector = mshv_interrupt;
> > 	....
> > }
> > #endif
> 
> This seems unnecessary. We've made the paths that determine
> mshv_interrupt separate. Now we can just use that here.
> 
> There is no need to write two copies of 
> 
> 	...
>    	sint.as_uint64 = 0;
>  	sint.vector = <whatever>;
> 	...
> 
> I could do the enable_percpu_irq() inside an ifdef. But do we gain
> anything from it? Won't the compiler optimize the current code as well
> since mshv_irq will always be -1 whenever HYPERVISOR_CALLBACK_VECTOR is
> defined?
> 

AFAIU this patc, x86 doesn’t need these variables at all. So it’s better
to separate them completely and explicitly.

Also, this isn’t the only place where ARM-specific logic is added. This
patch adds ARM-specific logic and tries to weave it into the existing
x86 flow.

If it were only one place, that might be OK. But here it happens in
several places. That makes the code harder to read and maintain. It also
makes future extensions more risky (and they will likely follow). The
dependencies are also not obvious. For example, on ARM the interrupt
vector comes from ACPI (at least that’s what the comments say). So it’s
not right to mix this into the common x86 path even if
HYPERVISOR_CALLBACK_VECTOR is a x86-specific define.

It would be much better to keep this ARM-specific logic in separate,
conditionally compiled code. I suggest changing the flow to make this
per-arch logic explicit. It will pay off later.

Thanks,
Stanislav

> Thanks,
> Anirudh.
> 
> > 
> > Thanks,
> > Stanislav
> > 
> > >  	/* Enable intercepts */
> > >  	sint.as_uint64 = 0;
> > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > +	sint.vector = mshv_interrupt;
> > >  	sint.masked = false;
> > >  	sint.auto_eoi = hv_recommend_using_aeoi();
> > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> > > @@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > >  
> > >  	/* Doorbell SINT */
> > >  	sint.as_uint64 = 0;
> > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > +	sint.vector = mshv_interrupt;
> > >  	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);
> > > @@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
> > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> > >  			      sint.as_uint64);
> > >  
> > > +	if (mshv_irq != -1)
> > > +		disable_percpu_irq(mshv_irq);
> > > +
> > >  	/* Disable Synic's event ring page */
> > >  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
> > >  	sirbp.sirbp_enabled = false;
> > > -- 
> > > 2.34.1
> > > 

On Thu, Jan 29, 2026 at 09:03:54AM -0800, Stanislav Kinsburskii wrote:
> On Thu, Jan 29, 2026 at 04:36:51AM +0000, Anirudh Rayabharam wrote:
> > On Wed, Jan 28, 2026 at 03:03:51PM -0800, Stanislav Kinsburskii wrote:
> > > On Wed, Jan 28, 2026 at 04:04:37PM +0000, Anirudh Rayabharam wrote:
> > > > From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> 
> <snip>
> 
> > > 
> > > > +static int mshv_irq = -1;
> > > > +
> > > 
> > > Should this be a path of mshv_root structure?
> > 
> > This doesn't need to be globally accessible. It is only used in this file.
> > So I guess it doesn't need to be in mshv_root. What do you think?
> > 
> 
> Please, see below.

The below part doesn't make a case for this variable being part of the
mshv_root structure. Did you miss this part in your reply?

> 
> <snip>
> 
> > > >  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(mshv_root.synic_pages);
> > > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > > > @@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > > >  
> > > >  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
> > > >  
> > > > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> > > > +	if (mshv_irq != -1)
> > > > +		enable_percpu_irq(mshv_irq, 0);
> > > > +
> > > 
> > > It's better to explicitly separate x86 and arm64 paths with #ifdefs.
> > > For example:
> > > 
> > > #ifdef CONFIG_X86_64
> > > int setup_cpu_sint() {
> > >   	/* Enable intercepts */
> > >   	sint.as_uint64 = 0;
> > > 	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > 	....
> > > }
> > > #endif
> > > #ifdef CONFIG_ARM64
> > > int setup_cpu_sint() {
> > > 	enable_percpu_irq(mshv_irq, 0);
> > > 
> > >   	/* Enable intercepts */
> > >   	sint.as_uint64 = 0;
> > > 	sint.vector = mshv_interrupt;
> > > 	....
> > > }
> > > #endif
> > 
> > This seems unnecessary. We've made the paths that determine
> > mshv_interrupt separate. Now we can just use that here.
> > 
> > There is no need to write two copies of 
> > 
> > 	...
> >    	sint.as_uint64 = 0;
> >  	sint.vector = <whatever>;
> > 	...
> > 
> > I could do the enable_percpu_irq() inside an ifdef. But do we gain
> > anything from it? Won't the compiler optimize the current code as well
> > since mshv_irq will always be -1 whenever HYPERVISOR_CALLBACK_VECTOR is
> > defined?
> > 
> 
> AFAIU this patc, x86 doesn’t need these variables at all. So it’s better
> to separate them completely and explicitly.
> 
> Also, this isn’t the only place where ARM-specific logic is added. This
> patch adds ARM-specific logic and tries to weave it into the existing
> x86 flow.
> 
> If it were only one place, that might be OK. But here it happens in
> several places. That makes the code harder to read and maintain. It also
> makes future extensions more risky (and they will likely follow). The
> dependencies are also not obvious. For example, on ARM the interrupt
> vector comes from ACPI (at least that’s what the comments say). So it’s
> not right to mix this into the common x86 path even if
> HYPERVISOR_CALLBACK_VECTOR is a x86-specific define.

We shouldn't think of this code in terms of X86 & ARM64. It's not about
arch at all. It's about whether or not we have a pre-defined vector
(a.k.a HYPERVISOR_CALLBACK_VECTOR). I feel that the current code cleanly
separates the two cases. The main difference in the two cases is in how
the vector is determined which is well seperated in the code paths. Once
the vector is determined, how we program it in the synic is the same for
both cases.

> 
> It would be much better to keep this ARM-specific logic in separate,
> conditionally compiled code. I suggest changing the flow to make this
> per-arch logic explicit. It will pay off later.

Most of the code introduced in this patch is conditionally compiled.
Building code from this patch on x86 will conditionally compile out a
large majority of it.

Are you by any chance suggesting we put it in a separate file?

Thanks,
Anirudh.

> 
> Thanks,
> Stanislav
> 
> > Thanks,
> > Anirudh.
> > 
> > > 
> > > Thanks,
> > > Stanislav
> > > 
> > > >  	/* Enable intercepts */
> > > >  	sint.as_uint64 = 0;
> > > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > +	sint.vector = mshv_interrupt;
> > > >  	sint.masked = false;
> > > >  	sint.auto_eoi = hv_recommend_using_aeoi();
> > > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> > > > @@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > > >  
> > > >  	/* Doorbell SINT */
> > > >  	sint.as_uint64 = 0;
> > > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > +	sint.vector = mshv_interrupt;
> > > >  	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);
> > > > @@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
> > > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> > > >  			      sint.as_uint64);
> > > >  
> > > > +	if (mshv_irq != -1)
> > > > +		disable_percpu_irq(mshv_irq);
> > > > +
> > > >  	/* Disable Synic's event ring page */
> > > >  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
> > > >  	sirbp.sirbp_enabled = false;
> > > > -- 
> > > > 2.34.1
> > > > 

On Fri, Jan 30, 2026 at 05:09:10PM +0000, Anirudh Rayabharam wrote:
> On Thu, Jan 29, 2026 at 09:03:54AM -0800, Stanislav Kinsburskii wrote:
> > On Thu, Jan 29, 2026 at 04:36:51AM +0000, Anirudh Rayabharam wrote:
> > > On Wed, Jan 28, 2026 at 03:03:51PM -0800, Stanislav Kinsburskii wrote:
> > > > On Wed, Jan 28, 2026 at 04:04:37PM +0000, Anirudh Rayabharam wrote:
> > > > > From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> > 
> > <snip>
> > 
> > > > 
> > > > > +static int mshv_irq = -1;
> > > > > +
> > > > 
> > > > Should this be a path of mshv_root structure?
> > > 
> > > This doesn't need to be globally accessible. It is only used in this file.
> > > So I guess it doesn't need to be in mshv_root. What do you think?
> > > 
> > 
> > Please, see below.
> 
> The below part doesn't make a case for this variable being part of the
> mshv_root structure. Did you miss this part in your reply?
> 

No, I didn't miss it. I just don't see the point of introducing there
variables unless the goal is to weave more logic into the existent flow.

> > 
> > <snip>
> > 
> > > > >  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(mshv_root.synic_pages);
> > > > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > > > > @@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > > > >  
> > > > >  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
> > > > >  
> > > > > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> > > > > +	if (mshv_irq != -1)
> > > > > +		enable_percpu_irq(mshv_irq, 0);
> > > > > +
> > > > 
> > > > It's better to explicitly separate x86 and arm64 paths with #ifdefs.
> > > > For example:
> > > > 
> > > > #ifdef CONFIG_X86_64
> > > > int setup_cpu_sint() {
> > > >   	/* Enable intercepts */
> > > >   	sint.as_uint64 = 0;
> > > > 	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > 	....
> > > > }
> > > > #endif
> > > > #ifdef CONFIG_ARM64
> > > > int setup_cpu_sint() {
> > > > 	enable_percpu_irq(mshv_irq, 0);
> > > > 
> > > >   	/* Enable intercepts */
> > > >   	sint.as_uint64 = 0;
> > > > 	sint.vector = mshv_interrupt;
> > > > 	....
> > > > }
> > > > #endif
> > > 
> > > This seems unnecessary. We've made the paths that determine
> > > mshv_interrupt separate. Now we can just use that here.
> > > 
> > > There is no need to write two copies of 
> > > 
> > > 	...
> > >    	sint.as_uint64 = 0;
> > >  	sint.vector = <whatever>;
> > > 	...
> > > 
> > > I could do the enable_percpu_irq() inside an ifdef. But do we gain
> > > anything from it? Won't the compiler optimize the current code as well
> > > since mshv_irq will always be -1 whenever HYPERVISOR_CALLBACK_VECTOR is
> > > defined?
> > > 
> > 
> > AFAIU this patc, x86 doesn’t need these variables at all. So it’s better
> > to separate them completely and explicitly.
> > 
> > Also, this isn’t the only place where ARM-specific logic is added. This
> > patch adds ARM-specific logic and tries to weave it into the existing
> > x86 flow.
> > 
> > If it were only one place, that might be OK. But here it happens in
> > several places. That makes the code harder to read and maintain. It also
> > makes future extensions more risky (and they will likely follow). The
> > dependencies are also not obvious. For example, on ARM the interrupt
> > vector comes from ACPI (at least that’s what the comments say). So it’s
> > not right to mix this into the common x86 path even if
> > HYPERVISOR_CALLBACK_VECTOR is a x86-specific define.
> 
> We shouldn't think of this code in terms of X86 & ARM64. It's not about
> arch at all. It's about whether or not we have a pre-defined vector
> (a.k.a HYPERVISOR_CALLBACK_VECTOR). I feel that the current code cleanly
> separates the two cases. The main difference in the two cases is in how
> the vector is determined which is well seperated in the code paths. Once
> the vector is determined, how we program it in the synic is the same for
> both cases.
> 

The major question is whether HYPERVISOR_CALLBACK_VECTOR can be
defined on ARM. If it can’t, then it’s effectively an x86-only feature.

The current code separates two cases. You are adding a third one: ARM,
with its own logic. But this is not stated explicitly in the code. As a
result, we now have three cases mixed together, and the flow becomes
spaghetti-like.

If we ever need to support DT on ARM (and we should expect that, because
ACPI on ARM looks odd), we will need to add yet another case to this
mix.

I hope you see the problem. The original code wasn't designed to be
extensible. Since you are adding a new case, this is a good opportunity
to redesign the flow and make it more extensible, instead of adding more
logic on top.

> > 
> > It would be much better to keep this ARM-specific logic in separate,
> > conditionally compiled code. I suggest changing the flow to make this
> > per-arch logic explicit. It will pay off later.
> 
> Most of the code introduced in this patch is conditionally compiled.
> Building code from this patch on x86 will conditionally compile out a
> large majority of it.
> 
> Are you by any chance suggesting we put it in a separate file?
> 

No, I’m not suggesting to move it into a separate file yet.
But making the arch-specific code clearly separated would be a good first step.

Thanks,
Stanislav.

> Thanks,
> Anirudh.
> 
> > 
> > Thanks,
> > Stanislav
> > 
> > > Thanks,
> > > Anirudh.
> > > 
> > > > 
> > > > Thanks,
> > > > Stanislav
> > > > 
> > > > >  	/* Enable intercepts */
> > > > >  	sint.as_uint64 = 0;
> > > > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > > +	sint.vector = mshv_interrupt;
> > > > >  	sint.masked = false;
> > > > >  	sint.auto_eoi = hv_recommend_using_aeoi();
> > > > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> > > > > @@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > > > >  
> > > > >  	/* Doorbell SINT */
> > > > >  	sint.as_uint64 = 0;
> > > > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > > +	sint.vector = mshv_interrupt;
> > > > >  	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);
> > > > > @@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
> > > > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> > > > >  			      sint.as_uint64);
> > > > >  
> > > > > +	if (mshv_irq != -1)
> > > > > +		disable_percpu_irq(mshv_irq);
> > > > > +
> > > > >  	/* Disable Synic's event ring page */
> > > > >  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
> > > > >  	sirbp.sirbp_enabled = false;
> > > > > -- 
> > > > > 2.34.1
> > > > > 

On Fri, Jan 30, 2026 at 11:00:51AM -0800, Stanislav Kinsburskii wrote:
> On Fri, Jan 30, 2026 at 05:09:10PM +0000, Anirudh Rayabharam wrote:
> > On Thu, Jan 29, 2026 at 09:03:54AM -0800, Stanislav Kinsburskii wrote:
> > > On Thu, Jan 29, 2026 at 04:36:51AM +0000, Anirudh Rayabharam wrote:
> > > > On Wed, Jan 28, 2026 at 03:03:51PM -0800, Stanislav Kinsburskii wrote:
> > > > > On Wed, Jan 28, 2026 at 04:04:37PM +0000, Anirudh Rayabharam wrote:
> > > > > > From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> > > 
> > > <snip>
> > > 
> > > > > 
> > > > > > +static int mshv_irq = -1;
> > > > > > +
> > > > > 
> > > > > Should this be a path of mshv_root structure?
> > > > 
> > > > This doesn't need to be globally accessible. It is only used in this file.
> > > > So I guess it doesn't need to be in mshv_root. What do you think?
> > > > 
> > > 
> > > Please, see below.
> > 
> > The below part doesn't make a case for this variable being part of the
> > mshv_root structure. Did you miss this part in your reply?
> > 
> 
> No, I didn't miss it. I just don't see the point of introducing there
> variables unless the goal is to weave more logic into the existent flow.

I introduced the variables in this file because they're only used in
this file. How would moving the variables to the mshv_root structure
help with the code weaving problem?

> 
> > > 
> > > <snip>
> > > 
> > > > > >  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(mshv_root.synic_pages);
> > > > > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > > > > > @@ -496,10 +632,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > > > > >  
> > > > > >  	hv_set_non_nested_msr(HV_MSR_SIRBP, sirbp.as_uint64);
> > > > > >  
> > > > > > -#ifdef HYPERVISOR_CALLBACK_VECTOR
> > > > > > +	if (mshv_irq != -1)
> > > > > > +		enable_percpu_irq(mshv_irq, 0);
> > > > > > +
> > > > > 
> > > > > It's better to explicitly separate x86 and arm64 paths with #ifdefs.
> > > > > For example:
> > > > > 
> > > > > #ifdef CONFIG_X86_64
> > > > > int setup_cpu_sint() {
> > > > >   	/* Enable intercepts */
> > > > >   	sint.as_uint64 = 0;
> > > > > 	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > > 	....
> > > > > }
> > > > > #endif
> > > > > #ifdef CONFIG_ARM64
> > > > > int setup_cpu_sint() {
> > > > > 	enable_percpu_irq(mshv_irq, 0);
> > > > > 
> > > > >   	/* Enable intercepts */
> > > > >   	sint.as_uint64 = 0;
> > > > > 	sint.vector = mshv_interrupt;
> > > > > 	....
> > > > > }
> > > > > #endif
> > > > 
> > > > This seems unnecessary. We've made the paths that determine
> > > > mshv_interrupt separate. Now we can just use that here.
> > > > 
> > > > There is no need to write two copies of 
> > > > 
> > > > 	...
> > > >    	sint.as_uint64 = 0;
> > > >  	sint.vector = <whatever>;
> > > > 	...
> > > > 
> > > > I could do the enable_percpu_irq() inside an ifdef. But do we gain
> > > > anything from it? Won't the compiler optimize the current code as well
> > > > since mshv_irq will always be -1 whenever HYPERVISOR_CALLBACK_VECTOR is
> > > > defined?
> > > > 
> > > 
> > > AFAIU this patc, x86 doesn’t need these variables at all. So it’s better
> > > to separate them completely and explicitly.
> > > 
> > > Also, this isn’t the only place where ARM-specific logic is added. This
> > > patch adds ARM-specific logic and tries to weave it into the existing
> > > x86 flow.
> > > 
> > > If it were only one place, that might be OK. But here it happens in
> > > several places. That makes the code harder to read and maintain. It also
> > > makes future extensions more risky (and they will likely follow). The
> > > dependencies are also not obvious. For example, on ARM the interrupt
> > > vector comes from ACPI (at least that’s what the comments say). So it’s
> > > not right to mix this into the common x86 path even if
> > > HYPERVISOR_CALLBACK_VECTOR is a x86-specific define.
> > 
> > We shouldn't think of this code in terms of X86 & ARM64. It's not about
> > arch at all. It's about whether or not we have a pre-defined vector
> > (a.k.a HYPERVISOR_CALLBACK_VECTOR). I feel that the current code cleanly
> > separates the two cases. The main difference in the two cases is in how
> > the vector is determined which is well seperated in the code paths. Once
> > the vector is determined, how we program it in the synic is the same for
> > both cases.
> > 
> 
> The major question is whether HYPERVISOR_CALLBACK_VECTOR can be
> defined on ARM. If it can’t, then it’s effectively an x86-only feature.

It cannot be defined for ARM. Just not possible with the way interrupts
are allocated on ARM.

> 
> The current code separates two cases. You are adding a third one: ARM,

The current code only really handles one case: where
HYPERVISOR_CALLBACK_VECTOR is defined. The other case is not handled at
all.

There is no case called ARM. The only two cases are:

  - HYPERVISOR_CALLBACK_VECTOR is defined
  - HYPERVISOR_CALLBACK_VECTOR is not defined

> with its own logic. But this is not stated explicitly in the code. As a
> result, we now have three cases mixed together, and the flow becomes
> spaghetti-like.
> 
> If we ever need to support DT on ARM (and we should expect that, because
> ACPI on ARM looks odd), we will need to add yet another case to this
> mix.

Nope there won't be another case. DT on ARM would fall under the
"HYPERVISOR_CALLBACK_VECTOR is not defined" case. Under that case, we
would check if we're using ACPI or DT and take the appropriate action.

Please see vmbus_drv.c, a similar approach is used there.

> 
> I hope you see the problem. The original code wasn't designed to be
> extensible. Since you are adding a new case, this is a good opportunity
> to redesign the flow and make it more extensible, instead of adding more
> logic on top.

I'll be sending a v2 soon where I believe I've cleaned this up
a little bit more. Let's see...

Thanks,
Anirudh.

> 
> > > 
> > > It would be much better to keep this ARM-specific logic in separate,
> > > conditionally compiled code. I suggest changing the flow to make this
> > > per-arch logic explicit. It will pay off later.
> > 
> > Most of the code introduced in this patch is conditionally compiled.
> > Building code from this patch on x86 will conditionally compile out a
> > large majority of it.
> > 
> > Are you by any chance suggesting we put it in a separate file?
> > 
> 
> No, I’m not suggesting to move it into a separate file yet.
> But making the arch-specific code clearly separated would be a good first step.
> 
> Thanks,
> Stanislav.
> 
> > Thanks,
> > Anirudh.
> > 
> > > 
> > > Thanks,
> > > Stanislav
> > > 
> > > > Thanks,
> > > > Anirudh.
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Stanislav
> > > > > 
> > > > > >  	/* Enable intercepts */
> > > > > >  	sint.as_uint64 = 0;
> > > > > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > > > +	sint.vector = mshv_interrupt;
> > > > > >  	sint.masked = false;
> > > > > >  	sint.auto_eoi = hv_recommend_using_aeoi();
> > > > > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
> > > > > > @@ -507,13 +645,12 @@ int mshv_synic_cpu_init(unsigned int cpu)
> > > > > >  
> > > > > >  	/* Doorbell SINT */
> > > > > >  	sint.as_uint64 = 0;
> > > > > > -	sint.vector = HYPERVISOR_CALLBACK_VECTOR;
> > > > > > +	sint.vector = mshv_interrupt;
> > > > > >  	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);
> > > > > > @@ -568,6 +705,9 @@ int mshv_synic_cpu_exit(unsigned int cpu)
> > > > > >  	hv_set_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_DOORBELL_SINT_INDEX,
> > > > > >  			      sint.as_uint64);
> > > > > >  
> > > > > > +	if (mshv_irq != -1)
> > > > > > +		disable_percpu_irq(mshv_irq);
> > > > > > +
> > > > > >  	/* Disable Synic's event ring page */
> > > > > >  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
> > > > > >  	sirbp.sirbp_enabled = false;
> > > > > > -- 
> > > > > > 2.34.1
> > > > > >