Re: [PATCH v4 02/11] target/loongarch: Add new object class for loongarch32 cpus

[Richard Henderson <richard.henderson@linaro.org>]

On 8/7/23 18:54, Jiajie Chen wrote:
> Add object class for future loongarch32 cpus. It is derived from the
> loongarch64 object class.
> 
> Signed-off-by: Jiajie Chen <c@jia.je>
> ---
>   target/loongarch/cpu.c | 24 ++++++++++++++++++++++++
>   target/loongarch/cpu.h | 11 +++++++++++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index ad93ecac92..3bd293d00a 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -732,6 +732,10 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>   #endif
>   }
>   
> +static void loongarch32_cpu_class_init(ObjectClass *c, void *data)
> +{
> +}
> +
>   #define DEFINE_LOONGARCH_CPU_TYPE(model, initfn) \
>       { \
>           .parent = TYPE_LOONGARCH_CPU, \
> @@ -754,3 +758,23 @@ static const TypeInfo loongarch_cpu_type_infos[] = {
>   };
>   
>   DEFINE_TYPES(loongarch_cpu_type_infos)
> +
> +#define DEFINE_LOONGARCH32_CPU_TYPE(model, initfn) \
> +    { \
> +        .parent = TYPE_LOONGARCH32_CPU, \
> +        .instance_init = initfn, \
> +        .name = LOONGARCH_CPU_TYPE_NAME(model), \
> +    }
> +
> +static const TypeInfo loongarch32_cpu_type_infos[] = {
> +    {
> +        .name = TYPE_LOONGARCH32_CPU,
> +        .parent = TYPE_LOONGARCH_CPU,
> +        .instance_size = sizeof(LoongArchCPU),
> +
> +        .abstract = true,
> +        .class_size = sizeof(LoongArchCPUClass),
> +        .class_init = loongarch32_cpu_class_init,
> +    },
> +};

You don't need to create a new array, you can put these into the existing 
loongarch_cpu_type_infos[] like so:


static const TypeInfo loongarch_cpu_type_infos[] = {
     {
         .name = TYPE_LOONGARCH_CPU,
         ...
     },
     {
         .name = TYPE_LOONGARCH32_CPU,
         ...
     },
     DEFINE_LOONGARCH_CPU_TYPE("la464", loongarch_la464_initfn),
     DEFINE_LOONGARCH32_CPU_TYPE("la132", loongarch_la132_initfn),
};


> +#define TYPE_LOONGARCH32_CPU "loongarch32-cpu"
> +typedef struct LoongArch32CPUClass LoongArch32CPUClass;
> +DECLARE_CLASS_CHECKERS(LoongArch32CPUClass, LOONGARCH32_CPU,
> +                       TYPE_LOONGARCH32_CPU)
> +
> +struct LoongArch32CPUClass {
> +    /*< private >*/
> +    LoongArchCPUClass parent_class;
> +    /*< public >*/
> +};

You don't need to declare another struct if it's just a wrapper.
If you do declare another struct, then you must actually use it in the .class_size 
initializer.


Also, I've noticed two existing bugs:

(1) Missing alignment on fprs, which is required by tcg_gen_gvec_*:

   typedef struct CPUArchState {
       uint64_t gpr[32];
       uint64_t pc;

-     fpr_t fpr[32];
+     fpr_t fpr[32] QEMU_ALIGNED(16);


(2) Missing instance_align on the class:

     .instance_align = __alignof(LoongArchCPU),


r~