On 06/02/2016 01:06 PM, Michael Rolnik wrote:
diff --git a/disas/avr.c b/disas/avr.c
new file mode 100644
index 0000000..f916e72
--- /dev/null
+++ b/disas/avr.c
@@ -0,0 +1,10 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "disas/bfd.h"
+
+int print_insn_avr(bfd_vma addr, disassemble_info *info)
+{
+ int length = 0;;
+ /* TODO */
+ return length;
+}
Since you never fill in this stub, drop it. There is already a fall-back in
disas.c if there is no printer for the guest.
+void avr_cpu_do_interrupt(
+ CPUState *cpu);
One more time: bad formatting.
I don't know what you're trying to accomplish with aligning all of these
arguments and function names, but you may have noticed that we do this no where
else in qemu.
Personally, I find all this extra whitespace hard to read.
Finally, you really do need to fix all of the warnings that checkpatch.pl
generates. There are 80 ERRORs and 433 WARNINGs in this patch set.
+static void avr_cpu_reset(
+ CPUState *s)
+{
+ AVRCPU *cpu = AVR_CPU(s);
+ AVRCPUClass *mcc = AVR_CPU_GET_CLASS(cpu);
+ CPUAVRState *env = &cpu->env;
+
+ mcc->parent_reset(s);
+
+ memset(env, 0, sizeof(CPUAVRState));
Normally one doesn't clear anything past "features". In particular, the stuff
in CPU_COMMON may need to stay as-is.
+#define TARGET_IS_LIENDIAN 1
Typo for BIENDIAN?
+#define CODE_INDEX 0
+#define DATA_INDEX 1
Better to use the standard naming, MMU_FOO_IDX.
+static inline int cpu_mmu_index(
+ CPUAVRState *env,
+ bool ifetch)
+{
+ return 0;
+}
Probably returning MMU_DATA_IDX would be more intelligable.
+static inline void cpu_get_tb_cpu_state(
+ CPUAVRState *env,
+ target_ulong *pc,
+ target_ulong *cs_base,
+ uint32_t *pflags)
+{
+ *pc = env->pc * 2;
I wonder if it would be helpful to change the name of env->pc so as to
emphasize that it's a word address. Otherwise these conversions can be confusing.
Perhaps
*pc_b = env->pc_w * 2;
uint32_t sregC; /* 1 bits */
uint32_t sregZ; /* 1 bits */
uint32_t sregN; /* 1 bits */
uint32_t sregV; /* 1 bits */
uint32_t sregS; /* 1 bits */
uint32_t sregH; /* 1 bits */
uint32_t sregT; /* 1 bits */
uint32_t sregI; /* 1 bits */
...
static inline int cpu_interrupts_enabled(CPUAVRState *env1)
{
return env1->sregI != 0;
}
...
+ sreg = (env->sregC & 0x01) << 0
+ | (env->sregZ & 0x01) << 1
+ | (env->sregN & 0x01) << 2
+ | (env->sregV & 0x01) << 3
+ | (env->sregS & 0x01) << 4
+ | (env->sregH & 0x01) << 5
+ | (env->sregT & 0x01) << 6
+ | (env->sregI & 0x01) << 7;
...
env->sregC = (tmp >> 0) & 0x01;
env->sregZ = (tmp >> 1) & 0x01;
env->sregN = (tmp >> 2) & 0x01;
env->sregV = (tmp >> 3) & 0x01;
env->sregS = (tmp >> 4) & 0x01;
env->sregH = (tmp >> 5) & 0x01;
env->sregT = (tmp >> 6) & 0x01;
env->sregI = (tmp >> 7) & 0x01;
Consider putting these into functions. Compare the target-arm functions
pstate_read and pstate_write.
You should also decide if you're going to canonicalize on a single bit set in
these variables or not. If they're always set via a mask, as above, then you
don't need to mask again when you read, nor do you need to compare vs 0 when
testing.
+void tlb_fill(
+ CPUState *cs,
+ target_ulong vaddr,
+ int is_write,
+ int mmu_idx,
+ uintptr_t retaddr)
+{
+ target_ulong page_size = TARGET_PAGE_SIZE;
+ int prot = 0;
+ MemTxAttrs attrs = {};
+ uint32_t paddr;
+
+ vaddr &= TARGET_PAGE_MASK;
+
+ if (mmu_idx == CODE_INDEX) {
+ paddr = PHYS_CODE_BASE + vaddr;
+ prot = PAGE_READ | PAGE_EXEC;
+ } else {
+ paddr = PHYS_DATA_BASE + vaddr;
+ prot = PAGE_READ | PAGE_WRITE;
+ }
+
+ tlb_set_page_with_attrs(cs, vaddr, paddr, attrs, prot, mmu_idx, page_size);
If there's no virtual memory, why do you set TARGET_PAGE_BITS so low?
Performance of qemu itself is better with a larger page size. Indeed, setting
it to the gcd of allowable ram sizes would produce the minimum number of tlb
entries.
+static inline void gen_goto_tb(CPUAVRState *env,
+ DisasContext *ctx,
+ int n,
+ target_ulong dest)
+{
+ TranslationBlock *tb;
+
+ tb = ctx->tb;
+
+ if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)
+ && (ctx->singlestep == 0)) {
+ tcg_gen_goto_tb(n);
+ tcg_gen_movi_i32(cpu_pc, dest);
+ tcg_gen_exit_tb((uintptr_t)tb + n);
+ } else {
+ tcg_gen_movi_i32(cpu_pc, dest);
+
+ if (ctx->singlestep) {
+ gen_helper_debug(cpu_env);
+ }
+ tcg_gen_exit_tb(0);
+ }
Since you have no paging, you don't need to worry about chains crossing pages.
Therefore this can be simplified to
if (ctx->singlestep) {
tcg_gen_movi_i32(cpu_pc, dest);
gen_helper_debug(cpu_env);
} else {
tcg_gen_goto_tb(n);
tcg_gen_movi_i32(cpu_pc, dest);
tcg_gen_exit_tb((uintptr_t)tb + n);
}
+ cpu_fprintf(f, "\n");
+ cpu_fprintf(f, "PC: %06x\n", env->pc);
+ cpu_fprintf(f, "SP: %04x\n", env->sp);
+ cpu_fprintf(f, "rampX: %02x\n", env->rampX);
+ cpu_fprintf(f, "rampY: %02x\n", env->rampY);
+ cpu_fprintf(f, "rampZ: %02x\n", env->rampZ);
+ cpu_fprintf(f, "rampD: %02x\n", env->rampD);
+ cpu_fprintf(f, "EIND: %02x\n", env->eind);
+ cpu_fprintf(f, "X: %02x%02x\n", env->r[27], env->r[26]);
+ cpu_fprintf(f, "Y: %02x%02x\n", env->r[29], env->r[28]);
+ cpu_fprintf(f, "Z: %02x%02x\n", env->r[31], env->r[30]);
Surely we can print this in a more compact form.
+ cpu_fprintf(f, " [ I T H S V N Z C ]\n");
+ cpu_fprintf(f, "SREG: [ %d %d %d %d %d %d %d %d ]\n",
A more compact form for this would be
"SREG: [ %c %c %c %c %c %c %c %c ]"
env->sregI ? 'I' : '-',
...
r~
