Re: [Qemu-devel] [PATCH 01/10] target-avr: AVR cores support is added. 1. basic CPU structure 2. registers 3. no instructions

[Richard Henderson]

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~