On Tue, May 20, 2025 at 01:58:45PM +0200, Andrew Jones wrote: > On Tue, May 20, 2025 at 06:41:03PM +0800, Meng Zhuo wrote: > > This patch adds host satp mode while kvm/host cpu satp mode is not > > set. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2931 > > Signed-off-by: Meng Zhuo <mengz...@iscas.ac.cn> > > --- > > Please provide a changelog here. I had to go reread the comments on v1 to > guess at what might have changed. > > > target/riscv/cpu.c | 3 +-- > > target/riscv/cpu.h | 1 + > > target/riscv/kvm/kvm-cpu.c | 20 +++++++++++++++++++- > > 3 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index d92874baa0..a84edd3a3b 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -433,8 +433,7 @@ const char *satp_mode_str(uint8_t satp_mode, bool > > is_32_bit) > > g_assert_not_reached(); > > } > > > > -static void set_satp_mode_max_supported(RISCVCPU *cpu, > > - uint8_t satp_mode) > > +void set_satp_mode_max_supported(RISCVCPU *cpu, uint8_t satp_mode) > > In v1 I suggested we add some sanity checking to this function to ensure > we can never set an invalid mode (one that's higher than the max > supported).
It appears [1], which I just learned exists (that series doesn't CC qemu-riscv) adds an assert for the check (among other changes). [1] https://lore.kernel.org/all/20250406070254.274797-3-pbonz...@redhat.com/ Thanks, drew > > > { > > bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > > const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64; > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > index b56d3afa69..d7136f1d72 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -915,6 +915,7 @@ char *riscv_cpu_get_name(RISCVCPU *cpu); > > > > void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp); > > void riscv_add_satp_mode_properties(Object *obj); > > +void set_satp_mode_max_supported(RISCVCPU *cpu, uint8_t satp_mode); > > bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu); > > > > /* CSR function table */ > > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c > > index 82f9728636..18fbca1a08 100644 > > --- a/target/riscv/kvm/kvm-cpu.c > > +++ b/target/riscv/kvm/kvm-cpu.c > > @@ -999,6 +999,23 @@ static void > > kvm_riscv_destroy_scratch_vcpu(KVMScratchCPU *scratch) > > close(scratch->kvmfd); > > } > > > > +static void kvm_riscv_init_satp_mode(RISCVCPU *cpu, KVMScratchCPU *kvmcpu) > > +{ > > + CPURISCVState *env = &cpu->env; > > + struct kvm_one_reg reg; > > + int ret; > > + uint64_t val; > > + > > + reg.id = RISCV_CONFIG_REG(env, satp_mode); > > + reg.addr = (uint64_t)&val; > > + ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, ®); > > + if (ret != 0) { > > + error_report("Unable to retrieve satp from host, error %d", ret); > > This will output "Unable..., error -1" on error. We should output at least > the errno and preferably the errno string. I see that this is just a copy > of a pattern used throughout this file though, so we need a patch fixing > all of the ioctl call error_reports. > > > + } > > + > > + set_satp_mode_max_supported(cpu, val); > > +} > > + > > static void kvm_riscv_init_machine_ids(RISCVCPU *cpu, KVMScratchCPU > > *kvmcpu) > > { > > struct kvm_one_reg reg; > > @@ -1302,6 +1319,7 @@ static void riscv_init_kvm_registers(Object *cpu_obj) > > kvm_riscv_init_machine_ids(cpu, &kvmcpu); > > kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu); > > kvm_riscv_init_cfg(cpu, &kvmcpu); > > + kvm_riscv_init_satp_mode(cpu, &kvmcpu); > > > > kvm_riscv_destroy_scratch_vcpu(&kvmcpu); > > } > > @@ -1980,7 +1998,7 @@ static bool kvm_cpu_realize(CPUState *cs, Error > > **errp) > > } > > } > > > > - return true; > > + return true; > > } > > > > void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp) > > -- > > 2.39.5 > > > > > > Otherwise, > > Reviewed-by: Andrew Jones <ajo...@ventanamicro.com>
