Re: [PATCH v14 18/22] accel: introduce AccelCPUClass extending CPUClass

[Claudio Fontana <cfontana@suse.de>]

On 1/28/21 2:03 PM, Philippe Mathieu-Daudé wrote:
> On 1/28/21 10:28 AM, Claudio Fontana wrote:
>> add a new optional interface to CPUClass,
>> which allows accelerators to extend the CPUClass
>> with additional accelerator-specific initializations.
>>
>> Add the field before tcg_ops, and mark tcg_ops as
>> needing to be last in the struct until we rework this
>> further in a later patch.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  include/hw/core/accel-cpu.h | 35 +++++++++++++++++++++++++++++
>>  include/hw/core/cpu.h       |  1 +
>>  accel/accel-common.c        | 44 +++++++++++++++++++++++++++++++++++++
>>  MAINTAINERS                 |  1 +
>>  4 files changed, 81 insertions(+)
>>  create mode 100644 include/hw/core/accel-cpu.h
>>
>> diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h
>> new file mode 100644
>> index 0000000000..246b3e2fcb
>> --- /dev/null
>> +++ b/include/hw/core/accel-cpu.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * Accelerator interface, specializes CPUClass
>> + *
>> + * Copyright 2020 SUSE LLC
> 
> 2020-2021 ;)

Thanks Philippe,

will check them all! It's a bright new year!

> 
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef ACCEL_CPU_H
>> +#define ACCEL_CPU_H
>> +
>> +/*
>> + * these defines cannot be in cpu.h, because we are using
>> + * CPU_RESOLVING_TYPE here.
>> + * Use this header to define your accelerator-specific
>> + * cpu-specific accelerator interfaces.
>> + */
>> +
>> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
>> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
>> +typedef struct AccelCPUClass AccelCPUClass;
>> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU)
>> +
>> +typedef struct AccelCPUClass {
>> +    /*< private >*/
>> +    ObjectClass parent_class;
>> +    /*< public >*/
>> +
>> +    void (*cpu_class_init)(CPUClass *cc);
>> +    void (*cpu_instance_init)(CPUState *cpu);
>> +    void (*cpu_realizefn)(CPUState *cpu, Error **errp);
> 
> If we want callers to check errp, better have the prototype return
> a boolean.

Good point, the whole errp thing is worth revisiting in the series,
there are many cases (which are basically reproduced in the refactoring from existing code),
where errp is passed but is really unused.

I am constantly internally debating whether to remove the parameter altogether, or to keep it in there.

What would you suggest?

> 
>> +} AccelCPUClass;
> 
> 

Thanks for looking at this,

Claudio