Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 11/25/20 10:30 AM, Paolo Bonzini wrote:
> On 25/11/20 10:21, Claudio Fontana wrote:
>> Hi Paolo,
>>
>> in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right?
>> But the fact that its behavior depends on current_accel() still disqualifies
>> it?
>> It is called right after the accelerator is chosen and initialized
>> in RFC v5, this still is "in the middle of the machine creation sequence"?
> Yes, machine creation basically starts after command line parsing, or
> perhaps even _with_ command line parsing. Basically once the user can
> control the flow it is already too late.
>
>> I am trying to find the actual things to fix, since when doing RFC
>> v5 I tried to specifically address two points:
>>
>> 1) no if () inside module init functions
>>
>> 2) no proliferation of module init functions
>>
>> which I accomplished via AccelClass extension to user mode, current_accel(),
>> and class lookup.
>
> Yes, the rest is great, I'm just not sure that MODULE_INIT_ACCEL_CPU is
> useful and if virtual functions on accel and CPU_RESOLVING_TYPE can
> achieve the same.
>
>> If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the
>> call so that it is not "in the middle"?
>
> No later than the runstate_init() call, roughly.
>
> Paolo
>
Hi Paolo, not super-related to the context above,
during the implementation I am trying to offer
accel/accel-softmmu.c
accel/accel-common.c
accel/accel-user.c
But I don't seem able to use CONFIG_USER_ONLY in accel-common.c .
in accel/meson.build I am saying
common_ss.add(files('accel-common.c'))
softmmu_ss.add(files('accel-softmmu.c'))
user_ss.add(files('accel-user.c'))
But this doesn't work, if I use common_ss. If I use specific_ss, it works.
So the term "common" is a bit overloaded, it means stuff for libcommon and
such, not common between softmmu and user, right?
Ciao,
Claudio
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 11/26/20 12:25 PM, Philippe Mathieu-Daudé wrote: > On 11/24/20 5:22 PM, Claudio Fontana wrote: >> apply this to the registration of the cpus accel interfaces, >> >> but this will be also in preparation for later use of this >> new module init step to also register per-accel x86 cpu type >> interfaces. >> >> Signed-off-by: Claudio Fontana >> --- >> accel/kvm/kvm-all.c | 11 +-- >> accel/qtest/qtest.c | 10 +- >> accel/tcg/tcg-all.c | 8 >> accel/tcg/tcg-cpus.c| 15 +++ >> accel/xen/xen-all.c | 12 +--- >> include/qemu/module.h | 2 ++ >> roms/qboot | 2 +- >> softmmu/vl.c| 6 ++ >> target/i386/hax/hax-all.c | 12 +--- >> target/i386/hvf/hvf.c | 10 +- >> target/i386/whpx/whpx-all.c | 11 +-- >> 11 files changed, 78 insertions(+), 21 deletions(-) > ... >> diff --git a/roms/qboot b/roms/qboot >> index a5300c4949..cb1c49e0cf 16 >> --- a/roms/qboot >> +++ b/roms/qboot >> @@ -1 +1 @@ >> -Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948 >> +Subproject commit cb1c49e0cfac99b9961d136ac0194da62c28cf64 > > Hmmm unrelated change I presume. > Hi Philippe, yes, clearly, Thanks! Ciao, CLaudio
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 11/24/20 5:22 PM, Claudio Fontana wrote: > apply this to the registration of the cpus accel interfaces, > > but this will be also in preparation for later use of this > new module init step to also register per-accel x86 cpu type > interfaces. > > Signed-off-by: Claudio Fontana > --- > accel/kvm/kvm-all.c | 11 +-- > accel/qtest/qtest.c | 10 +- > accel/tcg/tcg-all.c | 8 > accel/tcg/tcg-cpus.c| 15 +++ > accel/xen/xen-all.c | 12 +--- > include/qemu/module.h | 2 ++ > roms/qboot | 2 +- > softmmu/vl.c| 6 ++ > target/i386/hax/hax-all.c | 12 +--- > target/i386/hvf/hvf.c | 10 +- > target/i386/whpx/whpx-all.c | 11 +-- > 11 files changed, 78 insertions(+), 21 deletions(-) ... > diff --git a/roms/qboot b/roms/qboot > index a5300c4949..cb1c49e0cf 16 > --- a/roms/qboot > +++ b/roms/qboot > @@ -1 +1 @@ > -Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948 > +Subproject commit cb1c49e0cfac99b9961d136ac0194da62c28cf64 Hmmm unrelated change I presume.
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 11/25/20 10:30 AM, Paolo Bonzini wrote: > On 25/11/20 10:21, Claudio Fontana wrote: >> Hi Paolo, >> >> in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? >> But the fact that its behavior depends on current_accel() still disqualifies >> it? >> It is called right after the accelerator is chosen and initialized >> in RFC v5, this still is "in the middle of the machine creation sequence"? > Yes, machine creation basically starts after command line parsing, or > perhaps even _with_ command line parsing. Basically once the user can > control the flow it is already too late. > >> I am trying to find the actual things to fix, since when doing RFC >> v5 I tried to specifically address two points: >> >> 1) no if () inside module init functions >> >> 2) no proliferation of module init functions >> >> which I accomplished via AccelClass extension to user mode, current_accel(), >> and class lookup. > > Yes, the rest is great, I'm just not sure that MODULE_INIT_ACCEL_CPU is > useful and if virtual functions on accel and CPU_RESOLVING_TYPE can > achieve the same. > >> If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the >> call so that it is not "in the middle"? > > No later than the runstate_init() call, roughly. > > Paolo > > Aha! Then that solves it, I don't think it is feasible to put it that early with the meaning I intended for it. Thanks for clarifying this, Claudio
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 25/11/20 10:21, Claudio Fontana wrote: Hi Paolo, in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? But the fact that its behavior depends on current_accel() still disqualifies it? It is called right after the accelerator is chosen and initialized in RFC v5, this still is "in the middle of the machine creation sequence"? Yes, machine creation basically starts after command line parsing, or perhaps even _with_ command line parsing. Basically once the user can control the flow it is already too late. I am trying to find the actual things to fix, since when doing RFC v5 I tried to specifically address two points: 1) no if () inside module init functions 2) no proliferation of module init functions which I accomplished via AccelClass extension to user mode, current_accel(), and class lookup. Yes, the rest is great, I'm just not sure that MODULE_INIT_ACCEL_CPU is useful and if virtual functions on accel and CPU_RESOLVING_TYPE can achieve the same. If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the call so that it is not "in the middle"? No later than the runstate_init() call, roughly. Paolo
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 11/24/20 9:01 PM, Paolo Bonzini wrote: > On 24/11/20 20:08, Eduardo Habkost wrote: >>> Not a big advantage I agree, >>> I think however there is one, in using the existing framework that exists, >>> for the purposes that it was built for. >>> >>> As I understand it, the global module init framework is supposed to mark >>> the major initialization steps, >>> and this seems to fit the bill. >> That seems to be the main source of disagreement. I don't agree >> that's the purpose of module_init(). >> >> The module init framework is used to unconditionally register >> module-provided entities like option names, QOM types, block >> drivers, trace events, etc. The entities registered by module >> init functions represent a passive dynamically loadable piece of >> code. > > Indeed. Think of module_init() as C++ global constructors. > > Anything that has an "if" does not belong in module_init. > > If you look at my review of the previous versions, I was not necessarily > against MODULE_INIT_ACCEL_CPU, but I was (and am) strongly against > calling it in the middle of the machine creation sequence. > > Paolo > > Hi Paolo, in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? But the fact that its behavior depends on current_accel() still disqualifies it? It is called right after the accelerator is chosen and initialized in RFC v5, this still is "in the middle of the machine creation sequence"? I am trying to find the actual things to fix, since when doing RFC v5 I tried to specifically address two points: 1) no if () inside module init functions 2) no proliferation of module init functions which I accomplished via AccelClass extension to user mode, current_accel(), and class lookup. If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the call so that it is not "in the middle"? It is interesting for me to try to discern which meaning you folks give to MODULE_INIT. Keep in mind, I will experiment with Eduardo's option of "one accel_init() to rule them all", without MODULE_INIT_ACCEL_CPU, so I am not focused on using this no matter what. Ciao, Claudio
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 24/11/20 20:08, Eduardo Habkost wrote: Not a big advantage I agree, I think however there is one, in using the existing framework that exists, for the purposes that it was built for. As I understand it, the global module init framework is supposed to mark the major initialization steps, and this seems to fit the bill. That seems to be the main source of disagreement. I don't agree that's the purpose of module_init(). The module init framework is used to unconditionally register module-provided entities like option names, QOM types, block drivers, trace events, etc. The entities registered by module init functions represent a passive dynamically loadable piece of code. Indeed. Think of module_init() as C++ global constructors. Anything that has an "if" does not belong in module_init. If you look at my review of the previous versions, I was not necessarily against MODULE_INIT_ACCEL_CPU, but I was (and am) strongly against calling it in the middle of the machine creation sequence. Paolo
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On Tue, Nov 24, 2020 at 07:29:50PM +0100, Claudio Fontana wrote:
> On 11/24/20 6:08 PM, Eduardo Habkost wrote:
> > On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote:
> >> apply this to the registration of the cpus accel interfaces,
> >>
> >> but this will be also in preparation for later use of this
> >> new module init step to also register per-accel x86 cpu type
> >> interfaces.
> >>
> >> Signed-off-by: Claudio Fontana
> >> ---
> > [...]
> >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> >> index b4e731cb2b..482f89729f 100644
> >> --- a/accel/qtest/qtest.c
> >> +++ b/accel/qtest/qtest.c
> >> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = {
> >>
> >> static int qtest_init_accel(MachineState *ms)
> >> {
> >> -cpus_register_accel(_cpus);
> >> return 0;
> >> }
> >>
> >> @@ -58,3 +57,12 @@ static void qtest_type_init(void)
> >> }
> >>
> >> type_init(qtest_type_init);
> >> +
> >> +static void qtest_accel_cpu_init(void)
> >> +{
> >> +if (qtest_enabled()) {
> >> +cpus_register_accel(_cpus);
> >> +}
> >> +}
> >> +
> >> +accel_cpu_init(qtest_accel_cpu_init);
> >
> > I don't understand why this (and the similar changes on other
> > accelerators) is an improvement.
> >
> > You are replacing a trivial AccelClass-specific init method with
> > a module_init() function that has a hidden dependency on runtime
> > state.
> >
>
> Not a big advantage I agree,
> I think however there is one, in using the existing framework that exists,
> for the purposes that it was built for.
>
> As I understand it, the global module init framework is supposed to mark the
> major initialization steps,
> and this seems to fit the bill.
That seems to be the main source of disagreement. I don't agree
that's the purpose of module_init().
The module init framework is used to unconditionally register
module-provided entities like option names, QOM types, block
drivers, trace events, etc. The entities registered by module
init functions represent a passive dynamically loadable piece of
code.
module_init() was never used for initialization of machines,
devices, CPUs, or other runtime state. We don't have
MODULE_INIT_MONITOR, MODULE_INIT_OBJECTS, MODULE_INIT_MACHINE,
MODULE_INIT_CPUS, MODULE_INIT_DEVICES, etc.
And I'm not convinced we should, because it would hide
dependencies between initialization steps. It would force us to
make initialization functions affect and depend on global state.
I believe this:
int main()
{
result_of_A = init_A(input_for_A);
result_of_B = init_B(input_for_B);
result_of_C = init_C(input_for_C);
}
is clearer and more maintainable than:
int main()
{
module_init_call(MODULE_INIT_A); /* result_of_A hidden in global state */
module_init_call(MODULE_INIT_B); /* result_of_B hidden in global state */
module_init_call(MODULE_INIT_C); /* result_of_C hidden in global state */
}
>
> The "hidden" dependency on the fact that accels need to be initialized at
> that time, is not hidden at all I think,
> it is what this module init step is all about.
>
> It is explicitly meaning, "_now that the current accelerator is chosen_,
> perform these initializations".
>
> But, as you mentioned elsewhere, I will in the meantime anyway squash these
> things so they do not start fragmented at all, and centralize immediately.
Agreed. We still need to sort out the disagreement above, or
we'll spend a lot of energy arguing about code.
--
Eduardo
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On 11/24/20 6:08 PM, Eduardo Habkost wrote:
> On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote:
>> apply this to the registration of the cpus accel interfaces,
>>
>> but this will be also in preparation for later use of this
>> new module init step to also register per-accel x86 cpu type
>> interfaces.
>>
>> Signed-off-by: Claudio Fontana
>> ---
> [...]
>> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
>> index b4e731cb2b..482f89729f 100644
>> --- a/accel/qtest/qtest.c
>> +++ b/accel/qtest/qtest.c
>> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = {
>>
>> static int qtest_init_accel(MachineState *ms)
>> {
>> -cpus_register_accel(_cpus);
>> return 0;
>> }
>>
>> @@ -58,3 +57,12 @@ static void qtest_type_init(void)
>> }
>>
>> type_init(qtest_type_init);
>> +
>> +static void qtest_accel_cpu_init(void)
>> +{
>> +if (qtest_enabled()) {
>> +cpus_register_accel(_cpus);
>> +}
>> +}
>> +
>> +accel_cpu_init(qtest_accel_cpu_init);
>
> I don't understand why this (and the similar changes on other
> accelerators) is an improvement.
>
> You are replacing a trivial AccelClass-specific init method with
> a module_init() function that has a hidden dependency on runtime
> state.
>
Not a big advantage I agree,
I think however there is one, in using the existing framework that exists, for
the purposes that it was built for.
As I understand it, the global module init framework is supposed to mark the
major initialization steps,
and this seems to fit the bill.
The "hidden" dependency on the fact that accels need to be initialized at that
time, is not hidden at all I think,
it is what this module init step is all about.
It is explicitly meaning, "_now that the current accelerator is chosen_,
perform these initializations".
But, as you mentioned elsewhere, I will in the meantime anyway squash these
things so they do not start fragmented at all, and centralize immediately.
Thanks,
Claudio
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote:
> apply this to the registration of the cpus accel interfaces,
>
> but this will be also in preparation for later use of this
> new module init step to also register per-accel x86 cpu type
> interfaces.
>
> Signed-off-by: Claudio Fontana
> ---
[...]
> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> index b4e731cb2b..482f89729f 100644
> --- a/accel/qtest/qtest.c
> +++ b/accel/qtest/qtest.c
> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = {
>
> static int qtest_init_accel(MachineState *ms)
> {
> -cpus_register_accel(_cpus);
> return 0;
> }
>
> @@ -58,3 +57,12 @@ static void qtest_type_init(void)
> }
>
> type_init(qtest_type_init);
> +
> +static void qtest_accel_cpu_init(void)
> +{
> +if (qtest_enabled()) {
> +cpus_register_accel(_cpus);
> +}
> +}
> +
> +accel_cpu_init(qtest_accel_cpu_init);
I don't understand why this (and the similar changes on other
accelerators) is an improvement.
You are replacing a trivial AccelClass-specific init method with
a module_init() function that has a hidden dependency on runtime
state.
--
Eduardo
