Message-ID: <aX0YYbynhbiSnw_o@anirudh-surface.localdomain>
Date: Fri, 30 Jan 2026 20:45:21 +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 Fri, Jan 30, 2026 at 11:00:51AM -0800, Stanislav Kinsburskii wrote:
> 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.

I introduced the variables in this file because they're only used in
this file. How would moving the variables to the mshv_root structure
help with the code weaving problem?

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

It cannot be defined for ARM. Just not possible with the way interrupts
are allocated on ARM.

> 
> The current code separates two cases. You are adding a third one: ARM,

The current code only really handles one case: where
HYPERVISOR_CALLBACK_VECTOR is defined. The other case is not handled at
all.

There is no case called ARM. The only two cases are:

  - HYPERVISOR_CALLBACK_VECTOR is defined
  - HYPERVISOR_CALLBACK_VECTOR is not defined

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

Nope there won't be another case. DT on ARM would fall under the
"HYPERVISOR_CALLBACK_VECTOR is not defined" case. Under that case, we
would check if we're using ACPI or DT and take the appropriate action.

Please see vmbus_drv.c, a similar approach is used there.

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

I'll be sending a v2 soon where I believe I've cleaned this up
a little bit more. Let's see...

Thanks,
Anirudh.

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