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

[Anirudh Rayabharam <anirudh@anirudhrb.com>]

On Mon, Feb 23, 2026 at 08:49:37PM +0000, Michael Kelley wrote:
> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Monday, February 23, 2026 11:40 AM
> > 
> 
> [snip]
> 
> > > +
> > > +static int __init mshv_sint_vector_init(void)
> > > +{
> > > +	int ret;
> > > +	struct hv_register_assoc reg = {
> > > +		.name = HV_ARM64_REGISTER_SINT_RESERVED_INTERRUPT_ID,
> > > +	};
> > > +	union hv_input_vtl input_vtl = { 0 };
> > > +
> > > +	if (acpi_disabled)
> > > +		return -ENODEV;
> > > +
> > > +	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 = mshv_acpi_setup_sint_irq();
> > > +	if (ret <= 0) {
> > > +		pr_err("Failed to setup IRQ for MSHV SINT vector %d: %d\n",
> > > +			mshv_sint_vector, ret);
> > > +		goto out_fail;
> > > +	}
> > > +
> > > +	mshv_sint_irq = ret;
> > 
> > nit: given that mshv_sint_irq can't be zero, the logic can be simplified by
> > using 0 instead of -1.
> 
> The test for <= 0 is actually wrong -- it should be just < 0. Zero is a valid
> Linux IRQ number. For example, here's the output of /proc/interrupts on
> a Gen1 VM on Hyper-V, where IRQ 0 is used by the legacy timer:
> 
> root@gen1ubun:~# cat /proc/interrupts
>            CPU0       CPU1       CPU2       CPU3
>   0:         18          0          0          0 IR-IO-APIC   2-edge      timer
>   1:          0          9          0          0 IR-IO-APIC   1-edge      i8042
>   4:          0          0          0        792 IR-IO-APIC   4-edge      ttyS0
>   6:          6          0          0          0 IR-IO-APIC   6-edge      floppy
>   8:          0          0          0          0 IR-IO-APIC   8-edge      rtc0
>   9:          0          0          0          0 IR-IO-APIC   9-fasteoi   acpi
> 
> But I see other places throughout Linux kernel code that treat IRQ 0 as
> invalid. So I dunno .... But it's probably better to treat 0 as a valid IRQ
> number.

Agreed. I will fix this check in v6.

Thanks,
Anirudh.

> 
> Michael
> 
> > 
> > 
> > 
> > > +
> > > +	ret = request_percpu_irq(mshv_sint_irq, mshv_percpu_isr, "MSHV",
> > > +		&mshv_evt);
> > > +	if (ret)
> > > +		goto out_unregister;
> > > +
> > > +	return 0;
> > > +
> > > +out_unregister:
> > > +	mshv_acpi_cleanup_sint_irq();
> > > +out_fail:
> > > +	return ret;
> > > +}
> > > +
> > > +static void mshv_sint_vector_cleanup(void)
> > > +{
> > > +	free_percpu_irq(mshv_sint_irq, &mshv_evt);
> > > +	mshv_acpi_cleanup_sint_irq();
> > > +}
> > > +#else /* !HYPERVISOR_CALLBACK_VECTOR */
> > > +static int __init mshv_sint_vector_init(void)
> > 
> > nit: `init` is usually paired with `exit` or `fini`, so maybe `cleanup` can be
> > renamed to `exit` as well for better consistency?
> > 
> > Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> > 
> > > +{
> > > +	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)
> > > +		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 +810,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 +820,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
> > >
>