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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aXzltmZVDhYIDiaw@anirudh-surface.localdomain>
Date: Fri, 30 Jan 2026 17:09:10 +0000
From: Anirudh Rayabharam <anirudh@...rudhrb.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
Cc: kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
	decui@...rosoft.com, longli@...rosoft.com,
	linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mshv: add arm64 support for doorbell & intercept
 SINTs

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@...rudhrb.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
> > > >