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

[Boyoun Park <boyoun.park@xxxxxxxxxxx>]

On Wed, 3 Aug 2022, Julien Grall wrote:
> Hi Boyoun,
> 
> On 03/08/2022 03:40, Boyoun Park wrote:
>> 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
>> 
>> 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