On Wed, Feb 18, 2026 at 04:17:29AM +0000, Michael Kelley wrote: > From: Anirudh Rayabharam <[email protected]> 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) <[email protected]> > > --- > > 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, ®); > > + 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.
