Re: [PATCH v4 3/5] target/riscv: Move misa_mxl_max to class

[Daniel Henrique Barboza]

On 10/18/23 10:31, Akihiko Odaki wrote:
> On 2023/10/18 22:01, Daniel Henrique Barboza wrote:
> > On 10/17/23 15:53, Akihiko Odaki wrote:
> > > misa_mxl_max is common for all instances of a RISC-V CPU class so they
> > > are better put into class.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> >
> > I'll repeat what I said in the v1: this patch is adding an extra class 
> > parameter,
> > an extra param required to each class_init, and an extra CPUClass cast every 
> > time
> > we want to read misa_mxl_max, all of that because we want to assign 
> > gdb_core_xml_file
> > earlier.
> >
> > If my previous suggestion of assigning gdb_core directly into 
> > riscv_cpu_class_init()
> > doesn't work necause we need misa_mxl_max to do it, a good alternative is 
> > setting
> > gdb_core_xml_file in riscv_cpu_post_init(), which is executed after all 
> > cpu_init()
> > functions where we already have env->misa_mxl_max set properly.
>
> We want to assign gdb_core_xml_file *earlier* so assigning it after cpu_init() 
> is not OK. In general it should be considered unsafe to initialize a class 
> variable after class_init(); otherwise other subsystems cannot know when it 
> becomes available.

There's no difference in assigning it during .instance_init (cpu_init()) and
.instance_post_init. The callbacks are called one after the other during
init time. See qom/object.c,  object_initialize_with_type().

Now, if we want to be strict with only initializing class variables during 
class_init(),
which is something that I will defer to the maintainers, we want to minimize 
the use
of env->misa_mxl_max in the code before converting it to a class variable. 
env->misa_mxl
is always equal to env->misa_mxl_max, and they are being used interchangeably, 
but if
misa_mxl_max is going to be a class variable then we want to use env->misa_mxl 
instead
to avoid the RISCV_CPU_GET_CLASS() overhead.

I suggest a pre-patch to change the uses of env->misa_mxl_max to env->misa_mxl 
in
riscv_is_32bit(), riscv_cpu_gdb_read_register(), riscv_cpu_gdb_write_register() 
and
so on (there are quite a few places). And then apply this patch on top of it. 
This would
at least minimize the amount of casts being done.

All this considered, there's still one extra class cast that we will have to 
deal with
in riscv_tr_init_disas_context(). I am not sure how hot this function is but, 
taking a
look at other .init_disas_context implementations from other archs and not 
finding class
casts in them, I'm worried about the overhead it'll add. It seems like we can 
just do
"ctx->misa_mxl_max = env->misa_mxl" to avoid it.


Thanks,


Daniel