Message-ID: <aXz_4yWC6Jofqygj@skinsburskii.localdomain>
Date: Fri, 30 Jan 2026 11:00:51 -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 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@...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?
> 

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

Powered by blists - more mailing lists