Re: [PATCH v2 1/2] xen: add late init call in start_xen

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall