On Wed, Jan 3, 2018 at 8:12 PM, Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 01/02/2018 04:44 PM, Michael Clark wrote:
> > + target_ulong mode = env->priv;
> > + if (access_type != MMU_INST_FETCH) {
> > + if (get_field(env->mstatus, MSTATUS_MPRV)) {
> > + mode = get_field(env->mstatus, MSTATUS_MPP);
> > + }
> > + }
> > + if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> > + if (get_field(env->satp, SATP_MODE) == VM_1_09_MBARE) {
> > + mode = PRV_M;
> > + }
> > + } else {
> > + if (get_field(env->mstatus, MSTATUS_VM) == VM_1_10_MBARE) {
> > + mode = PRV_M;
> > + }
> > + }
>
> This is replicating cpu_mmu_index.
> Therefore you should be relying on mmu_idx.
>
> > + /* check to make sure that mmu_idx and mode that we get matches */
> > + if (unlikely(mode != mmu_idx)) {
> > + fprintf(stderr, "MODE: mmu_idx mismatch\n");
> > + exit(1);
> > + }
>
> As in the opposite of this.
OK. cpu_mmu_index has already translated the mode into mmu_idx for us so we
can eliminate the redundant mode fetch, check and error message.
Essentially we should trust mmu_idx returned from cpu_mmu_index, so this
statement should never trigger.
Will include in the next spin.
> > +
> > + if (mode == PRV_M) {
> > + target_ulong msb_mask = /*0x7FFFFFFFFFFFFFFF; */
> > + (((target_ulong)2) << (TARGET_LONG_BITS - 1)) - 1;
> > + *physical = address & msb_mask;
>
> Or perhaps extract64(address, 0, TARGET_LONG_BITS - 1)?
>
> > + if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> > + base = get_field(env->satp, SATP_PPN) << PGSHIFT;
> > + sum = get_field(env->mstatus, MSTATUS_SUM);
> > + vm = get_field(env->satp, SATP_MODE);
> > + switch (vm) {
> > + case VM_1_10_SV32:
> > + levels = 2; ptidxbits = 10; ptesize = 4; break;
> > + case VM_1_10_SV39:
> > + levels = 3; ptidxbits = 9; ptesize = 8; break;
> > + case VM_1_10_SV48:
> > + levels = 4; ptidxbits = 9; ptesize = 8; break;
> > + case VM_1_10_SV57:
> > + levels = 5; ptidxbits = 9; ptesize = 8; break;
> > + default:
> > + printf("unsupported SATP_MODE value\n");
> > + exit(1);
>
> Just qemu_log_mask with LOG_UNIMP or LOG_GUEST_ERROR, and then return
> TRANSLATE_FAIL. Printing to stdout and exiting isn't kosher. Lots more
> occurrences within this file.
Understand. I had aleady converted several printfs to error_report.
I wasn't sure which logging API to use. There are also quite a lot of uses
of grep -r error_report target.
I'll grep -r for printf and change to qemu_log_mask with appropriate level.
I see exit(1) called in quite a few of the other ports too. I was wondering
at the time if there is a canonical error_abort API?
Will try to improve things in the next spin.
> +static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
> > + MMUAccessType access_type)
> > +{
> > + CPUState *cs = CPU(riscv_env_get_cpu(env));
> > + int page_fault_exceptions =
> > + (env->priv_ver >= PRIV_VERSION_1_10_0) &&
> > + get_field(env->satp, SATP_MODE) != VM_1_10_MBARE;
> > + int exception = 0;
> > + if (access_type == MMU_INST_FETCH) { /* inst access */
> > + exception = page_fault_exceptions ?
> > + RISCV_EXCP_INST_PAGE_FAULT : RISCV_EXCP_INST_ACCESS_FAULT;
> > + env->badaddr = address;
> > + } else if (access_type == MMU_DATA_STORE) { /* store access */
> > + exception = page_fault_exceptions ?
> > + RISCV_EXCP_STORE_PAGE_FAULT : RISCV_EXCP_STORE_AMO_ACCESS_
> FAULT;
> > + env->badaddr = address;
> > + } else if (access_type == MMU_DATA_LOAD) { /* load access */
> > + exception = page_fault_exceptions ?
> > + RISCV_EXCP_LOAD_PAGE_FAULT : RISCV_EXCP_LOAD_ACCESS_FAULT;
> > + env->badaddr = address;
> > + } else {
> > + fprintf(stderr, "FAIL: invalid access_type\n");
> > + exit(1);
>
> Switch with a default: g_assert_not_reached(), since access_type is not
> controlled by the guest.
>
> > +void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> > + MMUAccessType access_type, int
> mmu_idx,
> > + uintptr_t retaddr)
> > +{
> > + RISCVCPU *cpu = RISCV_CPU(cs);
> > + CPURISCVState *env = &cpu->env;
> > + if (access_type == MMU_INST_FETCH) {
> > + fprintf(stderr, "unaligned inst fetch not handled here. should
> not "
> > + "trigger\n");
> > + exit(1);
>
> No exit. Do something logical.
Got it. Assertion.
> > + } else if (access_type == MMU_DATA_STORE) {
> > + cs->exception_index = RISCV_EXCP_STORE_AMO_ADDR_MIS;
> > + env->badaddr = addr;
>
> Why does STORE imply AMO? Why can't a normal store trigger an unaligned
> trap?
It's STORE or AMO. Here are the exception causes from the RISC-V Privileged
ISA Specification:
# Machine Cause Register faults (mcause), interrupt bit clear
cause misaligned_fetch 0 "Instruction address misaligned"
cause fault_fetch 1 "Instruction access fault"
cause illegal_instruction 2 "Illegal instruction"
cause breakpoint 3 "Breakpoint"
cause misaligned_load 4 "Load address misaligned"
cause fault_load 5 "Load access fault"
cause misaligned_store 6 "Store/AMO address misaligned"
cause fault_store 7 "Store/AMO access fault"
cause user_ecall 8 "Environment call from U-mode"
cause supervisor_ecall 9 "Environment call from S-mode"
cause hypervisor_ecall 10 "Environment call from H-mode"
cause machine_ecall 11 "Environment call from M-mode"
cause exec_page_fault 12 "Instruction page fault"
cause load_page_fault 13 "Load page fault"
cause store_page_fault 15 "Store/AMO page fault"
> > + fprintf(stderr, "Invalid MMUAccessType\n");
> > + exit(1);
>
> I'll stop pointing these out, but there need to be zero instances of exit
> within the backend.
I totally agree. Inherited. However it seems there are lots of calls to
exit(1) in many other targets. Not that that is an excuse.
> > +void riscv_cpu_do_interrupt(CPUState *cs)
> > +{
> > +#if !defined(CONFIG_USER_ONLY)
> > +
> > + RISCVCPU *cpu = RISCV_CPU(cs);
> > + CPURISCVState *env = &cpu->env;
> > +
> > + #ifdef RISCV_DEBUG_INTERRUPT
> > + if (cs->exception_index & 0x70000000) {
> > + fprintf(stderr, "core 0: exception trap_%s, epc 0x"
> TARGET_FMT_lx "\n"
> > + , riscv_interrupt_names[cs->exception_index &
> 0x0fffffff],
> > + env->pc);
> > + } else {
> > + fprintf(stderr, "core 0: exception trap_%s, epc 0x"
> TARGET_FMT_lx "\n"
> > + , riscv_excp_names[cs->exception_index], env->pc);
> > + }
> > + #endif
> > +
> > + if (cs->exception_index == RISCV_EXCP_BREAKPOINT) {
> > + fprintf(stderr, "debug mode not implemented\n");
> > + }
> > +
> > + /* skip dcsr cause check */
> > +
> > + target_ulong fixed_cause = 0;
> > + if (cs->exception_index & (0x70000000)) {
> > + /* hacky for now. the MSB (bit 63) indicates interrupt but
> cs->exception
> > + index is only 32 bits wide */
> > + fixed_cause = cs->exception_index & 0x0FFFFFFF;
> > + fixed_cause |= ((target_ulong)1) << (TARGET_LONG_BITS - 1);
> > + } else {
> > + /* fixup User ECALL -> correct priv ECALL */
> > + if (cs->exception_index == RISCV_EXCP_U_ECALL) {
> > + switch (env->priv) {
> > + case PRV_U:
> > + fixed_cause = RISCV_EXCP_U_ECALL;
> > + break;
> > + case PRV_S:
> > + fixed_cause = RISCV_EXCP_S_ECALL;
> > + break;
> > + case PRV_H:
> > + fixed_cause = RISCV_EXCP_H_ECALL;
> > + break;
> > + case PRV_M:
> > + fixed_cause = RISCV_EXCP_M_ECALL;
> > + break;
> > + }
> > + } else {
> > + fixed_cause = cs->exception_index;
> > + }
> > + }
> > +
> > + target_ulong backup_epc = env->pc;
> > +
> > + target_ulong bit = fixed_cause;
> > + target_ulong deleg = env->medeleg;
> > +
> > + int hasbadaddr =
> > + (fixed_cause == RISCV_EXCP_INST_ADDR_MIS) ||
> > + (fixed_cause == RISCV_EXCP_INST_ACCESS_FAULT) ||
> > + (fixed_cause == RISCV_EXCP_LOAD_ADDR_MIS) ||
> > + (fixed_cause == RISCV_EXCP_STORE_AMO_ADDR_MIS) ||
> > + (fixed_cause == RISCV_EXCP_LOAD_ACCESS_FAULT) ||
> > + (fixed_cause == RISCV_EXCP_STORE_AMO_ACCESS_FAULT) ||
> > + (fixed_cause == RISCV_EXCP_INST_PAGE_FAULT) ||
> > + (fixed_cause == RISCV_EXCP_LOAD_PAGE_FAULT) ||
> > + (fixed_cause == RISCV_EXCP_STORE_PAGE_FAULT);
> > +
> > + if (bit & ((target_ulong)1 << (TARGET_LONG_BITS - 1))) {
> > + deleg = env->mideleg;
> > + bit &= ~((target_ulong)1 << (TARGET_LONG_BITS - 1));
> > + }
> > +
> > + if (env->priv <= PRV_S && bit < 64 && ((deleg >> bit) & 1)) {
> > + /* handle the trap in S-mode */
> > + /* No need to check STVEC for misaligned - lower 2 bits cannot
> be set */
> > + env->pc = env->stvec;
> > + env->scause = fixed_cause;
> > + env->sepc = backup_epc;
> > +
> > + if (hasbadaddr) {
> > + #ifdef RISCV_DEBUG_INTERRUPT
> > + fprintf(stderr, "core %d: badaddr 0x" TARGET_FMT_lx "\n",
> > + env->mhartid, env->badaddr);
> > + #endif
> > + env->sbadaddr = env->badaddr;
> > + }
> > +
> > + target_ulong s = env->mstatus;
> > + s = set_field(s, MSTATUS_SPIE, get_field(s, MSTATUS_UIE <<
> env->priv));
> > + s = set_field(s, MSTATUS_SPP, env->priv);
> > + s = set_field(s, MSTATUS_SIE, 0);
> > + csr_write_helper(env, s, CSR_MSTATUS);
> > + set_privilege(env, PRV_S);
> > + } else {
> > + /* No need to check MTVEC for misaligned - lower 2 bits cannot
> be set */
> > + env->pc = env->mtvec;
> > + env->mepc = backup_epc;
> > + env->mcause = fixed_cause;
> > +
> > + if (hasbadaddr) {
> > + #ifdef RISCV_DEBUG_INTERRUPT
> > + fprintf(stderr, "core %d: badaddr 0x" TARGET_FMT_lx "\n",
> > + env->mhartid, env->badaddr);
> > + #endif
> > + env->mbadaddr = env->badaddr;
> > + }
> > +
> > + target_ulong s = env->mstatus;
> > + s = set_field(s, MSTATUS_MPIE, get_field(s, MSTATUS_UIE <<
> env->priv));
> > + s = set_field(s, MSTATUS_MPP, env->priv);
> > + s = set_field(s, MSTATUS_MIE, 0);
> > + csr_write_helper(env, s, CSR_MSTATUS);
> > + set_privilege(env, PRV_M);
> > + }
> > + /* TODO yield load reservation */
> > +#endif
> > + cs->exception_index = EXCP_NONE; /* mark handled to qemu */
> > +}
>
> Marking handled is done generically. Why do you need to do it here?
Nothing i guess. I'll remove and re-test...
> > +/* Floating Point - fused */
> > +DEF_HELPER_FLAGS_5(fmadd_s, TCG_CALL_NO_RWG, i64, env, i64, i64, i64,
> i64)
>
> Ideally these would go in with the patch that adds the helpers, so they're
> easier to validate. However, I suppose it doesn't really matter.
>
> > +void helper_raise_exception_mbadaddr(CPURISCVState *env, uint32_t
> exception,
> > + target_ulong bad_pc) {
>
> Brace on next line.
>
> > + #ifdef RISCV_DEBUG_PRINT
> > + fprintf(stderr, "Write CSR reg: 0x" TARGET_FMT_lx "\n", csrno);
> > + fprintf(stderr, "Write CSR val: 0x" TARGET_FMT_lx "\n",
> val_to_write);
> > + #endif
>
> Drop the debugging prints. Perhaps use the tracing infrastructure?
Agree. That will take some time...
>
> > + case CSR_MISA: {
> > + if (!(val_to_write & (1L << ('F' - 'A')))) {
> > + val_to_write &= ~(1L << ('D' - 'A'));
> > + }
> > +
> > + /* allow MAFDC bits in MISA to be modified */
> > + target_ulong mask = 0;
> > + mask |= 1L << ('M' - 'A');
> > + mask |= 1L << ('A' - 'A');
> > + mask |= 1L << ('F' - 'A');
> > + mask |= 1L << ('D' - 'A');
> > + mask |= 1L << ('C' - 'A');
> > + mask &= env->misa_mask;
> > +
> > + env->misa = (val_to_write & mask) | (env->misa & ~mask);
>
> Does this not affect the set of instructions that are allowable? If so,
> you'd
> want something like
>
> new_misa = (val_to_write & mask) | (env->misa & ~mask);
> if (env->misa != new_misa) {
> env->misa = new_misa;
> tb_flush(CPU(riscv_env_get_cpu(env)));
> }
>
> so that we start with all new translations, which would then check the new
> value of misa, and would then raise INST_ADDR_MIS (or not).
>
> > +inline target_ulong csr_read_helper(CPURISCVState *env, target_ulong
> csrno)
>
> Why mark such large functions inline?
Inherited. I'd outlined some occurences.
It seems we need to do quite a bit of work on the cpu core.
> +void set_privilege(CPURISCVState *env, target_ulong newpriv)
> > +{
> > + if (!(newpriv <= PRV_M)) {
> > + printf("INVALID PRIV SET\n");
> > + exit(1);
> > + }
> > + if (newpriv == PRV_H) {
> > + newpriv = PRV_U;
> > + }
> > + helper_tlb_flush(env);
>
> Why flush? Doesn't this just switch to a different mmu_idx?
Sagar or Bastien might be able to answer that.
> > +void helper_fence_i(CPURISCVState *env)
> > +{
> > + RISCVCPU *cpu = riscv_env_get_cpu(env);
> > + CPUState *cs = CPU(cpu);
> > + /* Flush QEMU's TLB */
> > + tlb_flush(cs);
> > + /* ARM port seems to not know if this is okay inside a TB
> > + But we need to do it */
> > + tb_flush(cs);
> > +}
>
> You should not require either flush.
> This insn can be implemented in qemu as a nop.
>
So self modifying code with no advice is fine? I guess that is required to
support x86. Will include in the next spin.
This may take some time... When is code freeze for 2.12?
