Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On Wed, Nov 25, 2020 at 12:48:22PM +0100, Claudio Fontana wrote: > 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()? Yes. Anything that requires MachineState (and is softmmu-specific) would go to ->machine_init(). Anything that is not softmmu-specific would go to ->init(). > > This would make tcg even more different from the other accelerators. That's true, but isn't this only because TCG is the only one that really needs it? > > 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? That would be possible (and maybe a good idea), but not necessary to make it work. > > 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); That would work too. I would implement it as an accel_init(NULL) call, however, to avoid duplicating the code from accel_init_machine(). Calling ->machine_init(NULL) is just a bit surprising because of the name (calling machine_init() when there's no machine), and because we know most accelerators will crash if getting a NULL argument. Anyway, the split between ->machine_init() and ->init() is just a suggestion. Keeping a single init method that accepts a NULL MachineState* as argument is not my favourite option, but it works. Whatever you choose, my only ask is to document clearly the expectations and requirements of the AccelClass methods you are using. > > 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 Thank you very much for your patience! I think we're going on the right direction. -- Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
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! >> > >
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
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(). 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
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
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(). > >> > >> > >> > >> 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? > > 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(). > > > > 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. > > 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! -- Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
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(). >> >> >> >> 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). 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. > > 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? > 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. 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! Claudio
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
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(). > > > > 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? accel_init_machine() has 2-3 lines of code with side effects. It calls AccelClass.init_machine(), which may may have hundreds of 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? -- Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On 11/24/20 6:48 PM, Eduardo Habkost wrote:
> On Tue, Nov 24, 2020 at 05:22:10PM +0100, Claudio Fontana wrote:
>> Signed-off-by: Claudio Fontana
>> ---
>> accel/kvm/kvm-all.c | 9 ---
>> accel/kvm/kvm-cpus.c | 26 +-
>> accel/kvm/kvm-cpus.h | 2 --
>> accel/qtest/qtest.c | 31 --
>> accel/tcg/tcg-cpus-icount.c | 11 +---
>> accel/tcg/tcg-cpus-icount.h | 2 ++
>> accel/tcg/tcg-cpus-mttcg.c | 12 +++--
>> accel/tcg/tcg-cpus-mttcg.h | 19 ++
>> accel/tcg/tcg-cpus-rr.c | 7 -
>> accel/tcg/tcg-cpus.c | 48 ++---
>> accel/tcg/tcg-cpus.h | 4 ---
>> accel/xen/xen-all.c | 29 ++--
>> include/sysemu/cpus.h| 39 ---
>> softmmu/cpus.c | 51 +---
>> target/i386/hax/hax-all.c| 9 ---
>> target/i386/hax/hax-cpus.c | 29 +++-
>> target/i386/hax/hax-cpus.h | 2 --
>> target/i386/hvf/hvf-cpus.c | 27 ++-
>> target/i386/hvf/hvf-cpus.h | 2 --
>> target/i386/hvf/hvf.c| 9 ---
>> target/i386/whpx/whpx-all.c | 9 ---
>> target/i386/whpx/whpx-cpus.c | 29 +++-
>> target/i386/whpx/whpx-cpus.h | 2 --
>> 23 files changed, 251 insertions(+), 157 deletions(-)
>> create mode 100644 accel/tcg/tcg-cpus-mttcg.h
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 509b249f52..33156cc4c7 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3234,12 +3234,3 @@ static void kvm_type_init(void)
>> }
>>
>> type_init(kvm_type_init);
>> -
>> -static void kvm_accel_cpu_init(void)
>> -{
>> -if (kvm_enabled()) {
>> -cpus_register_accel(_cpus);
>> -}
>> -}
>> -
>> -accel_cpu_init(kvm_accel_cpu_init);
>> diff --git a/accel/kvm/kvm-cpus.c b/accel/kvm/kvm-cpus.c
>> index d809b1e74c..33dc8e737a 100644
>> --- a/accel/kvm/kvm-cpus.c
>> +++ b/accel/kvm/kvm-cpus.c
>> @@ -74,11 +74,25 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
>> cpu, QEMU_THREAD_JOINABLE);
>> }
>>
>> -const CpusAccel kvm_cpus = {
>> -.create_vcpu_thread = kvm_start_vcpu_thread,
>> +static void kvm_cpus_class_init(ObjectClass *oc, void *data)
>> +{
>> +CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc);
>
> Why do you need a separate QOM type hierarchy instead of just
> doing this inside AccelClass methods and/or existing accel
> class_init functions?
>
>>
>> -.synchronize_post_reset = kvm_cpu_synchronize_post_reset,
>> -.synchronize_post_init = kvm_cpu_synchronize_post_init,
>> -.synchronize_state = kvm_cpu_synchronize_state,
>> -.synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm,
>> +ops->create_vcpu_thread = kvm_start_vcpu_thread;
>> +ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset;
>> +ops->synchronize_post_init = kvm_cpu_synchronize_post_init;
>> +ops->synchronize_state = kvm_cpu_synchronize_state;
>> +ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm;
>
> All of these could be AccelClass fields.
Makes sense, and I like also the idea below (to have a pointer from the
AccelClass to the Ops).
I'll give both a try.
>
> TCG makes it a bit more complicated because there's a different
> set of methods chosen for TYPE_TCG_ACCEL depending on the
> configuration.
Right, that was a bit painful,
> This could be solved by patching AccelClass at
> init time, or by moving the method pointers to AccelState.
Ok I'll experiment here.
>
> Alternatively, if you still want to keep the
> CpusAccel/CpusAccelOps struct, that's OK. You can just add a
> `CpusAccel *cpu_accel_ops` field to AccelClass or AccelState. No
> need for a separate QOM hierarchy, either.
>
> If you _really_ want a separate TYPE_CPU_ACCEL_OPS QOM type, you
No, I do not think I really need a separate QOM type.
> can still have it. But it can be just an interface implemented
> by each accel subclass, instead of requiring a separate
> CPUS_ACCEL_TYPE_NAME(...) type to be registered for each
> accelerator. (I don't see why you would want it, though.)
>
>
>> };
>> +static const TypeInfo kvm_cpus_type_info = {
>> +.name = CPUS_ACCEL_TYPE_NAME("kvm"),
>> +
>> +.parent = TYPE_CPUS_ACCEL_OPS,
>> +.class_init = kvm_cpus_class_init,
>> +.abstract = true,
>> +};
>> +static void kvm_cpus_register_types(void)
>> +{
>> +type_register_static(_cpus_type_info);
>> +}
>> +type_init(kvm_cpus_register_types);
> [...]
>> -typedef struct CpusAccel {
>> -void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY */
>> +typedef struct CpusAccelOps CpusAccelOps;
>> +DECLARE_CLASS_CHECKERS(CpusAccelOps, CPUS_ACCEL_OPS, TYPE_CPUS_ACCEL_OPS)
>> +
>> +struct CpusAccelOps {
>> +ObjectClass parent_class;
>> +
>> +/* initialization function called when accel is chosen */
>> +void
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On Tue, Nov 24, 2020 at 05:22:10PM +0100, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana
> ---
> accel/kvm/kvm-all.c | 9 ---
> accel/kvm/kvm-cpus.c | 26 +-
> accel/kvm/kvm-cpus.h | 2 --
> accel/qtest/qtest.c | 31 --
> accel/tcg/tcg-cpus-icount.c | 11 +---
> accel/tcg/tcg-cpus-icount.h | 2 ++
> accel/tcg/tcg-cpus-mttcg.c | 12 +++--
> accel/tcg/tcg-cpus-mttcg.h | 19 ++
> accel/tcg/tcg-cpus-rr.c | 7 -
> accel/tcg/tcg-cpus.c | 48 ++---
> accel/tcg/tcg-cpus.h | 4 ---
> accel/xen/xen-all.c | 29 ++--
> include/sysemu/cpus.h| 39 ---
> softmmu/cpus.c | 51 +---
> target/i386/hax/hax-all.c| 9 ---
> target/i386/hax/hax-cpus.c | 29 +++-
> target/i386/hax/hax-cpus.h | 2 --
> target/i386/hvf/hvf-cpus.c | 27 ++-
> target/i386/hvf/hvf-cpus.h | 2 --
> target/i386/hvf/hvf.c| 9 ---
> target/i386/whpx/whpx-all.c | 9 ---
> target/i386/whpx/whpx-cpus.c | 29 +++-
> target/i386/whpx/whpx-cpus.h | 2 --
> 23 files changed, 251 insertions(+), 157 deletions(-)
> create mode 100644 accel/tcg/tcg-cpus-mttcg.h
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 509b249f52..33156cc4c7 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3234,12 +3234,3 @@ static void kvm_type_init(void)
> }
>
> type_init(kvm_type_init);
> -
> -static void kvm_accel_cpu_init(void)
> -{
> -if (kvm_enabled()) {
> -cpus_register_accel(_cpus);
> -}
> -}
> -
> -accel_cpu_init(kvm_accel_cpu_init);
> diff --git a/accel/kvm/kvm-cpus.c b/accel/kvm/kvm-cpus.c
> index d809b1e74c..33dc8e737a 100644
> --- a/accel/kvm/kvm-cpus.c
> +++ b/accel/kvm/kvm-cpus.c
> @@ -74,11 +74,25 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
> cpu, QEMU_THREAD_JOINABLE);
> }
>
> -const CpusAccel kvm_cpus = {
> -.create_vcpu_thread = kvm_start_vcpu_thread,
> +static void kvm_cpus_class_init(ObjectClass *oc, void *data)
> +{
> +CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc);
Why do you need a separate QOM type hierarchy instead of just
doing this inside AccelClass methods and/or existing accel
class_init functions?
>
> -.synchronize_post_reset = kvm_cpu_synchronize_post_reset,
> -.synchronize_post_init = kvm_cpu_synchronize_post_init,
> -.synchronize_state = kvm_cpu_synchronize_state,
> -.synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm,
> +ops->create_vcpu_thread = kvm_start_vcpu_thread;
> +ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset;
> +ops->synchronize_post_init = kvm_cpu_synchronize_post_init;
> +ops->synchronize_state = kvm_cpu_synchronize_state;
> +ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm;
All of these could be AccelClass fields.
TCG makes it a bit more complicated because there's a different
set of methods chosen for TYPE_TCG_ACCEL depending on the
configuration. This could be solved by patching AccelClass at
init time, or by moving the method pointers to AccelState.
Alternatively, if you still want to keep the
CpusAccel/CpusAccelOps struct, that's OK. You can just add a
`CpusAccel *cpu_accel_ops` field to AccelClass or AccelState. No
need for a separate QOM hierarchy, either.
If you _really_ want a separate TYPE_CPU_ACCEL_OPS QOM type, you
can still have it. But it can be just an interface implemented
by each accel subclass, instead of requiring a separate
CPUS_ACCEL_TYPE_NAME(...) type to be registered for each
accelerator. (I don't see why you would want it, though.)
> };
> +static const TypeInfo kvm_cpus_type_info = {
> +.name = CPUS_ACCEL_TYPE_NAME("kvm"),
> +
> +.parent = TYPE_CPUS_ACCEL_OPS,
> +.class_init = kvm_cpus_class_init,
> +.abstract = true,
> +};
> +static void kvm_cpus_register_types(void)
> +{
> +type_register_static(_cpus_type_info);
> +}
> +type_init(kvm_cpus_register_types);
[...]
> -typedef struct CpusAccel {
> -void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY */
> +typedef struct CpusAccelOps CpusAccelOps;
> +DECLARE_CLASS_CHECKERS(CpusAccelOps, CPUS_ACCEL_OPS, TYPE_CPUS_ACCEL_OPS)
> +
> +struct CpusAccelOps {
> +ObjectClass parent_class;
> +
> +/* initialization function called when accel is chosen */
> +void (*accel_chosen_init)(CpusAccelOps *ops);
This can be an AccelClass method too. What about just naming it
AccelClass.init?
> +
> +void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */
> void (*kick_vcpu_thread)(CPUState *cpu);
>
> void (*synchronize_post_reset)(CPUState *cpu);
> @@ -20,13 +45,7 @@ typedef struct CpusAccel {
>
> int64_t (*get_virtual_clock)(void);
> int64_t (*get_elapsed_ticks)(void);
> -}
