Re: [Qemu-devel] [PATCH 02/18] target-riscv: Add RISC-V Target stubs inside target-riscv/

[Richard Henderson]

On 09/26/2016 03:56 AM, Sagar Karandikar wrote:
> +/* QEMU addressing/paging config */
> +#define TARGET_PAGE_BITS 12 /* 4 KiB Pages */
> +#if defined(TARGET_RISCV64)
> +#define TARGET_LONG_BITS 64 /* this defs TCGv as TCGv_i64 in tcg/tcg-op.h */
> +#define TARGET_PHYS_ADDR_SPACE_BITS 50
> +#define TARGET_VIRT_ADDR_SPACE_BITS 39
> +#elif defined(TARGET_RISCV32)
> +#define TARGET_LONG_BITS 32 /* this defs TCGv as TCGv_i64 in tcg/tcg-op.h */

Typo, and there's not really a need for that comment anyway.

> +#define PGSHIFT 12

Just use TARGET_PAGE_BITS, surely.

> +#define MSTATUS_UIE         0x00000001
> +#define MSTATUS_SIE         0x00000002
> +#define MSTATUS_HIE         0x00000004
> +#define MSTATUS_MIE         0x00000008
> +#define MSTATUS_UPIE        0x00000010
> +#define MSTATUS_SPIE        0x00000020
> +#define MSTATUS_HPIE        0x00000040
> +#define MSTATUS_MPIE        0x00000080
> +#define MSTATUS_SPP         0x00000100
> +#define MSTATUS_HPP         0x00000600
> +#define MSTATUS_MPP         0x00001800
> +#define MSTATUS_FS          0x00006000
> +#define MSTATUS_XS          0x00018000
> +#define MSTATUS_MPRV        0x00020000
> +#define MSTATUS_PUM         0x00040000
> +#define MSTATUS_MXR         0x00080000
> +#define MSTATUS_VM          0x1F000000
> +
> +#define MSTATUS32_SD        0x80000000
> +#define MSTATUS64_SD        0x8000000000000000
> +
> +#define SSTATUS_UIE         0x00000001
> +#define SSTATUS_SIE         0x00000002
> +#define SSTATUS_UPIE        0x00000010
> +#define SSTATUS_SPIE        0x00000020
> +#define SSTATUS_SPP         0x00000100
> +#define SSTATUS_FS          0x00006000
> +#define SSTATUS_XS          0x00018000
> +#define SSTATUS_PUM         0x00040000
> +#define SSTATUS32_SD        0x80000000
> +#define SSTATUS64_SD        0x8000000000000000

Given that the status bits for lesser privs are the same as MSTATUS, with some 
omissions, reproducing these is not ideal.  Perhaps better to define a 
SSTATUS_MASK that lists the MSTATUS bits that are controlable.

> +/*
> + * ctz in Spike returns 0 if val == 0, wrap helper
> + */
> +static int ctz(target_ulong val)
> +{
> +    return val ? ctz64(val) : 0;
> +}

Does this really belong here?  I don't see it used by op_helper, only ...

> +
> +/*
> + * Return RISC-V IRQ number if an interrupt should be taken, else -1.
> + * Used in cpu-exec.c
> + *
> + * Adapted from Spike's processor_t::take_interrupt()
> + */
> +static inline int cpu_riscv_hw_interrupts_pending(CPURISCVState *env)
> +{
> +    target_ulong pending_interrupts = env->csr[CSR_MIP] & env->csr[CSR_MIE];
> +
> +    target_ulong mie = get_field(env->csr[CSR_MSTATUS], MSTATUS_MIE);
> +    target_ulong m_enabled = env->priv < PRV_M || (env->priv == PRV_M && mie);
> +    target_ulong enabled_interrupts = pending_interrupts &
> +                                      ~env->csr[CSR_MIDELEG] & -m_enabled;
> +
> +    target_ulong sie = get_field(env->csr[CSR_MSTATUS], MSTATUS_SIE);
> +    target_ulong s_enabled = env->priv < PRV_S || (env->priv == PRV_S && sie);
> +    enabled_interrupts |= pending_interrupts & env->csr[CSR_MIDELEG] &
> +                          -s_enabled;
> +
> +    if (enabled_interrupts) {
> +        target_ulong counted = ctz(enabled_interrupts);

... here, where we've already eliminated 0.  So, surely you could just use 
ctz64 directly.


> +        if (counted == IRQ_HOST) {
> +            /* we're handing it to the cpu now, so get rid of the qemu irq */
> +            qemu_irq_lower(HTIF_IRQ);
> +        } else if (counted == IRQ_M_TIMER) {
> +            /* we're handing it to the cpu now, so get rid of the qemu irq */
> +            qemu_irq_lower(TIMER_IRQ);
> +        } else if (counted == IRQ_S_TIMER || counted == IRQ_H_TIMER) {
> +            /* don't lower irq here */
> +        }
> +        return counted;
> +    } else {
> +        return EXCP_NONE; /* indicates no pending interrupt */
> +    }
> +}
> +
> +#include "exec/cpu-all.h"
> +
> +void riscv_tcg_init(void);
> +RISCVCPU *cpu_riscv_init(const char *cpu_model);
> +int cpu_riscv_signal_handler(int host_signum, void *pinfo, void *puc);
> +
> +#define cpu_init(cpu_model) CPU(cpu_riscv_init(cpu_model))
> +
> +/* hw/riscv/riscv_rtc.c  - supplies instret by approximating */
> +uint64_t cpu_riscv_read_instret(CPURISCVState *env);
> +
> +int riscv_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, MMUAccessType rw,
> +                              int mmu_idx);
> +#if !defined(CONFIG_USER_ONLY)
> +hwaddr cpu_riscv_translate_address(CPURISCVState *env, target_ulong address,
> +                                   int rw);
> +#endif
> +
> +static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> +                                        target_ulong *cs_base, uint32_t *flags)
> +{
> +    *pc = env->PC;
> +    *cs_base = 0;
> +    *flags = 0; /* necessary to avoid compiler warning */

At minimum, you'll need to include the priv level in the flags, so that one can 
only run TBs that were built for the cpu's current priv level.

> +++ b/target-riscv/op_helper.c
> @@ -0,0 +1,44 @@
> +/*
> + * RISC-V Emulation Helpers for QEMU.
> + *
> + * Author: Sagar Karandikar, address@hidden
> + *
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include <stdlib.h>
> +#include "cpu.h"
> +#include "qemu/host-utils.h"
> +#include "exec/helper-proto.h"
> +
> +#ifndef CONFIG_USER_ONLY
> +void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> +                                   MMUAccessType access_type, int mmu_idx,
> +                                   uintptr_t retaddr)
> +{
> +}
> +
> +void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
> +        int mmu_idx, uintptr_t retaddr)
> +{
> +}
> +
> +void riscv_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
> +        bool is_exec, int unused, unsigned size)
> +{
> +}

These are better placed in helper.c, reserving op_helper.c for those functions 
implementing helpers for "op"s, i.e. instructions.


r~