| 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
| ||
|
Message-ID: <aYIo7Ft25Wy5L4pc@skinsburskii.localdomain>
Date: Tue, 3 Feb 2026 08:57:16 -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 v2 1/2] mshv: refactor synic init and cleanup
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@...rudhrb.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@...rudhrb.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
> > >
Powered by blists - more mailing lists