RE: [PATCH V2 1/5] target/riscv: Add basic vmstate description of CPU

[Jiangyifei]

> -----Original Message-----
> From: Richard Henderson [mailto:richard.henderson@linaro.org]
> Sent: Saturday, October 10, 2020 9:23 PM
> To: Jiangyifei <jiangyifei@huawei.com>; qemu-devel@nongnu.org;
> qemu-riscv@nongnu.org
> Cc: palmer@dabbelt.com; Alistair.Francis@wdc.com;
> sagark@eecs.berkeley.edu; kbastian@mail.uni-paderborn.de; Zhangxiaofeng
> (F) <victor.zhangxiaofeng@huawei.com>; Wubin (H) <wu.wubin@huawei.com>;
> Zhanghailiang <zhang.zhanghailiang@huawei.com>; dengkai (A)
> <dengkai1@huawei.com>; yinyipeng <yinyipeng1@huawei.com>
> Subject: Re: [PATCH V2 1/5] target/riscv: Add basic vmstate description of CPU
> 
> On 10/10/20 3:06 AM, Yifei Jiang wrote:
> > +++ b/target/riscv/cpu.h
> > @@ -311,6 +311,10 @@ extern const char * const riscv_fpr_regnames[];
> > extern const char * const riscv_excp_names[];  extern const char *
> > const riscv_intr_names[];
> >
> > +#ifndef CONFIG_USER_ONLY
> > +extern const VMStateDescription vmstate_riscv_cpu; #endif
> > +
> 
> This is not part of the public interface to RISCVCPU, so it should go in
> internals.h.
> 
> Not that there aren't other things in cpu.h that don't belong.  No target has
> been perfect in differentiating what's interface and what's implementation.
> 

Yes, I think it should go in internals.h, although most architectures declare 
it in cpu.h.
I would move it to internals.h in the next series.

> > +
> > +#ifdef TARGET_RISCV32
> > +        VMSTATE_UINTTL(env.mstatush, RISCVCPU), #endif
> 
> Would this be a good time to expand mstatus to uint64_t instead of
> target_ulong so that it can be saved as one unit and reduce some ifdefs in the
> code base?
> 
> Similarly with some of the other status registers that are two halved for
> riscv32.

I agree with you that it should be rearranged.
But I hope this series will focus on achieving migration.
Can I send another patch to rearrange it later?

Yifei
> 
> 
> r~