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] [day] [month] [year] [list]

Message-ID: <aXuS-ogiBX2Z3Gnf@skinsburskii.localdomain>
Date: Thu, 29 Jan 2026 09:03:54 -0800
From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
To: Anirudh Rayabharam <anirudh@...rudhrb.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 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.

<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
> > >