[Qemu-devel] Re: [PATCH 02/17] lm32: translation routines

[Edgar E. Iglesias]

On Mon, Feb 07, 2011 at 11:00:38PM +0100, Michael Walle wrote:
> Hi Edgar.
> 
> first of all let me thank you for the review.
> 
> On Mon, Feb 07 2011, 19:41:25, Edgar E. Iglesias wrote:
> > > +#define JMP_NOJMP    0
> > > +#define JMP_DIRECT   1
> > > +#define JMP_INDIRECT 2
> > 
> > These don't seem to be used, can we remove them?
> Good catch :) I'll remove them.
> 
> > > +
> > > +/* This is the state at translation time.  */
> > > +typedef struct DisasContext {
> > > +    CPUState *env;
> > > +    target_ulong pc;
> > > +
> > > +    /* Decoder.  */
> > > +    int format;
> > > +    uint32_t ir;
> > > +    uint8_t opcode;
> > > +    uint8_t r0, r1, r2, csr;
> > > +    uint16_t imm5;
> > > +    uint16_t imm16;
> > > +    uint32_t imm26;
> > > +
> > > +    unsigned int delayed_branch;
> > > +    unsigned int tb_flags, synced_flags; /* tb dependent flags.  */
> > > +    int is_jmp;
> > > +
> > > +    unsigned int jmp;
> > > +    uint32_t jmp_pc;
> > 
> > These too.
> Ditto.
> 
> > > +
> > > +    int nr_nops;
> > 
> > This should probably go aswell..
> See below.
> 
> > > +    /* try guessing 'empty' instruction memory, although it may be a
> > > valid +     * instruction sequence (eg. srui r0, r0, 0) */
> > > +    if (dc->ir) {
> > > +        dc->nr_nops = 0;
> > > +    } else {
> > > +        LOG_DIS("nr_nops=%d\t", dc->nr_nops);
> > > +        dc->nr_nops++;
> > > +        if (dc->nr_nops > 4) {
> > > +            cpu_abort(dc->env, "fetching nop sequence\n");
> > > +        }
> > > +    }
> > 
> > This nop sequence detection should probably go away. A reminder for
> > me too to remove it from the microblaze port (from where I guess you
> > inherited it)..
> I've always found this 'feature' handy, esp when you are porting new 
> software. 
> Is there any particular reason why this should be removed? Besides the fact 
> that it won't match the real hardware if those instructions are executed. In 
> the case of the LM32, these are really nonsense instructions (shift r0 right 
> by 0, r0 is by definition 0).

Yes, I found the nop tracking handy too but It exposes a way for guests
to completely abort the vm with valid code. On microblaze, even guest
userspace can trig the cpu_abort. Your port doesn't have an MMU so guest
userland can probably kill the vm anyway, so it might be less of an issue.

Maybe the event could be logged instead of cpu_abort:ed?

Anyway, I don't think this is a showstopper preventing your port from
going upstream. If you insist and none of the other maintainers feel
strongly about it, I'm ok with it too.

Cheers