qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps


From: Claudio Fontana
Subject: Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
Date: Wed, 25 Nov 2020 12:48:22 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0

On 11/25/20 10:32 AM, Claudio Fontana wrote:
> On 11/24/20 9:34 PM, Eduardo Habkost wrote:
>> On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote:
>>> On 11/24/20 8:27 PM, Eduardo Habkost wrote:
>>>> On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote:
>>>> [...]
>>>>>>> +    }
>>>>>>
>>>>>> Additionally, if you call arch_cpu_accel_init() here, you won't
>>>>>> need MODULE_INIT_ACCEL_CPU anymore.  The
>>>>>>
>>>>>>   module_call_init(MODULE_INIT_ACCEL_CPU)
>>>>>>
>>>>>> call with implicit dependencies on runtime state inside vl.c and
>>>>>> *-user/main.c becomes a trivial:
>>>>>>
>>>>>>   accel_init(accel)
>>>>>>
>>>>>> call in accel_init_machine() and *-user:main().


On this one I see an issue:

the *-user_main() would still need an ac->machine_init() call to initialize tcg 
itself,
currently the accelerator initialization is put into ac->machine_init

(tcg_init, kvm_init, xen_init, etc).

Or are you proposing to move tcg initialization away from the current 
->machine_init(),
into the new ac->init called by accel_init()?

This would make tcg even more different from the other accelerators.

Or are you proposing for all accelerators to separate the initialization of the 
accelerator itself
from the machine state input, leading to, for example, separating kvm-all.c 
kvm_init() into two
functions, one which takes the input from MachineState and puts it into the 
AccelState, and
another one which actually then initializes kvm proper? And same for all accels?

In my view we could still do in *-user main.c,

ac = ACCEL_GET_CLASS(current_accel())
ac->machine_init(NULL);
ac->init_cpu_interfaces(ac);

to solve this, or something like that, but also the option of fixing all 
accelerators to separate
the gathering of the input from the MachineState to the actual accelerator 
initialization is
a possibility to me.

Ciao,

Claudio


>>>>>
>>>>>
>>>>>
>>>>> I do need a separate thing for the arch cpu accel specialization though,
>>>>> without MODULE_INIT_ACCEL_CPU that link between all operations done at 
>>>>> accel-chosen time is missing..
>>>>>
>>>>
>>>> I think this is a key point we need to sort out.
>>>>
>>>> What do you mean by "link between all operations done at
>>>> accel-chosen time" and why that's important?
>>>
>>>
>>> For understanding by a reader that tries to figure this out,
>>> (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread).
>>
>> Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU)
>> indirection makes this easier to figure out than just looking at
>> a accel_init() function that makes regular function calls?
> 
> 
> I agree, if we accomplish a single accel_init() call that does everything 
> (accelerator initialization and arch independent ops initialization and arch 
> dependent specialization of the x86 cpu),
> that would be the best outcome in my view also.
> 
> 
>>
>>
>>>
>>> And it could be that the high level plan to make accelerators fully 
>>> dynamically loadable plugins in the future
>>> also conditioned me to want to have a very clear demarcation line around 
>>> the choice of the accelerator.
>>
>> We have dynamically loadable modules for other QOM types,
>> already, and they just use type_init().  I don't see why an extra
>> module_init() type makes this easier.
>>
>>>
>>>
>>>>
>>>> accel_init_machine() has 2-3 lines of code with side effects.  It
>>>> calls AccelClass.init_machine(), which may may have hundreds of
>>>
>>>
>>> could we initialize also all arch-dependent stuff in here?
>>
>> You can, if you use a wrapper + stub, like arch_cpu_accel_init().
>>
> 
> As mentioned elsewhere, I'll try to avoid stubs. One is too many I think in 
> the codebase (well one is probably ok, but you get what I mean, I don't like 
> their proliferation).
> 
>>
>>>
>>>
>>>> lines of code.  accel_setup_post() has one additional method
>>>> call, which is triggered at a slightly different moment.
>>>>
>>>> You are using MODULE_INIT_ACCEL for 2 additional lines of code:
>>>> - the cpus_register_accel() call
>>>> - the x86_cpu_accel_init() call
>>>>
>>>> What makes those 2 lines of code so special, to make them deserve
>>>> a completely new mechanism to trigger them, instead of using
>>>> trivial function calls inside a accel_init() function?
>>>>
>>>
>>> ...can we do also the x86_cpu_accel_init inside accel_init()?
>>>
>>>
>>> In any case I'll try also the alternative, it would be nice if I could 
>>> bring everything together under the same roof,
>>> and easily discoverable, both the arch-specific steps that we need to do at 
>>> accel-chosen time and the non-arch-specific steps.
>>
>> One way to bring everything together under the same roof is to
>> call everything inside a accel_init() function.
> 
> Will try!
> 
> 
>>
>>
>>>
>>> Hope this helps clarifying where I am coming from,
>>> but I am open to have my mind changed, also trying the alternatives you 
>>> propose here could help me see first hand how things play out.
>>
>> Thanks!
>>
> 
> Thanks,
> 
> Ciao,
> 
> Claudio
> 







reply via email to

[Prev in Thread] Current Thread [Next in Thread]