Message ID | 20220119051824.17494-22-zhiwei_liu@c-sky.com |
---|---|
State | New |
Headers | show |
Series | Support UXL filed in xstatus | expand |
On Wed, Jan 19, 2022 at 3:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > --- > target/riscv/csr.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index b11d92b51b..90f78eca65 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -572,6 +572,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > { > uint64_t mstatus = env->mstatus; > uint64_t mask = 0; > + RISCVMXL xl = riscv_cpu_mxl(env); > > /* flush tlb on mstatus fields that affect VM */ > if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | > @@ -583,21 +584,22 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR | > MSTATUS_TW | MSTATUS_VS; > > - if (riscv_cpu_mxl(env) != MXL_RV32) { > + if (xl != MXL_RV32) { > /* > * RV32: MPV and GVA are not in mstatus. The current plan is to > * add them to mstatush. For now, we just don't support it. > */ > mask |= MSTATUS_MPV | MSTATUS_GVA; > + if ((val & MSTATUS64_UXL) != 0) { > + mask |= MSTATUS64_UXL; > + } > } > > mstatus = (mstatus & ~mask) | (val & mask); > > - RISCVMXL xl = riscv_cpu_mxl(env); > if (xl > MXL_RV32) { > - /* SXL and UXL fields are for now read only */ > + /* SXL field is for now read only */ > mstatus = set_field(mstatus, MSTATUS64_SXL, xl); > - mstatus = set_field(mstatus, MSTATUS64_UXL, xl); This change causes: ERROR:../target/riscv/translate.c:295:get_gpr: code should not be reached to assert when running an Xvisor (Hypervisor extension) guest on the 64-bit virt machine. Alistair
On 2022/1/20 上午8:35, Alistair Francis wrote: > On Wed, Jan 19, 2022 at 3:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: >> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >> --- >> target/riscv/csr.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index b11d92b51b..90f78eca65 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -572,6 +572,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, >> { >> uint64_t mstatus = env->mstatus; >> uint64_t mask = 0; >> + RISCVMXL xl = riscv_cpu_mxl(env); >> >> /* flush tlb on mstatus fields that affect VM */ >> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | >> @@ -583,21 +584,22 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, >> MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR | >> MSTATUS_TW | MSTATUS_VS; >> >> - if (riscv_cpu_mxl(env) != MXL_RV32) { >> + if (xl != MXL_RV32) { >> /* >> * RV32: MPV and GVA are not in mstatus. The current plan is to >> * add them to mstatush. For now, we just don't support it. >> */ >> mask |= MSTATUS_MPV | MSTATUS_GVA; >> + if ((val & MSTATUS64_UXL) != 0) { >> + mask |= MSTATUS64_UXL; >> + } >> } >> >> mstatus = (mstatus & ~mask) | (val & mask); >> >> - RISCVMXL xl = riscv_cpu_mxl(env); >> if (xl > MXL_RV32) { >> - /* SXL and UXL fields are for now read only */ >> + /* SXL field is for now read only */ >> mstatus = set_field(mstatus, MSTATUS64_SXL, xl); >> - mstatus = set_field(mstatus, MSTATUS64_UXL, xl); > This change causes: > > ERROR:../target/riscv/translate.c:295:get_gpr: code should not be reached > > to assert when running an Xvisor (Hypervisor extension) guest on the > 64-bit virt machine. Hi Alistair, I am almost sure that there is an UXL field write error in Xvisor. I guess there is an write_sstatus instruction that writes a 0 to SSTATUS64_UXL. We can fix it on Xvisor. But before that, we should also give more strict constraints on SSTATUS64_UXL write. + if ((val & SSTATUS64_UXL) != 0) { + mask |= SSTATUS64_UXL; + } - mask |= SSTATUS64_UXL; I will send v8 patch set later for you to test later. Thanks, Zhiwei > Alistair
On 2022/1/20 上午8:35, Alistair Francis wrote: > On Wed, Jan 19, 2022 at 3:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: >> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >> --- >> target/riscv/csr.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index b11d92b51b..90f78eca65 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -572,6 +572,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, >> { >> uint64_t mstatus = env->mstatus; >> uint64_t mask = 0; >> + RISCVMXL xl = riscv_cpu_mxl(env); >> >> /* flush tlb on mstatus fields that affect VM */ >> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | >> @@ -583,21 +584,22 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, >> MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR | >> MSTATUS_TW | MSTATUS_VS; >> >> - if (riscv_cpu_mxl(env) != MXL_RV32) { >> + if (xl != MXL_RV32) { >> /* >> * RV32: MPV and GVA are not in mstatus. The current plan is to >> * add them to mstatush. For now, we just don't support it. >> */ >> mask |= MSTATUS_MPV | MSTATUS_GVA; >> + if ((val & MSTATUS64_UXL) != 0) { >> + mask |= MSTATUS64_UXL; >> + } >> } >> >> mstatus = (mstatus & ~mask) | (val & mask); >> >> - RISCVMXL xl = riscv_cpu_mxl(env); >> if (xl > MXL_RV32) { >> - /* SXL and UXL fields are for now read only */ >> + /* SXL field is for now read only */ >> mstatus = set_field(mstatus, MSTATUS64_SXL, xl); >> - mstatus = set_field(mstatus, MSTATUS64_UXL, xl); > This change causes: > > ERROR:../target/riscv/translate.c:295:get_gpr: code should not be reached > > to assert when running an Xvisor (Hypervisor extension) guest on the > 64-bit virt machine. Hi Alistair, Thanks for pointing it out. I will have a test on Xvisor. Thanks, Zhiwei > > Alistair
On Thu, Jan 20, 2022 at 12:12 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > > On 2022/1/20 上午8:35, Alistair Francis wrote: > > On Wed, Jan 19, 2022 at 3:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > >> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> > >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > >> --- > >> target/riscv/csr.c | 17 ++++++++++++----- > >> 1 file changed, 12 insertions(+), 5 deletions(-) > >> > >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >> index b11d92b51b..90f78eca65 100644 > >> --- a/target/riscv/csr.c > >> +++ b/target/riscv/csr.c > >> @@ -572,6 +572,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > >> { > >> uint64_t mstatus = env->mstatus; > >> uint64_t mask = 0; > >> + RISCVMXL xl = riscv_cpu_mxl(env); > >> > >> /* flush tlb on mstatus fields that affect VM */ > >> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | > >> @@ -583,21 +584,22 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > >> MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR | > >> MSTATUS_TW | MSTATUS_VS; > >> > >> - if (riscv_cpu_mxl(env) != MXL_RV32) { > >> + if (xl != MXL_RV32) { > >> /* > >> * RV32: MPV and GVA are not in mstatus. The current plan is to > >> * add them to mstatush. For now, we just don't support it. > >> */ > >> mask |= MSTATUS_MPV | MSTATUS_GVA; > >> + if ((val & MSTATUS64_UXL) != 0) { > >> + mask |= MSTATUS64_UXL; > >> + } > >> } > >> > >> mstatus = (mstatus & ~mask) | (val & mask); > >> > >> - RISCVMXL xl = riscv_cpu_mxl(env); > >> if (xl > MXL_RV32) { > >> - /* SXL and UXL fields are for now read only */ > >> + /* SXL field is for now read only */ > >> mstatus = set_field(mstatus, MSTATUS64_SXL, xl); > >> - mstatus = set_field(mstatus, MSTATUS64_UXL, xl); > > This change causes: > > > > ERROR:../target/riscv/translate.c:295:get_gpr: code should not be reached > > > > to assert when running an Xvisor (Hypervisor extension) guest on the > > 64-bit virt machine. > > Hi Alistair, > > I am almost sure that there is an UXL field write error in Xvisor. You are probably right, but a guest bug like that shouldn't be able to crash QEMU > > I guess there is an write_sstatus instruction that writes a 0 to > SSTATUS64_UXL. > > We can fix it on Xvisor. But before that, we should also give more > strict constraints on SSTATUS64_UXL write. > > + if ((val & SSTATUS64_UXL) != 0) { > + mask |= SSTATUS64_UXL; > + } > - mask |= SSTATUS64_UXL; > > > I will send v8 patch set later for you to test later. Thanks! Alistair > > > Thanks, > Zhiwei > > > Alistair
Hi Alistair, Do you mind share you test method? I follow the xvisor document on https://github.com/xvisor/xvisor/blob/v0.3.1/docs/riscv/riscv64-qemu.txt. But it can't run even on QEMU master branch. It blocks on OpenSBI. liuzw@b12e0231:/mnt/ssd/liuzw/git/xvisor$ qemu-system-riscv64 -cpu rv64,h=true -M virt -m 512M -nographic -bios ../opensbi/build/platform/generic/firmware/fw_jump.bin -kernel ./build/vmm.bin -initrd ./build/disk.img -append 'vmm.bootcmd="vfs mount initrd /;vfs run /boot.xscript;vfs cat /system/banner.txt"' OpenSBI v1.0-2-g6dde435 ____ _____ ____ _____ / __ \ / ____| _ \_ _| | | | |_ __ ___ _ __ | (___ | |_) || | | | | | '_ \ / _ \ '_ \ \___ \| _ < | | | |__| | |_) | __/ | | |____) | |_) || |_ \____/| .__/ \___|_| |_|_____/|____/_____| | | |_| Platform Name : riscv-virtio,qemu Platform Features : medeleg Platform HART Count : 1 Platform IPI Device : aclint-mswi Platform Timer Device : aclint-mtimer @ 10000000Hz Platform Console Device : uart8250 Platform HSM Device : --- Platform Reboot Device : sifive_test Platform Shutdown Device : sifive_test Firmware Base : 0x80000000 Firmware Size : 252 KB Runtime SBI Version : 0.3 Domain0 Name : root Domain0 Boot HART : 0 Domain0 HARTs : 0* Domain0 Region00 : 0x0000000002000000-0x000000000200ffff (I) Domain0 Region01 : 0x0000000080000000-0x000000008003ffff () Domain0 Region02 : 0x0000000000000000-0xffffffffffffffff (R,W,X) Domain0 Next Address : 0x0000000080200000 Domain0 Next Arg1 : 0x0000000082200000 Domain0 Next Mode : S-mode Domain0 SysReset : yes Boot HART ID : 0 Boot HART Domain : root Boot HART ISA : rv64imafdcsuh Boot HART Features : scounteren,mcounteren,time Boot HART PMP Count : 16 Boot HART PMP Granularity : 4 Boot HART PMP Address Bits: 54 Boot HART MHPM Count : 0 Boot HART MIDELEG : 0x0000000000000666 Boot HART MEDELEG : 0x0000000000f0b509 QEMU: Terminated Thanks, Zhiwei On 2022/1/20 上午11:29, Alistair Francis wrote: > On Thu, Jan 20, 2022 at 12:12 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: >> >> On 2022/1/20 上午8:35, Alistair Francis wrote: >>> On Wed, Jan 19, 2022 at 3:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: >>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> >>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>> --- >>>> target/riscv/csr.c | 17 ++++++++++++----- >>>> 1 file changed, 12 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>> index b11d92b51b..90f78eca65 100644 >>>> --- a/target/riscv/csr.c >>>> +++ b/target/riscv/csr.c >>>> @@ -572,6 +572,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, >>>> { >>>> uint64_t mstatus = env->mstatus; >>>> uint64_t mask = 0; >>>> + RISCVMXL xl = riscv_cpu_mxl(env); >>>> >>>> /* flush tlb on mstatus fields that affect VM */ >>>> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | >>>> @@ -583,21 +584,22 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, >>>> MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR | >>>> MSTATUS_TW | MSTATUS_VS; >>>> >>>> - if (riscv_cpu_mxl(env) != MXL_RV32) { >>>> + if (xl != MXL_RV32) { >>>> /* >>>> * RV32: MPV and GVA are not in mstatus. The current plan is to >>>> * add them to mstatush. For now, we just don't support it. >>>> */ >>>> mask |= MSTATUS_MPV | MSTATUS_GVA; >>>> + if ((val & MSTATUS64_UXL) != 0) { >>>> + mask |= MSTATUS64_UXL; >>>> + } >>>> } >>>> >>>> mstatus = (mstatus & ~mask) | (val & mask); >>>> >>>> - RISCVMXL xl = riscv_cpu_mxl(env); >>>> if (xl > MXL_RV32) { >>>> - /* SXL and UXL fields are for now read only */ >>>> + /* SXL field is for now read only */ >>>> mstatus = set_field(mstatus, MSTATUS64_SXL, xl); >>>> - mstatus = set_field(mstatus, MSTATUS64_UXL, xl); >>> This change causes: >>> >>> ERROR:../target/riscv/translate.c:295:get_gpr: code should not be reached >>> >>> to assert when running an Xvisor (Hypervisor extension) guest on the >>> 64-bit virt machine. >> Hi Alistair, >> >> I am almost sure that there is an UXL field write error in Xvisor. > You are probably right, but a guest bug like that shouldn't be able to > crash QEMU > >> I guess there is an write_sstatus instruction that writes a 0 to >> SSTATUS64_UXL. >> >> We can fix it on Xvisor. But before that, we should also give more >> strict constraints on SSTATUS64_UXL write. >> >> + if ((val & SSTATUS64_UXL) != 0) { >> + mask |= SSTATUS64_UXL; >> + } >> - mask |= SSTATUS64_UXL; >> >> >> I will send v8 patch set later for you to test later. > Thanks! > > Alistair > >> >> Thanks, >> Zhiwei >> >>> Alistair
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index b11d92b51b..90f78eca65 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -572,6 +572,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, { uint64_t mstatus = env->mstatus; uint64_t mask = 0; + RISCVMXL xl = riscv_cpu_mxl(env); /* flush tlb on mstatus fields that affect VM */ if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | @@ -583,21 +584,22 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR | MSTATUS_TW | MSTATUS_VS; - if (riscv_cpu_mxl(env) != MXL_RV32) { + if (xl != MXL_RV32) { /* * RV32: MPV and GVA are not in mstatus. The current plan is to * add them to mstatush. For now, we just don't support it. */ mask |= MSTATUS_MPV | MSTATUS_GVA; + if ((val & MSTATUS64_UXL) != 0) { + mask |= MSTATUS64_UXL; + } } mstatus = (mstatus & ~mask) | (val & mask); - RISCVMXL xl = riscv_cpu_mxl(env); if (xl > MXL_RV32) { - /* SXL and UXL fields are for now read only */ + /* SXL field is for now read only */ mstatus = set_field(mstatus, MSTATUS64_SXL, xl); - mstatus = set_field(mstatus, MSTATUS64_UXL, xl); } env->mstatus = mstatus; env->xl = cpu_recompute_xl(env); @@ -907,7 +909,9 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno, target_ulong *val) { target_ulong mask = (sstatus_v1_10_mask); - + if (env->xl != MXL_RV32) { + mask |= SSTATUS64_UXL; + } /* TODO: Use SXL not MXL. */ *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); return RISCV_EXCP_NONE; @@ -917,6 +921,9 @@ static RISCVException write_sstatus(CPURISCVState *env, int csrno, target_ulong val) { target_ulong mask = (sstatus_v1_10_mask); + if (env->xl != MXL_RV32) { + mask |= SSTATUS64_UXL; + } target_ulong newval = (env->mstatus & ~mask) | (val & mask); return write_mstatus(env, CSR_MSTATUS, newval); }