| Message ID | 1470588578.9967963.1659494457934@mail-kr2-1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] xen: add late init call in start_xen | expand |
Hi Boyoun, On 03/08/2022 03:40, Boyoun Park wrote: > Thank you for your reply. I will seriously consider contributing > platform specific drivers. Currently, I make a sample usage by applying this > to a function in start_xen. I think functions in start_xen could be reduced > for readability and understandability. > These patches are just one of my suggestion. So feel free to reject it. Please avoid top-posting. Also, I would suggest to read through [1] to check how patch series should be submitted. For instance... > > On Sat, 30 Jul 2022, Stefano Stabellini wrote: >> On Fri, 29 Jul 2022, Boyoun Park wrote: >>> I really appreciate all the comments and reviews. >>> I understand that it is hard to add this patch without any usage. >>> >>> On Fri, 29 Jul 2022, Stefano Stabellini: >>>> On Thu, 28 Jul 2022, Jan Beulich wrote: >>>>> On 28.07.2022 11:22, Boyoun Park wrote: >>>>>> Subject: [PATCH v1] xen: add late init call in start_xen >>>>>> >>>>>> This patch added late_initcall section in init.data. >>>>>> >>>>>> The late initcall would be called after initcall >>>>>> >>>>>> in the start_xen function. >>>>>> >>>>>> Some initializing works on priority should be run >>>>>> >>>>>> in do_initcalls and other non-prioritized works >>>>>> >>>>>> would be run in do_late_initcalls. >>>>>> >>>>>> To call some functions by late_initcall, >>>>>> >>>>>> then it is possible by using >>>>>> >>>>>> __late_initcall(/*Function Name*/); >>>>>> >>>>>> Signed-off-by: Boyoun Park <boyoun.park@samsung.com> >>>>> >>>>> So I could imagine this patch to be in a series where a subsequent >>>>> patch then adds an actual use of the new functionality. Without >>>>> that what you're proposing is to add dead code. >>>> >>>> Yeah, I think it would be cool to have late initcalls but it makes sense >>>> to add them if we have someone that makes use of them. >>> >>> I totally agree with your comments. Some drivers that I'm developing now and use this function are specific to my hardware environment. >>> Thus, it seems difficult to arrange them in the near future. >>> I will update patches if I can suggest an actual use. >> >> I totally understand custom setups and non-upstreamable configurations >> and I have certainly some of them myself. >> >> However, I just wanted to let you know that we are fine with accepting >> platform specific drivers in Xen where it makes sense, for instance: >> >> - Renesas IPMMU-VMSA found in R-Car Gen3/Gen4 SoCs (an IOMMU driver) >> xen/drivers/passthrough/arm/ipmmu-vmsa.c >> >> - Xilinx EEMI firmware interface "mediator" in Xen (power management) >> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > > From: Boyoun Park <boyoun.park@samsung.com> > Date: Tue, 15 Mar 2022 12:57:59 +0900 > Subject: [PATCH v2 1/2] xen: add late init call in start_xen A new version of a series is usually sent as a separate thread rather than in-reply to the previous version. Furthermore, as there is 2 patches, you need to provide a cover letter (even it doesn't contain much). Lastly (process wise), please provide a list of changes made in v2. I tend to prefer per-patch changelog, but one in the cover letter would also work. > > This patch added late_initcall section in init.data. > The late initcall would be called after initcall > in the start_xen function. I think this is a bit too vague. AFAIU, you want late_initcall() to happen *after* all the domains have been created but *before* they are unpaused. Is that correct? From the previous discussion, I saw you said you have drivers needing to call initlate. Could you briefly explain why they can't be called in initcall? > > Some initializing works on priority should be run > in do_initcalls and other non-prioritized works > would be run in do_late_initcalls. IIUC, you are saying that do_late_initcalls() was introduced for prioritization purpose. But then, there are also a difference in behavior (initcalls happens before creating the domains whereas late happens after). Therefore, if the priority is the only reasons, then I think we should introduce priority within the initcalls. > > To call some functions by late_initcall, > then it is possible by using > __late_initcall(/*Function Name*/); > > Signed-off-by: Boyoun Park <boyoun.park@samsung.com> > --- > xen/arch/arm/setup.c | 2 ++ > xen/arch/arm/xen.lds.S | 2 ++ > xen/arch/x86/setup.c | 2 ++ > xen/arch/x86/xen.lds.S | 2 ++ > xen/common/kernel.c | 9 ++++++++- > xen/include/xen/init.h | 3 +++ > 6 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 85ff956..332a207 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -1063,6 +1063,8 @@ void __init start_xen(unsigned long boot_phys_offset, > /* Hide UART from DOM0 if we're using it */ > serial_endboot(); > > + do_late_initcalls(); > + [...] > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index f08b07b..5dc6654 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1952,6 +1952,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > setup_io_bitmap(dom0); > > + do_late_initcalls(); > + It would be preferable if the call is done roughly at the same place on all architecture. So if it easier for a developer to know when this will be called (e.g. just after serial_endboot()). If you need to call the function at the different place, then I think this ought to be explained. Cheers, [1] https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
On Wed, 3 Aug 2022, Julien Grall wrote: > Hi Boyoun, > > On 03/08/2022 03:40, Boyoun Park wrote: >> From: Boyoun Park <boyoun.park@samsung.com> >> Date: Tue, 15 Mar 2022 12:57:59 +0900 >> Subject: [PATCH v2 1/2] xen: add late init call in start_xen >> >> This patch added late_initcall section in init.data. >> The late initcall would be called after initcall >> in the start_xen function. > >I think this is a bit too vague. AFAIU, you want late_initcall() to >happen *after* all the domains have been created but *before* they are >unpaused. Is that correct? > > From the previous discussion, I saw you said you have drivers needing >to call initlate. Could you briefly explain why they can't be called in >initcall? > >> >> Some initializing works on priority should be run >> in do_initcalls and other non-prioritized works >> would be run in do_late_initcalls. > >IIUC, you are saying that do_late_initcalls() was introduced for >prioritization purpose. But then, there are also a difference in >behavior (initcalls happens before creating the domains whereas late >happens after). > >Therefore, if the priority is the only reasons, then I think we should >introduce priority within the initcalls. When I made the patch for the first time, there was a problem related to memory access from some of my drivers with original initcall which is not the problem in Xen mainline. But it is resolved now, so for my case, it seems that the location when it is called is more important now as you said. Most of my drivers are domain-specific so they are called in late boot time after creating domains. Similar to priority within the initcalls, I think it could be subdivided according to the functions' own purposes such as arch_initcall, device_initcall, and others in Linux Kernel. >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index f08b07b..5dc6654 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1952,6 +1952,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> >> setup_io_bitmap(dom0); >> >> + do_late_initcalls(); >> + > >It would be preferable if the call is done roughly at the same place on >all architecture. So if it easier for a developer to know when this will >be called (e.g. just after serial_endboot()). > >If you need to call the function at the different place, then I think >this ought to be explained. I understand your comments. The reason of the location should be explained. I also checked Jan's review. All of the reviews including yours are reasonable. It would be better to add this function with my specific drivers after removing lots of unclean codes. Thank you for all of your replies. Although it is my first time to send patches to open-source project, it helps a lot to understand what kinds of codes are appropriate to be contributed. I will make new thread if I can improve the ideas and patches. With gratitude, Boyoun Park
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 85ff956..332a207 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -1063,6 +1063,8 @@ void __init start_xen(unsigned long boot_phys_offset, /* Hide UART from DOM0 if we're using it */ serial_endboot(); + do_late_initcalls(); + if ( (rc = xsm_set_system_active()) != 0 ) panic("xsm: unable to switch to SYSTEM_ACTIVE privilege: %d\n", rc); diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 1e986e2..215e2c3 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -163,6 +163,8 @@ SECTIONS __presmp_initcall_end = .; *(.initcall1.init) __initcall_end = .; + *(.initcalllate.init) + __late_initcall_end = .; . = ALIGN(4); __alt_instructions = .; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f08b07b..5dc6654 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1952,6 +1952,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) setup_io_bitmap(dom0); + do_late_initcalls(); + if ( bsp_delay_spec_ctrl ) { info->spec_ctrl_flags &= ~SCF_use_shadow; diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 8930e14..c90c7b0 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -250,6 +250,8 @@ SECTIONS __presmp_initcall_end = .; *(.initcall1.init) __initcall_end = .; + *(.initcalllate.init) + __late_initcall_end = .; *(.init.data) *(.init.data.rel) diff --git a/xen/common/kernel.c b/xen/common/kernel.c index f8134d3..5a3d037 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -369,7 +369,7 @@ void add_taint(unsigned int flag) } extern const initcall_t __initcall_start[], __presmp_initcall_end[], - __initcall_end[]; + __initcall_end[], __late_initcall_end[]; void __init do_presmp_initcalls(void) { @@ -385,6 +385,13 @@ void __init do_initcalls(void) (*call)(); } +void __init do_late_initcalls(void) +{ + const initcall_t *call; + for ( call = __initcall_end; call < __late_initcall_end; call++ ) + (*call)(); +} + #ifdef CONFIG_HYPFS static unsigned int __read_mostly major_version; static unsigned int __read_mostly minor_version; diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h index 0af0e23..48210ee 100644 --- a/xen/include/xen/init.h +++ b/xen/include/xen/init.h @@ -68,11 +68,14 @@ typedef void (*exitcall_t)(void); const static initcall_t __initcall_##fn __init_call("presmp") = fn #define __initcall(fn) \ const static initcall_t __initcall_##fn __init_call("1") = fn +#define __late_initcall(fn) \ + const static initcall_t __initcall_##fn __init_call("late") = fn #define __exitcall(fn) \ static exitcall_t __exitcall_##fn __exit_call = fn void do_presmp_initcalls(void); void do_initcalls(void); +void do_late_initcalls(void); #endif /* __ASSEMBLY__ */