Re: [Qemu-devel] [PATCH v2 05/21] RISC-V CPU Helpers

[Christoph Hellwig]

#ifdef CONFIG_USER_ONLY
int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
{
    return 0;
}

bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
{
    return false;
}

int riscv_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
        int access_type, int mmu_idx)
{
    cs->exception_index = QEMU_USER_EXCP_FAULT;
    return TRANSLATE_FAIL;
}

void riscv_cpu_do_interrupt(CPUState *cs)
{
    cs->exception_index = EXCP_NONE; /* mark handled to qemu */
}
#else
... real implementation here ...
#endif

To keep the code flow straight?

> +int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return 0;
> +#else

Can you do a large section of USER ONLY stubs instead of sprinkling
the ifdefs?  E.g.

> +static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> +                                int *prot, target_ulong address,
> +                                int access_type, int mmu_idx)
> +{
> +    /* NOTE: the env->pc value visible here will not be
> +     * correct, but the value visible to the exception handler
> +     * (riscv_cpu_do_interrupt) is correct */
> +
> +    const int mode = mmu_idx;

Why don't you just name the actual argument mode and mark it const
if really needed?

> +
> +    *prot = 0;
> +    CPUState *cs = CPU(riscv_env_get_cpu(env));

Clearing out prot just before declaring cs looks a little odd.  Especially
the prot clearing can easily be moved past the next branch.

> +    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 {
> +        g_assert_not_reached();

Why isn't this a switch statement?

> +    if (access_type == MMU_INST_FETCH) {
> +        cs->exception_index = RISCV_EXCP_INST_ADDR_MIS;
> +        env->badaddr = addr;
> +    } else if (access_type == MMU_DATA_STORE) {
> +        cs->exception_index = RISCV_EXCP_STORE_AMO_ADDR_MIS;
> +        env->badaddr = addr;
> +    } else if (access_type == MMU_DATA_LOAD) {
> +        cs->exception_index = RISCV_EXCP_LOAD_ADDR_MIS;
> +        env->badaddr = addr;
> +    } else {
> +        g_assert_not_reached();
> +    }

Same here.

> +    case CSR_SPTBR:

And just as comment on V2:  please don't use CSR_SPTBR here, but
CSR_SAPT, and don't even define the CSR_SPTBR cpp symbol.