Re: [PATCH v2 1/2] mshv: refactor synic init and cleanup

[Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>]

On Tue, Feb 03, 2026 at 10:19:10AM +0530, Anirudh Rayabharam wrote:
> On Mon, Feb 02, 2026 at 11:07:17AM -0800, Stanislav Kinsburskii wrote:
> > On Mon, Feb 02, 2026 at 06:27:05PM +0000, Anirudh Rayabharam wrote:
> > > From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> > > 
> > > Rename mshv_synic_init() to mshv_synic_cpu_init() and
> > > mshv_synic_cleanup() to mshv_synic_cpu_exit() to better reflect that
> > > these functions handle per-cpu synic setup and teardown.
> > > 
> > > Use mshv_synic_init/cleanup() to perform init/cleanup that is not per-cpu.
> > > Move all the synic related setup from mshv_parent_partition_init.
> > > 
> > > Move the reboot notifier to mshv_synic.c because it currently only
> > > operates on the synic cpuhp state.
> > > 
> > > Move out synic_pages from the global mshv_root since it's use is now
> > > completely local to mshv_synic.c.
> > > 
> > > This is in preparation for the next patch which will add more stuff to
> > > mshv_synic_init().
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>

<snip>

> > > diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> > > index f8b0337cdc82..98c58755846d 100644
> > > --- a/drivers/hv/mshv_synic.c
> > > +++ b/drivers/hv/mshv_synic.c
> > > @@ -12,11 +12,16 @@
> > >  #include <linux/mm.h>
> > >  #include <linux/io.h>
> > >  #include <linux/random.h>
> > > +#include <linux/cpuhotplug.h>
> > > +#include <linux/reboot.h>
> > >  #include <asm/mshyperv.h>
> > >  
> > >  #include "mshv_eventfd.h"
> > >  #include "mshv.h"
> > >  
> > > +static int synic_cpuhp_online;
> > > +static struct hv_synic_pages __percpu *synic_pages;
> > > +
> > >  static u32 synic_event_ring_get_queued_port(u32 sint_index)
> > >  {
> > >  	struct hv_synic_event_ring_page **event_ring_page;
> > > @@ -26,7 +31,7 @@ static u32 synic_event_ring_get_queued_port(u32 sint_index)
> > >  	u32 message;
> > >  	u8 tail;
> > >  
> > > -	spages = this_cpu_ptr(mshv_root.synic_pages);
> > > +	spages = this_cpu_ptr(synic_pages);
> > >  	event_ring_page = &spages->synic_event_ring_page;
> > >  	synic_eventring_tail = (u8 **)this_cpu_ptr(hv_synic_eventring_tail);
> > >  
> > > @@ -393,7 +398,7 @@ mshv_intercept_isr(struct hv_message *msg)
> > >  
> > >  void mshv_isr(void)
> > >  {
> > > -	struct hv_synic_pages *spages = this_cpu_ptr(mshv_root.synic_pages);
> > > +	struct hv_synic_pages *spages = this_cpu_ptr(synic_pages);
> > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > >  	struct hv_message *msg;
> > >  	bool handled;
> > > @@ -446,7 +451,7 @@ void mshv_isr(void)
> > >  	}
> > >  }
> > >  
> > > -int mshv_synic_init(unsigned int cpu)
> > > +static int mshv_synic_cpu_init(unsigned int cpu)
> > >  {
> > >  	union hv_synic_simp simp;
> > >  	union hv_synic_siefp siefp;
> > > @@ -455,7 +460,7 @@ int mshv_synic_init(unsigned int cpu)
> > >  	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_synic_pages *spages = this_cpu_ptr(synic_pages);
> > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > >  	struct hv_synic_event_flags_page **event_flags_page =
> > >  			&spages->synic_event_flags_page;
> > > @@ -542,14 +547,14 @@ int mshv_synic_init(unsigned int cpu)
> > >  	return -EFAULT;
> > >  }
> > >  
> > > -int mshv_synic_cleanup(unsigned int cpu)
> > > +static int mshv_synic_cpu_exit(unsigned int cpu)
> > >  {
> > >  	union hv_synic_sint sint;
> > >  	union hv_synic_simp simp;
> > >  	union hv_synic_siefp siefp;
> > >  	union hv_synic_sirbp sirbp;
> > >  	union hv_synic_scontrol sctrl;
> > > -	struct hv_synic_pages *spages = this_cpu_ptr(mshv_root.synic_pages);
> > > +	struct hv_synic_pages *spages = this_cpu_ptr(synic_pages);
> > >  	struct hv_message_page **msg_page = &spages->hyp_synic_message_page;
> > >  	struct hv_synic_event_flags_page **event_flags_page =
> > >  		&spages->synic_event_flags_page;
> > > @@ -663,3 +668,57 @@ mshv_unregister_doorbell(u64 partition_id, int doorbell_portid)
> > >  
> > >  	mshv_portid_free(doorbell_portid);
> > >  }
> > > +
> > > +static int mshv_synic_reboot_notify(struct notifier_block *nb,
> > > +			      unsigned long code, void *unused)
> > > +{
> > > +	cpuhp_remove_state(synic_cpuhp_online);
> > > +	return 0;
> > > +}
> > > +
> > > +static struct notifier_block mshv_synic_reboot_nb = {
> > > +	.notifier_call = mshv_synic_reboot_notify,
> > > +};
> > > +
> > > +int __init mshv_synic_init(struct device *dev)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	synic_pages = alloc_percpu(struct hv_synic_pages);
> > > +	if (!synic_pages) {
> > > +		dev_err(dev, "Failed to allocate percpu synic page\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mshv_synic",
> > > +				mshv_synic_cpu_init,
> > > +				mshv_synic_cpu_exit);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "Failed to setup cpu hotplug state: %i\n", ret);
> > > +		goto free_synic_pages;
> > > +	}
> > > +
> > > +	synic_cpuhp_online = ret;
> > > +
> > > +	if (hv_root_partition()) {
> > 
> > Nit: it's probably better to branch in the notifier itself.
> > It will introduce an additional object, but the branching will be in one
> > palce instead of two and it will also make to code simpler and easier to
> > read.
> 
> Maybe I introduce mshv_synic_root_partition_init/exit() which will have
> branching inside? Similar to what we did in mshv_root_main.c. That will
> avoid introducing the additional object. But I guess the branch will
> still be in both init and exit functions...
> 

This is a matter of taste, but from my POV, in general, less code is
better. The reboot notifier (or device shutdown) hook is not a hot path.
Also, we will need to do work there for L1VH eventually anyway. So
keeping the distinction in the callback makes more sense to me.

Thanks,
Stanislav

> Thanks,
> Anirudh.
> 
> > 
> > Thanks
> > Stanislav.
> > 
> > > +		ret = register_reboot_notifier(&mshv_synic_reboot_nb);
> > > +		if (ret)
> > > +			goto remove_cpuhp_state;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +remove_cpuhp_state:
> > > +	cpuhp_remove_state(synic_cpuhp_online);
> > > +free_synic_pages:
> > > +	free_percpu(synic_pages);
> > > +	return ret;
> > > +}
> > > +
> > > +void mshv_synic_cleanup(void)
> > > +{
> > > +	if (hv_root_partition())
> > > +		unregister_reboot_notifier(&mshv_synic_reboot_nb);
> > > +	cpuhp_remove_state(synic_cpuhp_online);
> > > +	free_percpu(synic_pages);
> > > +}
> > > -- 
> > > 2.34.1
> > >