diff mbox series

[v32,04/13] target/avr: Add instruction translation - Registers definition

Message ID 20191014161843.84845-5-mrolnik@gmail.com
State New
Headers show
Series QEMU AVR 8 bit cores | expand

Commit Message

Michael Rolnik Oct. 14, 2019, 4:18 p.m. UTC 
Signed-off-by: Michael Rolnik <mrolnik@gmail.com>
---
 target/avr/translate.c | 132 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100644 target/avr/translate.c

Comments

Aleksandar Markovic Oct. 17, 2019, 5:25 p.m. UTC | #1 
On Monday, October 14, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:

> Signed-off-by: Michael Rolnik <mrolnik@gmail.com>
> ---
>  target/avr/translate.c | 132 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 132 insertions(+)
>  create mode 100644 target/avr/translate.c
>
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> new file mode 100644
> index 0000000000..53c9892a60
> --- /dev/null
> +++ b/target/avr/translate.c
> @@ -0,0 +1,132 @@
> +/*
> + * QEMU AVR CPU
> + *
> + * Copyright (c) 2019 Michael Rolnik
> + *
> + * 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.1 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/lgpl-2.1.html>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/qemu-print.h"
> +#include "tcg/tcg.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +#include "disas/disas.h"
> +#include "tcg-op.h"
> +#include "exec/cpu_ldst.h"
> +#include "exec/helper-proto.h"
> +#include "exec/helper-gen.h"
> +#include "exec/log.h"
> +#include "exec/gdbstub.h"
> +#include "exec/translator.h"
> +#include "exec/gen-icount.h"
> +
> +/*
> + *  Define if you want a BREAK instruction translated to a breakpoint
> + *  Active debugging connection is assumed
> + *  This is for
> + *  https://github.com/seharris/qemu-avr-tests/tree/master/
> instruction-tests
> + *  tests
> + */
> +#undef BREAKPOINT_ON_BREAK
> +
> +static TCGv cpu_pc;
> +
> +static TCGv cpu_Cf;
> +static TCGv cpu_Zf;
> +static TCGv cpu_Nf;
> +static TCGv cpu_Vf;
> +static TCGv cpu_Sf;
> +static TCGv cpu_Hf;
> +static TCGv cpu_Tf;
> +static TCGv cpu_If;
> +


Hello, Michael,

Is there any particular reason or motivation beyond modelling status
register flags as TCGv variables?

A.




> +static TCGv cpu_rampD;
> +static TCGv cpu_rampX;
> +static TCGv cpu_rampY;
> +static TCGv cpu_rampZ;
> +
> +static TCGv cpu_r[NO_CPU_REGISTERS];
> +static TCGv cpu_eind;
> +static TCGv cpu_sp;
> +
> +static TCGv cpu_skip;
> +
> +static const char reg_names[NO_CPU_REGISTERS][8] = {
> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
> +};
> +#define REG(x) (cpu_r[x])
> +
> +enum {
> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main
> loop.  */
> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition
> exit.  */
> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.
> */
> +};
> +
> +typedef struct DisasContext DisasContext;
> +
> +/* This is the state at translation time. */
> +struct DisasContext {
> +    TranslationBlock *tb;
> +
> +    CPUAVRState *env;
> +    CPUState *cs;
> +
> +    target_long npc;
> +    uint32_t opcode;
> +
> +    /* Routine used to access memory */
> +    int memidx;
> +    int bstate;
> +    int singlestep;
> +
> +    TCGv skip_var0;
> +    TCGv skip_var1;
> +    TCGCond skip_cond;
> +    bool free_skip_var0;
> +};
> +
> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) *
> 2; }
> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
> +
> +static uint16_t next_word(DisasContext *ctx)
> +{
> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
> +}
> +
> +static int append_16(DisasContext *ctx, int x)
> +{
> +    return x << 16 | next_word(ctx);
> +}
> +
> +
> +static bool avr_have_feature(DisasContext *ctx, int feature)
> +{
> +    if (!avr_feature(ctx->env, feature)) {
> +        gen_helper_unsupported(cpu_env);
> +        ctx->bstate = DISAS_NORETURN;
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
> +#include "decode_insn.inc.c"
> +
> --
> 2.17.2 (Apple Git-113)
>
>
Michael Rolnik Oct. 17, 2019, 7:15 p.m. UTC | #2 
On Thu, Oct 17, 2019 at 8:25 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
>
>
>
> On Monday, October 14, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>
>> Signed-off-by: Michael Rolnik <mrolnik@gmail.com>
>> ---
>>  target/avr/translate.c | 132 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 132 insertions(+)
>>  create mode 100644 target/avr/translate.c
>>
>> diff --git a/target/avr/translate.c b/target/avr/translate.c
>> new file mode 100644
>> index 0000000000..53c9892a60
>> --- /dev/null
>> +++ b/target/avr/translate.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * QEMU AVR CPU
>> + *
>> + * Copyright (c) 2019 Michael Rolnik
>> + *
>> + * 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.1 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/lgpl-2.1.html>
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/qemu-print.h"
>> +#include "tcg/tcg.h"
>> +#include "cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "disas/disas.h"
>> +#include "tcg-op.h"
>> +#include "exec/cpu_ldst.h"
>> +#include "exec/helper-proto.h"
>> +#include "exec/helper-gen.h"
>> +#include "exec/log.h"
>> +#include "exec/gdbstub.h"
>> +#include "exec/translator.h"
>> +#include "exec/gen-icount.h"
>> +
>> +/*
>> + *  Define if you want a BREAK instruction translated to a breakpoint
>> + *  Active debugging connection is assumed
>> + *  This is for
>> + *  https://github.com/seharris/qemu-avr-tests/tree/master/instruction-tests
>> + *  tests
>> + */
>> +#undef BREAKPOINT_ON_BREAK
>> +
>> +static TCGv cpu_pc;
>> +
>> +static TCGv cpu_Cf;
>> +static TCGv cpu_Zf;
>> +static TCGv cpu_Nf;
>> +static TCGv cpu_Vf;
>> +static TCGv cpu_Sf;
>> +static TCGv cpu_Hf;
>> +static TCGv cpu_Tf;
>> +static TCGv cpu_If;
>> +
>
>
> Hello, Michael,
>
> Is there any particular reason or motivation beyond modelling status register flags as TCGv variables?
I think it's easier this way as I don't need to convert flag values to
bits or bits to flag values.
>
> A.
>
>
>
>>
>> +static TCGv cpu_rampD;
>> +static TCGv cpu_rampX;
>> +static TCGv cpu_rampY;
>> +static TCGv cpu_rampZ;
>> +
>> +static TCGv cpu_r[NO_CPU_REGISTERS];
>> +static TCGv cpu_eind;
>> +static TCGv cpu_sp;
>> +
>> +static TCGv cpu_skip;
>> +
>> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>> +};
>> +#define REG(x) (cpu_r[x])
>> +
>> +enum {
>> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main loop.  */
>> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition exit.  */
>> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.  */
>> +};
>> +
>> +typedef struct DisasContext DisasContext;
>> +
>> +/* This is the state at translation time. */
>> +struct DisasContext {
>> +    TranslationBlock *tb;
>> +
>> +    CPUAVRState *env;
>> +    CPUState *cs;
>> +
>> +    target_long npc;
>> +    uint32_t opcode;
>> +
>> +    /* Routine used to access memory */
>> +    int memidx;
>> +    int bstate;
>> +    int singlestep;
>> +
>> +    TCGv skip_var0;
>> +    TCGv skip_var1;
>> +    TCGCond skip_cond;
>> +    bool free_skip_var0;
>> +};
>> +
>> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
>> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
>> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) * 2; }
>> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
>> +
>> +static uint16_t next_word(DisasContext *ctx)
>> +{
>> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>> +}
>> +
>> +static int append_16(DisasContext *ctx, int x)
>> +{
>> +    return x << 16 | next_word(ctx);
>> +}
>> +
>> +
>> +static bool avr_have_feature(DisasContext *ctx, int feature)
>> +{
>> +    if (!avr_feature(ctx->env, feature)) {
>> +        gen_helper_unsupported(cpu_env);
>> +        ctx->bstate = DISAS_NORETURN;
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>> +#include "decode_insn.inc.c"
>> +
>> --
>> 2.17.2 (Apple Git-113)
>>
Aleksandar Markovic Oct. 17, 2019, 8:16 p.m. UTC | #3 
>
>
> >> +static TCGv cpu_Cf;
> >> +static TCGv cpu_Zf;
> >> +static TCGv cpu_Nf;
> >> +static TCGv cpu_Vf;
> >> +static TCGv cpu_Sf;
> >> +static TCGv cpu_Hf;
> >> +static TCGv cpu_Tf;
> >> +static TCGv cpu_If;
> >> +
> >
> >
> > Hello, Michael,
> >
> > Is there any particular reason or motivation beyond modelling status
> register flags as TCGv variables?



I think it's easier this way as I don't need to convert flag values to
> bits or bits to flag values.


Ok. But, how do you map 0/1 flag value to the value of a TCGv variable and
vice versa? In other words, what value or values (out of 2^32 vales) of a
TCGv variable mean the flag is 1? And the same question for 0.

Is 0110000111000010100 one or zero?

Besides, in such arrangement, how do you display the 8-bit status register
in gdb, if at all?

A.


> >
> > A.
> >
> >
> >
> >>
> >> +static TCGv cpu_rampD;
> >> +static TCGv cpu_rampX;
> >> +static TCGv cpu_rampY;
> >> +static TCGv cpu_rampZ;
> >> +
> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
> >> +static TCGv cpu_eind;
> >> +static TCGv cpu_sp;
> >> +
> >> +static TCGv cpu_skip;
> >> +
> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
> >> +};
> >> +#define REG(x) (cpu_r[x])
> >> +
> >> +enum {
> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main
> loop.  */
> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition
> exit.  */
> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition
> exit.  */
> >> +};
> >> +
> >> +typedef struct DisasContext DisasContext;
> >> +
> >> +/* This is the state at translation time. */
> >> +struct DisasContext {
> >> +    TranslationBlock *tb;
> >> +
> >> +    CPUAVRState *env;
> >> +    CPUState *cs;
> >> +
> >> +    target_long npc;
> >> +    uint32_t opcode;
> >> +
> >> +    /* Routine used to access memory */
> >> +    int memidx;
> >> +    int bstate;
> >> +    int singlestep;
> >> +
> >> +    TCGv skip_var0;
> >> +    TCGv skip_var1;
> >> +    TCGCond skip_cond;
> >> +    bool free_skip_var0;
> >> +};
> >> +
> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx %
> 16); }
> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8);
> }
> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4)
> * 2; }
> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2;
> }
> >> +
> >> +static uint16_t next_word(DisasContext *ctx)
> >> +{
> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
> >> +}
> >> +
> >> +static int append_16(DisasContext *ctx, int x)
> >> +{
> >> +    return x << 16 | next_word(ctx);
> >> +}
> >> +
> >> +
> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
> >> +{
> >> +    if (!avr_feature(ctx->env, feature)) {
> >> +        gen_helper_unsupported(cpu_env);
> >> +        ctx->bstate = DISAS_NORETURN;
> >> +        return false;
> >> +    }
> >> +    return true;
> >> +}
> >> +
> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
> >> +#include "decode_insn.inc.c"
> >> +
> >> --
> >> 2.17.2 (Apple Git-113)
> >>
>
>
> --
> Best Regards,
> Michael Rolnik
>
Michael Rolnik Oct. 17, 2019, 8:43 p.m. UTC | #4 
On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
>>
>>
>> >> +static TCGv cpu_Cf;
>> >> +static TCGv cpu_Zf;
>> >> +static TCGv cpu_Nf;
>> >> +static TCGv cpu_Vf;
>> >> +static TCGv cpu_Sf;
>> >> +static TCGv cpu_Hf;
>> >> +static TCGv cpu_Tf;
>> >> +static TCGv cpu_If;
>> >> +
>> >
>> >
>> > Hello, Michael,
>> >
>> > Is there any particular reason or motivation beyond modelling status register flags as TCGv variables?
>>
>>
>>
>> I think it's easier this way as I don't need to convert flag values to
>> bits or bits to flag values.
>
>
> Ok. But, how do you map 0/1 flag value to the value of a TCGv variable and vice versa? In other words, what value or values (out of 2^32 vales) of a TCGv variable mean the flag is 1? And the same question for 0.
>
> Is 0110000111000010100 one or zero?
>
> Besides, in such arrangement, how do you display the 8-bit status register in gdb, if at all?

each flag register is either 0 or 1, they are calculated here
1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L146-L148
2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L166
3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L185-L187
4. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L205
5. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L214-L215
6. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L222-L223
The COU itself never uses SREG at all, only the flags.

As for the GDB it's get assembled/disassembled here
1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/cpu.h#L219-L243
2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L35-L37
3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L66-L68

>
> A.
>
>>
>> >
>> > A.
>> >
>> >
>> >
>> >>
>> >> +static TCGv cpu_rampD;
>> >> +static TCGv cpu_rampX;
>> >> +static TCGv cpu_rampY;
>> >> +static TCGv cpu_rampZ;
>> >> +
>> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>> >> +static TCGv cpu_eind;
>> >> +static TCGv cpu_sp;
>> >> +
>> >> +static TCGv cpu_skip;
>> >> +
>> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>> >> +};
>> >> +#define REG(x) (cpu_r[x])
>> >> +
>> >> +enum {
>> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main loop.  */
>> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition exit.  */
>> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.  */
>> >> +};
>> >> +
>> >> +typedef struct DisasContext DisasContext;
>> >> +
>> >> +/* This is the state at translation time. */
>> >> +struct DisasContext {
>> >> +    TranslationBlock *tb;
>> >> +
>> >> +    CPUAVRState *env;
>> >> +    CPUState *cs;
>> >> +
>> >> +    target_long npc;
>> >> +    uint32_t opcode;
>> >> +
>> >> +    /* Routine used to access memory */
>> >> +    int memidx;
>> >> +    int bstate;
>> >> +    int singlestep;
>> >> +
>> >> +    TCGv skip_var0;
>> >> +    TCGv skip_var1;
>> >> +    TCGCond skip_cond;
>> >> +    bool free_skip_var0;
>> >> +};
>> >> +
>> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
>> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
>> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) * 2; }
>> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
>> >> +
>> >> +static uint16_t next_word(DisasContext *ctx)
>> >> +{
>> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>> >> +}
>> >> +
>> >> +static int append_16(DisasContext *ctx, int x)
>> >> +{
>> >> +    return x << 16 | next_word(ctx);
>> >> +}
>> >> +
>> >> +
>> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>> >> +{
>> >> +    if (!avr_feature(ctx->env, feature)) {
>> >> +        gen_helper_unsupported(cpu_env);
>> >> +        ctx->bstate = DISAS_NORETURN;
>> >> +        return false;
>> >> +    }
>> >> +    return true;
>> >> +}
>> >> +
>> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>> >> +#include "decode_insn.inc.c"
>> >> +
>> >> --
>> >> 2.17.2 (Apple Git-113)
>> >>
>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
Aleksandar Markovic Oct. 18, 2019, 8:52 a.m. UTC | #5 
On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:

> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
> <aleksandar.m.mail@gmail.com> wrote:
> >>
> >>
> >> >> +static TCGv cpu_Cf;
> >> >> +static TCGv cpu_Zf;
> >> >> +static TCGv cpu_Nf;
> >> >> +static TCGv cpu_Vf;
> >> >> +static TCGv cpu_Sf;
> >> >> +static TCGv cpu_Hf;
> >> >> +static TCGv cpu_Tf;
> >> >> +static TCGv cpu_If;
> >> >> +
> >> >
> >> >
> >> > Hello, Michael,
> >> >
> >> > Is there any particular reason or motivation beyond modelling status
> register flags as TCGv variables?
> >>
> >>
> >>
> >> I think it's easier this way as I don't need to convert flag values to
> >> bits or bits to flag values.
> >
> >
> > Ok. But, how do you map 0/1 flag value to the value of a TCGv variable
> and vice versa? In other words, what value or values (out of 2^32 vales) of
> a TCGv variable mean the flag is 1? And the same question for 0.
> >
> > Is 0110000111000010100 one or zero?
> >
> > Besides, in such arrangement, how do you display the 8-bit status
> register in gdb, if at all?
>
> each flag register is either 0 or 1,....


>
>
Michael,

If this is true, why is there a special handling of two flags in the
following code:


static inline uint8_t cpu_get_sreg(CPUAVRState *env)
{
uint8_t sreg;
sreg = (env->sregC & 0x01) << 0
| (env->sregZ == 0 ? 1 : 0) << 1
| (env->sregN) << 2
| (env->sregV) << 3
| (env->sregS) << 4
| (env->sregH) << 5
| (env->sregT) << 6
| (env->sregI) << 7;
return sreg;
}
static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
{
env->sregC = (sreg >> 0) & 0x01;
env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
env->sregN = (sreg >> 2) & 0x01;
env->sregV = (sreg >> 3) & 0x01;
env->sregS = (sreg >> 4) & 0x01;
env->sregH = (sreg >> 5) & 0x01;
env->sregT = (sreg >> 6) & 0x01;
env->sregI = (sreg >> 7) & 0x01;
}
 ?


Thanks,
A.

>
>  they are calculated here
> 1. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L146-L148
> 2. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L166
> 3. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L185-L187
> 4. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L205
> 5. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L214-L215
> 6. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L222-L223
> The COU itself never uses SREG at all, only the flags.
>
> As for the GDB it's get assembled/disassembled here
> 1. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/cpu.h#L219-L243
> 2. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/gdbstub.c#L35-L37
> 3. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/gdbstub.c#L66-L68
>
> >
> > A.
> >
> >>
> >> >
> >> > A.
> >> >
> >> >
> >> >
> >> >>
> >> >> +static TCGv cpu_rampD;
> >> >> +static TCGv cpu_rampX;
> >> >> +static TCGv cpu_rampY;
> >> >> +static TCGv cpu_rampZ;
> >> >> +
> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
> >> >> +static TCGv cpu_eind;
> >> >> +static TCGv cpu_sp;
> >> >> +
> >> >> +static TCGv cpu_skip;
> >> >> +
> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
> >> >> +};
> >> >> +#define REG(x) (cpu_r[x])
> >> >> +
> >> >> +enum {
> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu
> main loop.  */
> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition
> exit.  */
> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition
> exit.  */
> >> >> +};
> >> >> +
> >> >> +typedef struct DisasContext DisasContext;
> >> >> +
> >> >> +/* This is the state at translation time. */
> >> >> +struct DisasContext {
> >> >> +    TranslationBlock *tb;
> >> >> +
> >> >> +    CPUAVRState *env;
> >> >> +    CPUState *cs;
> >> >> +
> >> >> +    target_long npc;
> >> >> +    uint32_t opcode;
> >> >> +
> >> >> +    /* Routine used to access memory */
> >> >> +    int memidx;
> >> >> +    int bstate;
> >> >> +    int singlestep;
> >> >> +
> >> >> +    TCGv skip_var0;
> >> >> +    TCGv skip_var1;
> >> >> +    TCGCond skip_cond;
> >> >> +    bool free_skip_var0;
> >> >> +};
> >> >> +
> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx %
> 16); }
> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx %
> 8); }
> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx %
> 4) * 2; }
> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) *
> 2; }
> >> >> +
> >> >> +static uint16_t next_word(DisasContext *ctx)
> >> >> +{
> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
> >> >> +}
> >> >> +
> >> >> +static int append_16(DisasContext *ctx, int x)
> >> >> +{
> >> >> +    return x << 16 | next_word(ctx);
> >> >> +}
> >> >> +
> >> >> +
> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
> >> >> +{
> >> >> +    if (!avr_feature(ctx->env, feature)) {
> >> >> +        gen_helper_unsupported(cpu_env);
> >> >> +        ctx->bstate = DISAS_NORETURN;
> >> >> +        return false;
> >> >> +    }
> >> >> +    return true;
> >> >> +}
> >> >> +
> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
> >> >> +#include "decode_insn.inc.c"
> >> >> +
> >> >> --
> >> >> 2.17.2 (Apple Git-113)
> >> >>
> >>
> >>
> >> --
> >> Best Regards,
> >> Michael Rolnik
>
>
>
> --
> Best Regards,
> Michael Rolnik
>
Michael Rolnik Oct. 18, 2019, 11:27 a.m. UTC | #6 
On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <
aleksandar.m.mail@gmail.com> wrote:

>
>
> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>
>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>> <aleksandar.m.mail@gmail.com> wrote:
>> >>
>> >>
>> >> >> +static TCGv cpu_Cf;
>> >> >> +static TCGv cpu_Zf;
>> >> >> +static TCGv cpu_Nf;
>> >> >> +static TCGv cpu_Vf;
>> >> >> +static TCGv cpu_Sf;
>> >> >> +static TCGv cpu_Hf;
>> >> >> +static TCGv cpu_Tf;
>> >> >> +static TCGv cpu_If;
>> >> >> +
>> >> >
>> >> >
>> >> > Hello, Michael,
>> >> >
>> >> > Is there any particular reason or motivation beyond modelling status
>> register flags as TCGv variables?
>> >>
>> >>
>> >>
>> >> I think it's easier this way as I don't need to convert flag values to
>> >> bits or bits to flag values.
>> >
>> >
>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv variable
>> and vice versa? In other words, what value or values (out of 2^32 vales) of
>> a TCGv variable mean the flag is 1? And the same question for 0.
>> >
>> > Is 0110000111000010100 one or zero?
>> >
>> > Besides, in such arrangement, how do you display the 8-bit status
>> register in gdb, if at all?
>>
>> each flag register is either 0 or 1,....
>
>
>>
>>
> Michael,
>
> If this is true, why is there a special handling of two flags in the
> following code:
>
>
> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
> {
> uint8_t sreg;
> sreg = (env->sregC & 0x01) << 0
> | (env->sregZ == 0 ? 1 : 0) << 1
> | (env->sregN) << 2
> | (env->sregV) << 3
> | (env->sregS) << 4
> | (env->sregH) << 5
> | (env->sregT) << 6
> | (env->sregI) << 7;
> return sreg;
> }
> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
> {
> env->sregC = (sreg >> 0) & 0x01;
> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
> env->sregN = (sreg >> 2) & 0x01;
> env->sregV = (sreg >> 3) & 0x01;
> env->sregS = (sreg >> 4) & 0x01;
> env->sregH = (sreg >> 5) & 0x01;
> env->sregT = (sreg >> 6) & 0x01;
> env->sregI = (sreg >> 7) & 0x01;
> }
>  ?
>
> Aleksandar,

If I understand your question correctly cpu_get_sreg assembles SREG value
to be presented by GDB, and cpu_set_sreg sets flags values when GDB
modifies SREG.

Michael


>
> Thanks,
> A.
>
>>
>>  they are calculated here
>> 1.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L146-L148
>> 2.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L166
>> 3.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L185-L187
>> 4.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L205
>> 5.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L214-L215
>> 6.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L222-L223
>> The COU itself never uses SREG at all, only the flags.
>>
>> As for the GDB it's get assembled/disassembled here
>> 1.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/cpu.h#L219-L243
>> 2.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L35-L37
>> 3.
>> https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L66-L68
>>
>> >
>> > A.
>> >
>> >>
>> >> >
>> >> > A.
>> >> >
>> >> >
>> >> >
>> >> >>
>> >> >> +static TCGv cpu_rampD;
>> >> >> +static TCGv cpu_rampX;
>> >> >> +static TCGv cpu_rampY;
>> >> >> +static TCGv cpu_rampZ;
>> >> >> +
>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>> >> >> +static TCGv cpu_eind;
>> >> >> +static TCGv cpu_sp;
>> >> >> +
>> >> >> +static TCGv cpu_skip;
>> >> >> +
>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>> >> >> +};
>> >> >> +#define REG(x) (cpu_r[x])
>> >> >> +
>> >> >> +enum {
>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu
>> main loop.  */
>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable
>> condition exit.  */
>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition
>> exit.  */
>> >> >> +};
>> >> >> +
>> >> >> +typedef struct DisasContext DisasContext;
>> >> >> +
>> >> >> +/* This is the state at translation time. */
>> >> >> +struct DisasContext {
>> >> >> +    TranslationBlock *tb;
>> >> >> +
>> >> >> +    CPUAVRState *env;
>> >> >> +    CPUState *cs;
>> >> >> +
>> >> >> +    target_long npc;
>> >> >> +    uint32_t opcode;
>> >> >> +
>> >> >> +    /* Routine used to access memory */
>> >> >> +    int memidx;
>> >> >> +    int bstate;
>> >> >> +    int singlestep;
>> >> >> +
>> >> >> +    TCGv skip_var0;
>> >> >> +    TCGv skip_var1;
>> >> >> +    TCGCond skip_cond;
>> >> >> +    bool free_skip_var0;
>> >> >> +};
>> >> >> +
>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx %
>> 16); }
>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx %
>> 8); }
>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx %
>> 4) * 2; }
>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16)
>> * 2; }
>> >> >> +
>> >> >> +static uint16_t next_word(DisasContext *ctx)
>> >> >> +{
>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>> >> >> +}
>> >> >> +
>> >> >> +static int append_16(DisasContext *ctx, int x)
>> >> >> +{
>> >> >> +    return x << 16 | next_word(ctx);
>> >> >> +}
>> >> >> +
>> >> >> +
>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>> >> >> +{
>> >> >> +    if (!avr_feature(ctx->env, feature)) {
>> >> >> +        gen_helper_unsupported(cpu_env);
>> >> >> +        ctx->bstate = DISAS_NORETURN;
>> >> >> +        return false;
>> >> >> +    }
>> >> >> +    return true;
>> >> >> +}
>> >> >> +
>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>> >> >> +#include "decode_insn.inc.c"
>> >> >> +
>> >> >> --
>> >> >> 2.17.2 (Apple Git-113)
>> >> >>
>> >>
>> >>
>> >> --
>> >> Best Regards,
>> >> Michael Rolnik
>>
>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
>>
>
Michael Rolnik Oct. 18, 2019, 12:10 p.m. UTC | #7 
resending.


On Fri, Oct 18, 2019 at 2:27 PM Michael Rolnik <mrolnik@gmail.com> wrote:
>
>
>
> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote:
>>
>>
>>
>> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>>
>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>>> <aleksandar.m.mail@gmail.com> wrote:
>>> >>
>>> >>
>>> >> >> +static TCGv cpu_Cf;
>>> >> >> +static TCGv cpu_Zf;
>>> >> >> +static TCGv cpu_Nf;
>>> >> >> +static TCGv cpu_Vf;
>>> >> >> +static TCGv cpu_Sf;
>>> >> >> +static TCGv cpu_Hf;
>>> >> >> +static TCGv cpu_Tf;
>>> >> >> +static TCGv cpu_If;
>>> >> >> +
>>> >> >
>>> >> >
>>> >> > Hello, Michael,
>>> >> >
>>> >> > Is there any particular reason or motivation beyond modelling status register flags as TCGv variables?
>>> >>
>>> >>
>>> >>
>>> >> I think it's easier this way as I don't need to convert flag values to
>>> >> bits or bits to flag values.
>>> >
>>> >
>>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv variable and vice versa? In other words, what value or values (out of 2^32 vales) of a TCGv variable mean the flag is 1? And the same question for 0.
>>> >
>>> > Is 0110000111000010100 one or zero?
>>> >
>>> > Besides, in such arrangement, how do you display the 8-bit status register in gdb, if at all?
>>>
>>> each flag register is either 0 or 1,....
>>>
>>>
>>>
>>
>> Michael,
>>
>> If this is true, why is there a special handling of two flags in the following code:
>>
>>
>> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
>> {
>> uint8_t sreg;
>> sreg = (env->sregC & 0x01) << 0
>> | (env->sregZ == 0 ? 1 : 0) << 1
>> | (env->sregN) << 2
>> | (env->sregV) << 3
>> | (env->sregS) << 4
>> | (env->sregH) << 5
>> | (env->sregT) << 6
>> | (env->sregI) << 7;
>> return sreg;
>> }
>> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
>> {
>> env->sregC = (sreg >> 0) & 0x01;
>> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
>> env->sregN = (sreg >> 2) & 0x01;
>> env->sregV = (sreg >> 3) & 0x01;
>> env->sregS = (sreg >> 4) & 0x01;
>> env->sregH = (sreg >> 5) & 0x01;
>> env->sregT = (sreg >> 6) & 0x01;
>> env->sregI = (sreg >> 7) & 0x01;
>> }
>>  ?
>>
> Aleksandar,
>
> If I understand your question correctly cpu_get_sreg assembles SREG value to be presented by GDB, and cpu_set_sreg sets flags values when GDB modifies SREG.
>
> Michael
>
>>
>>
>> Thanks,
>> A.
>>>
>>>
>>>  they are calculated here
>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L146-L148
>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L166
>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L185-L187
>>> 4. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L205
>>> 5. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L214-L215
>>> 6. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L222-L223
>>> The COU itself never uses SREG at all, only the flags.
>>>
>>> As for the GDB it's get assembled/disassembled here
>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/cpu.h#L219-L243
>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L35-L37
>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L66-L68
>>>
>>> >
>>> > A.
>>> >
>>> >>
>>> >> >
>>> >> > A.
>>> >> >
>>> >> >
>>> >> >
>>> >> >>
>>> >> >> +static TCGv cpu_rampD;
>>> >> >> +static TCGv cpu_rampX;
>>> >> >> +static TCGv cpu_rampY;
>>> >> >> +static TCGv cpu_rampZ;
>>> >> >> +
>>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>>> >> >> +static TCGv cpu_eind;
>>> >> >> +static TCGv cpu_sp;
>>> >> >> +
>>> >> >> +static TCGv cpu_skip;
>>> >> >> +
>>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>>> >> >> +};
>>> >> >> +#define REG(x) (cpu_r[x])
>>> >> >> +
>>> >> >> +enum {
>>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main loop.  */
>>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition exit.  */
>>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.  */
>>> >> >> +};
>>> >> >> +
>>> >> >> +typedef struct DisasContext DisasContext;
>>> >> >> +
>>> >> >> +/* This is the state at translation time. */
>>> >> >> +struct DisasContext {
>>> >> >> +    TranslationBlock *tb;
>>> >> >> +
>>> >> >> +    CPUAVRState *env;
>>> >> >> +    CPUState *cs;
>>> >> >> +
>>> >> >> +    target_long npc;
>>> >> >> +    uint32_t opcode;
>>> >> >> +
>>> >> >> +    /* Routine used to access memory */
>>> >> >> +    int memidx;
>>> >> >> +    int bstate;
>>> >> >> +    int singlestep;
>>> >> >> +
>>> >> >> +    TCGv skip_var0;
>>> >> >> +    TCGv skip_var1;
>>> >> >> +    TCGCond skip_cond;
>>> >> >> +    bool free_skip_var0;
>>> >> >> +};
>>> >> >> +
>>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
>>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
>>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) * 2; }
>>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
>>> >> >> +
>>> >> >> +static uint16_t next_word(DisasContext *ctx)
>>> >> >> +{
>>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>>> >> >> +}
>>> >> >> +
>>> >> >> +static int append_16(DisasContext *ctx, int x)
>>> >> >> +{
>>> >> >> +    return x << 16 | next_word(ctx);
>>> >> >> +}
>>> >> >> +
>>> >> >> +
>>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>>> >> >> +{
>>> >> >> +    if (!avr_feature(ctx->env, feature)) {
>>> >> >> +        gen_helper_unsupported(cpu_env);
>>> >> >> +        ctx->bstate = DISAS_NORETURN;
>>> >> >> +        return false;
>>> >> >> +    }
>>> >> >> +    return true;
>>> >> >> +}
>>> >> >> +
>>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>>> >> >> +#include "decode_insn.inc.c"
>>> >> >> +
>>> >> >> --
>>> >> >> 2.17.2 (Apple Git-113)
>>> >> >>
>>> >>
>>> >>
>>> >> --
>>> >> Best Regards,
>>> >> Michael Rolnik
>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Michael Rolnik
>
>
>
> --
> Best Regards,
> Michael Rolnik
Aleksandar Markovic Oct. 18, 2019, 1:23 p.m. UTC | #8 
On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:

>
>
> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <
> aleksandar.m.mail@gmail.com> wrote:
>
>>
>>
>> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>
>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>>> <aleksandar.m.mail@gmail.com> wrote:
>>> >>
>>> >>
>>> >> >> +static TCGv cpu_Cf;
>>> >> >> +static TCGv cpu_Zf;
>>> >> >> +static TCGv cpu_Nf;
>>> >> >> +static TCGv cpu_Vf;
>>> >> >> +static TCGv cpu_Sf;
>>> >> >> +static TCGv cpu_Hf;
>>> >> >> +static TCGv cpu_Tf;
>>> >> >> +static TCGv cpu_If;
>>> >> >> +
>>> >> >
>>> >> >
>>> >> > Hello, Michael,
>>> >> >
>>> >> > Is there any particular reason or motivation beyond modelling
>>> status register flags as TCGv variables?
>>> >>
>>> >>
>>> >>
>>> >> I think it's easier this way as I don't need to convert flag values to
>>> >> bits or bits to flag values.
>>> >
>>> >
>>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv variable
>>> and vice versa? In other words, what value or values (out of 2^32 vales) of
>>> a TCGv variable mean the flag is 1? And the same question for 0.
>>> >
>>> > Is 0110000111000010100 one or zero?
>>> >
>>> > Besides, in such arrangement, how do you display the 8-bit status
>>> register in gdb, if at all?
>>>
>>> each flag register is either 0 or 1,....
>>
>>
>>>
>>>
>> Michael,
>>
>> If this is true, why is there a special handling of two flags in the
>> following code:
>>
>>
>> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
>> {
>> uint8_t sreg;
>> sreg = (env->sregC & 0x01) << 0
>> | (env->sregZ == 0 ? 1 : 0) << 1
>> | (env->sregN) << 2
>> | (env->sregV) << 3
>> | (env->sregS) << 4
>> | (env->sregH) << 5
>> | (env->sregT) << 6
>> | (env->sregI) << 7;
>> return sreg;
>> }
>> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
>> {
>> env->sregC = (sreg >> 0) & 0x01;
>> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
>> env->sregN = (sreg >> 2) & 0x01;
>> env->sregV = (sreg >> 3) & 0x01;
>> env->sregS = (sreg >> 4) & 0x01;
>> env->sregH = (sreg >> 5) & 0x01;
>> env->sregT = (sreg >> 6) & 0x01;
>> env->sregI = (sreg >> 7) & 0x01;
>> }
>>  ?
>>
>> Aleksandar,
>
> If I understand your question correctly cpu_get_sreg assembles SREG value
> to be presented by GDB, and cpu_set_sreg sets flags values when GDB
> modifies SREG.
>
> Michael
>


Why is handling of sregC and sregZ flags different than handling of other
flags? This contradicts your previos statement that 1 (in TCGv) means 1
(flag), and 0 (in TCGv) means 0 (flag)?


Whatever is the explanation, ot should be included, in my opinion, in code
comments.

Please, Michael, let's first clarify the issue from the question above.

Thanks, Aleksandar





>> Thanks,
>> A.
>>
>>>
>>>  they are calculated here
>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/translate.c#L146-L148
>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/translate.c#L166
>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/translate.c#L185-L187
>>> 4. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/translate.c#L205
>>> 5. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/translate.c#L214-L215
>>> 6. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/translate.c#L222-L223
>>> The COU itself never uses SREG at all, only the flags.
>>>
>>> As for the GDB it's get assembled/disassembled here
>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/cpu.h#L219-L243
>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/gdbstub.c#L35-L37
>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/
>>> avr-v32/target/avr/gdbstub.c#L66-L68
>>>
>>> >
>>> > A.
>>> >
>>> >>
>>> >> >
>>> >> > A.
>>> >> >
>>> >> >
>>> >> >
>>> >> >>
>>> >> >> +static TCGv cpu_rampD;
>>> >> >> +static TCGv cpu_rampX;
>>> >> >> +static TCGv cpu_rampY;
>>> >> >> +static TCGv cpu_rampZ;
>>> >> >> +
>>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>>> >> >> +static TCGv cpu_eind;
>>> >> >> +static TCGv cpu_sp;
>>> >> >> +
>>> >> >> +static TCGv cpu_skip;
>>> >> >> +
>>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>>> >> >> +};
>>> >> >> +#define REG(x) (cpu_r[x])
>>> >> >> +
>>> >> >> +enum {
>>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu
>>> main loop.  */
>>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable
>>> condition exit.  */
>>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition
>>> exit.  */
>>> >> >> +};
>>> >> >> +
>>> >> >> +typedef struct DisasContext DisasContext;
>>> >> >> +
>>> >> >> +/* This is the state at translation time. */
>>> >> >> +struct DisasContext {
>>> >> >> +    TranslationBlock *tb;
>>> >> >> +
>>> >> >> +    CPUAVRState *env;
>>> >> >> +    CPUState *cs;
>>> >> >> +
>>> >> >> +    target_long npc;
>>> >> >> +    uint32_t opcode;
>>> >> >> +
>>> >> >> +    /* Routine used to access memory */
>>> >> >> +    int memidx;
>>> >> >> +    int bstate;
>>> >> >> +    int singlestep;
>>> >> >> +
>>> >> >> +    TCGv skip_var0;
>>> >> >> +    TCGv skip_var1;
>>> >> >> +    TCGCond skip_cond;
>>> >> >> +    bool free_skip_var0;
>>> >> >> +};
>>> >> >> +
>>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx
>>> % 16); }
>>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx
>>> % 8); }
>>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx
>>> % 4) * 2; }
>>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16)
>>> * 2; }
>>> >> >> +
>>> >> >> +static uint16_t next_word(DisasContext *ctx)
>>> >> >> +{
>>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>>> >> >> +}
>>> >> >> +
>>> >> >> +static int append_16(DisasContext *ctx, int x)
>>> >> >> +{
>>> >> >> +    return x << 16 | next_word(ctx);
>>> >> >> +}
>>> >> >> +
>>> >> >> +
>>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>>> >> >> +{
>>> >> >> +    if (!avr_feature(ctx->env, feature)) {
>>> >> >> +        gen_helper_unsupported(cpu_env);
>>> >> >> +        ctx->bstate = DISAS_NORETURN;
>>> >> >> +        return false;
>>> >> >> +    }
>>> >> >> +    return true;
>>> >> >> +}
>>> >> >> +
>>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>>> >> >> +#include "decode_insn.inc.c"
>>> >> >> +
>>> >> >> --
>>> >> >> 2.17.2 (Apple Git-113)
>>> >> >>
>>> >>
>>> >>
>>> >> --
>>> >> Best Regards,
>>> >> Michael Rolnik
>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Michael Rolnik
>>>
>>
>
> --
> Best Regards,
> Michael Rolnik
>
Michael Rolnik Oct. 18, 2019, 1:56 p.m. UTC | #9 
On Fri, Oct 18, 2019 at 4:23 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
>
>
>
> On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>
>>
>>
>> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote:
>>>
>>>
>>>
>>> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>>>
>>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>>>> <aleksandar.m.mail@gmail.com> wrote:
>>>> >>
>>>> >>
>>>> >> >> +static TCGv cpu_Cf;
>>>> >> >> +static TCGv cpu_Zf;
>>>> >> >> +static TCGv cpu_Nf;
>>>> >> >> +static TCGv cpu_Vf;
>>>> >> >> +static TCGv cpu_Sf;
>>>> >> >> +static TCGv cpu_Hf;
>>>> >> >> +static TCGv cpu_Tf;
>>>> >> >> +static TCGv cpu_If;
>>>> >> >> +
>>>> >> >
>>>> >> >
>>>> >> > Hello, Michael,
>>>> >> >
>>>> >> > Is there any particular reason or motivation beyond modelling status register flags as TCGv variables?
>>>> >>
>>>> >>
>>>> >>
>>>> >> I think it's easier this way as I don't need to convert flag values to
>>>> >> bits or bits to flag values.
>>>> >
>>>> >
>>>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv variable and vice versa? In other words, what value or values (out of 2^32 vales) of a TCGv variable mean the flag is 1? And the same question for 0.
>>>> >
>>>> > Is 0110000111000010100 one or zero?
>>>> >
>>>> > Besides, in such arrangement, how do you display the 8-bit status register in gdb, if at all?
>>>>
>>>> each flag register is either 0 or 1,....
>>>>
>>>>
>>>>
>>>
>>> Michael,
>>>
>>> If this is true, why is there a special handling of two flags in the following code:
>>>
>>>
>>> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
>>> {
>>> uint8_t sreg;
>>> sreg = (env->sregC & 0x01) << 0
>>> | (env->sregZ == 0 ? 1 : 0) << 1
>>> | (env->sregN) << 2
>>> | (env->sregV) << 3
>>> | (env->sregS) << 4
>>> | (env->sregH) << 5
>>> | (env->sregT) << 6
>>> | (env->sregI) << 7;
>>> return sreg;
>>> }
>>> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
>>> {
>>> env->sregC = (sreg >> 0) & 0x01;
>>> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
>>> env->sregN = (sreg >> 2) & 0x01;
>>> env->sregV = (sreg >> 3) & 0x01;
>>> env->sregS = (sreg >> 4) & 0x01;
>>> env->sregH = (sreg >> 5) & 0x01;
>>> env->sregT = (sreg >> 6) & 0x01;
>>> env->sregI = (sreg >> 7) & 0x01;
>>> }
>>>  ?
>>>
>> Aleksandar,
>>
>> If I understand your question correctly cpu_get_sreg assembles SREG value to be presented by GDB, and cpu_set_sreg sets flags values when GDB modifies SREG.
>>
>> Michael
>
>
>
> Why is handling of sregC and sregZ flags different than handling of other flags? This contradicts your previos statement that 1 (in TCGv) means 1 (flag), and 0 (in TCGv) means 0 (flag)?
>
>
> Whatever is the explanation, ot should be included, in my opinion, in code comments.
>
> Please, Michael, let's first clarify the issue from the question above.
>
> Thanks, Aleksandar
>
>
there is a comment here
https://github.com/michaelrolnik/qemu-avr/blob/master/target/avr/cpu.h#L122-L129
>
>
>
>>>
>>> Thanks,
>>> A.
>>>>
>>>>
>>>>  they are calculated here
>>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L146-L148
>>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L166
>>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L185-L187
>>>> 4. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L205
>>>> 5. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L214-L215
>>>> 6. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L222-L223
>>>> The COU itself never uses SREG at all, only the flags.
>>>>
>>>> As for the GDB it's get assembled/disassembled here
>>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/cpu.h#L219-L243
>>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L35-L37
>>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L66-L68
>>>>
>>>> >
>>>> > A.
>>>> >
>>>> >>
>>>> >> >
>>>> >> > A.
>>>> >> >
>>>> >> >
>>>> >> >
>>>> >> >>
>>>> >> >> +static TCGv cpu_rampD;
>>>> >> >> +static TCGv cpu_rampX;
>>>> >> >> +static TCGv cpu_rampY;
>>>> >> >> +static TCGv cpu_rampZ;
>>>> >> >> +
>>>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>>>> >> >> +static TCGv cpu_eind;
>>>> >> >> +static TCGv cpu_sp;
>>>> >> >> +
>>>> >> >> +static TCGv cpu_skip;
>>>> >> >> +
>>>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>>>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>>>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>>>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>>>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>>>> >> >> +};
>>>> >> >> +#define REG(x) (cpu_r[x])
>>>> >> >> +
>>>> >> >> +enum {
>>>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main loop.  */
>>>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition exit.  */
>>>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.  */
>>>> >> >> +};
>>>> >> >> +
>>>> >> >> +typedef struct DisasContext DisasContext;
>>>> >> >> +
>>>> >> >> +/* This is the state at translation time. */
>>>> >> >> +struct DisasContext {
>>>> >> >> +    TranslationBlock *tb;
>>>> >> >> +
>>>> >> >> +    CPUAVRState *env;
>>>> >> >> +    CPUState *cs;
>>>> >> >> +
>>>> >> >> +    target_long npc;
>>>> >> >> +    uint32_t opcode;
>>>> >> >> +
>>>> >> >> +    /* Routine used to access memory */
>>>> >> >> +    int memidx;
>>>> >> >> +    int bstate;
>>>> >> >> +    int singlestep;
>>>> >> >> +
>>>> >> >> +    TCGv skip_var0;
>>>> >> >> +    TCGv skip_var1;
>>>> >> >> +    TCGCond skip_cond;
>>>> >> >> +    bool free_skip_var0;
>>>> >> >> +};
>>>> >> >> +
>>>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
>>>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
>>>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) * 2; }
>>>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
>>>> >> >> +
>>>> >> >> +static uint16_t next_word(DisasContext *ctx)
>>>> >> >> +{
>>>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>>>> >> >> +}
>>>> >> >> +
>>>> >> >> +static int append_16(DisasContext *ctx, int x)
>>>> >> >> +{
>>>> >> >> +    return x << 16 | next_word(ctx);
>>>> >> >> +}
>>>> >> >> +
>>>> >> >> +
>>>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>>>> >> >> +{
>>>> >> >> +    if (!avr_feature(ctx->env, feature)) {
>>>> >> >> +        gen_helper_unsupported(cpu_env);
>>>> >> >> +        ctx->bstate = DISAS_NORETURN;
>>>> >> >> +        return false;
>>>> >> >> +    }
>>>> >> >> +    return true;
>>>> >> >> +}
>>>> >> >> +
>>>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>>>> >> >> +#include "decode_insn.inc.c"
>>>> >> >> +
>>>> >> >> --
>>>> >> >> 2.17.2 (Apple Git-113)
>>>> >> >>
>>>> >>
>>>> >>
>>>> >> --
>>>> >> Best Regards,
>>>> >> Michael Rolnik
>>>>
>>>>
>>>>
>>>> --
>>>> Best Regards,
>>>> Michael Rolnik
>>
>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
Aleksandar Markovic Oct. 18, 2019, 4:51 p.m. UTC | #10 
On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:

> On Fri, Oct 18, 2019 at 4:23 PM Aleksandar Markovic
> <aleksandar.m.mail@gmail.com> wrote:
> >
> >
> >
> > On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
> >>
> >>
> >>
> >> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <
> aleksandar.m.mail@gmail.com> wrote:
> >>>
> >>>
> >>>
> >>> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com>
> wrote:
> >>>>
> >>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
> >>>> <aleksandar.m.mail@gmail.com> wrote:
> >>>> >>
> >>>> >>
> >>>> >> >> +static TCGv cpu_Cf;
> >>>> >> >> +static TCGv cpu_Zf;
> >>>> >> >> +static TCGv cpu_Nf;
> >>>> >> >> +static TCGv cpu_Vf;
> >>>> >> >> +static TCGv cpu_Sf;
> >>>> >> >> +static TCGv cpu_Hf;
> >>>> >> >> +static TCGv cpu_Tf;
> >>>> >> >> +static TCGv cpu_If;
> >>>> >> >> +
> >>>> >> >
> >>>> >> >
> >>>> >> > Hello, Michael,
> >>>> >> >
> >>>> >> > Is there any particular reason or motivation beyond modelling
> status register flags as TCGv variables?
> >>>> >>
> >>>> >>
> >>>> >>
> >>>> >> I think it's easier this way as I don't need to convert flag
> values to
> >>>> >> bits or bits to flag values.
> >>>> >
> >>>> >
> >>>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv
> variable and vice versa? In other words, what value or values (out of 2^32
> vales) of a TCGv variable mean the flag is 1? And the same question for 0.
> >>>> >
> >>>> > Is 0110000111000010100 one or zero?
> >>>> >
> >>>> > Besides, in such arrangement, how do you display the 8-bit status
> register in gdb, if at all?
> >>>>
> >>>> each flag register is either 0 or 1,....
> >>>>
> >>>>
> >>>>
> >>>
> >>> Michael,
> >>>
> >>> If this is true, why is there a special handling of two flags in the
> following code:
> >>>
> >>>
> >>> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
> >>> {
> >>> uint8_t sreg;
> >>> sreg = (env->sregC & 0x01) << 0
> >>> | (env->sregZ == 0 ? 1 : 0) << 1
> >>> | (env->sregN) << 2
> >>> | (env->sregV) << 3
> >>> | (env->sregS) << 4
> >>> | (env->sregH) << 5
> >>> | (env->sregT) << 6
> >>> | (env->sregI) << 7;
> >>> return sreg;
> >>> }
> >>> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
> >>> {
> >>> env->sregC = (sreg >> 0) & 0x01;
> >>> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
> >>> env->sregN = (sreg >> 2) & 0x01;
> >>> env->sregV = (sreg >> 3) & 0x01;
> >>> env->sregS = (sreg >> 4) & 0x01;
> >>> env->sregH = (sreg >> 5) & 0x01;
> >>> env->sregT = (sreg >> 6) & 0x01;
> >>> env->sregI = (sreg >> 7) & 0x01;
> >>> }
> >>>  ?
> >>>
> >> Aleksandar,
> >>
> >> If I understand your question correctly cpu_get_sreg assembles SREG
> value to be presented by GDB, and cpu_set_sreg sets flags values when GDB
> modifies SREG.
> >>
> >> Michael
> >
> >
> >
> >

Why is handling of sregC and sregZ flags different than handling of other
> flags? This contradicts your previos statement that 1 (in TCGv) means 1
> (flag), and 0 (in TCGv) means 0 (flag)?
> >
> >
> > Whatever is the explanation, ot should be included, in my opinion, in
> code comments.
> >
> > Please, Michael, let's first clarify the issue from the question above.
> >
> > Thanks, Aleksandar
> >
> >
> there is a comment here
> https://github.com/michaelrolnik/qemu-avr/blob/
> master/target/avr/cpu.h#L122-L129
> >



...but it does explain WHY of my question.

The reason I insist on your explanation is that when we model a cpu or a
device in QEMU, a goal is that the model is as close to the hardware as
possible. One may not, for pletora of reasons, succeed in reaching that
goal, or, I can imagine, on purpose depart from that goal for some reason -
perhaps that was the case in your implementation, where you modelled a
single 8-bit status register with 8 TCGv variables.

But, even that way of modelling was done inconsistently across bits of the
status register. In that light, I want to know the justification for that,
so repeat my question: Why is handling of sregC and sregZ flags different
than handling of other flags in functions cpu_get_sreg()
and cpu_get_sreg()? This was not explained in any comment or commit
message. And is in contradiction with one of your previous answers.

Yours,
Aleksandar


> >
> >
> >>>
> >>> Thanks,
> >>> A.
> >>>>
> >>>>
> >>>>  they are calculated here
> >>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L146-L148
> >>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L166
> >>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L185-L187
> >>>> 4. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L205
> >>>> 5. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L214-L215
> >>>> 6. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/translate.c#L222-L223
> >>>> The COU itself never uses SREG at all, only the flags.
> >>>>
> >>>> As for the GDB it's get assembled/disassembled here
> >>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/cpu.h#L219-L243
> >>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/gdbstub.c#L35-L37
> >>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/
> avr-v32/target/avr/gdbstub.c#L66-L68
> >>>>
> >>>> >
> >>>> > A.
> >>>> >
> >>>> >>
> >>>> >> >
> >>>> >> > A.
> >>>> >> >
> >>>> >> >
> >>>> >> >
> >>>> >> >>
> >>>> >> >> +static TCGv cpu_rampD;
> >>>> >> >> +static TCGv cpu_rampX;
> >>>> >> >> +static TCGv cpu_rampY;
> >>>> >> >> +static TCGv cpu_rampZ;
> >>>> >> >> +
> >>>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
> >>>> >> >> +static TCGv cpu_eind;
> >>>> >> >> +static TCGv cpu_sp;
> >>>> >> >> +
> >>>> >> >> +static TCGv cpu_skip;
> >>>> >> >> +
> >>>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
> >>>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
> >>>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
> >>>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
> >>>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
> >>>> >> >> +};
> >>>> >> >> +#define REG(x) (cpu_r[x])
> >>>> >> >> +
> >>>> >> >> +enum {
> >>>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the
> cpu main loop.  */
> >>>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable
> condition exit.  */
> >>>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single
> condition exit.  */
> >>>> >> >> +};
> >>>> >> >> +
> >>>> >> >> +typedef struct DisasContext DisasContext;
> >>>> >> >> +
> >>>> >> >> +/* This is the state at translation time. */
> >>>> >> >> +struct DisasContext {
> >>>> >> >> +    TranslationBlock *tb;
> >>>> >> >> +
> >>>> >> >> +    CPUAVRState *env;
> >>>> >> >> +    CPUState *cs;
> >>>> >> >> +
> >>>> >> >> +    target_long npc;
> >>>> >> >> +    uint32_t opcode;
> >>>> >> >> +
> >>>> >> >> +    /* Routine used to access memory */
> >>>> >> >> +    int memidx;
> >>>> >> >> +    int bstate;
> >>>> >> >> +    int singlestep;
> >>>> >> >> +
> >>>> >> >> +    TCGv skip_var0;
> >>>> >> >> +    TCGv skip_var1;
> >>>> >> >> +    TCGCond skip_cond;
> >>>> >> >> +    bool free_skip_var0;
> >>>> >> >> +};
> >>>> >> >> +
> >>>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 +
> (indx % 16); }
> >>>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 +
> (indx % 8); }
> >>>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 +
> (indx % 4) * 2; }
> >>>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx %
> 16) * 2; }
> >>>> >> >> +
> >>>> >> >> +static uint16_t next_word(DisasContext *ctx)
> >>>> >> >> +{
> >>>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
> >>>> >> >> +}
> >>>> >> >> +
> >>>> >> >> +static int append_16(DisasContext *ctx, int x)
> >>>> >> >> +{
> >>>> >> >> +    return x << 16 | next_word(ctx);
> >>>> >> >> +}
> >>>> >> >> +
> >>>> >> >> +
> >>>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
> >>>> >> >> +{
> >>>> >> >> +    if (!avr_feature(ctx->env, feature)) {
> >>>> >> >> +        gen_helper_unsupported(cpu_env);
> >>>> >> >> +        ctx->bstate = DISAS_NORETURN;
> >>>> >> >> +        return false;
> >>>> >> >> +    }
> >>>> >> >> +    return true;
> >>>> >> >> +}
> >>>> >> >> +
> >>>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
> >>>> >> >> +#include "decode_insn.inc.c"
> >>>> >> >> +
> >>>> >> >> --
> >>>> >> >> 2.17.2 (Apple Git-113)
> >>>> >> >>
> >>>> >>
> >>>> >>
> >>>> >> --
> >>>> >> Best Regards,
> >>>> >> Michael Rolnik
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Best Regards,
> >>>> Michael Rolnik
> >>
> >>
> >>
> >> --
> >> Best Regards,
> >> Michael Rolnik
>
>
>
> --
> Best Regards,
> Michael Rolnik
>
Aleksandar Markovic Oct. 18, 2019, 6:08 p.m. UTC | #11 
On Friday, October 18, 2019, Aleksandar Markovic <
aleksandar.m.mail@gmail.com> wrote:

>
>
> On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>
>> On Fri, Oct 18, 2019 at 4:23 PM Aleksandar Markovic
>> <aleksandar.m.mail@gmail.com> wrote:
>> >
>> >
>> >
>> > On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>> >>
>> >>
>> >>
>> >> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <
>> aleksandar.m.mail@gmail.com> wrote:
>> >>>
>> >>>
>> >>>
>> >>> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com>
>> wrote:
>> >>>>
>> >>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>> >>>> <aleksandar.m.mail@gmail.com> wrote:
>> >>>> >>
>> >>>> >>
>> >>>> >> >> +static TCGv cpu_Cf;
>> >>>> >> >> +static TCGv cpu_Zf;
>> >>>> >> >> +static TCGv cpu_Nf;
>> >>>> >> >> +static TCGv cpu_Vf;
>> >>>> >> >> +static TCGv cpu_Sf;
>> >>>> >> >> +static TCGv cpu_Hf;
>> >>>> >> >> +static TCGv cpu_Tf;
>> >>>> >> >> +static TCGv cpu_If;
>> >>>> >> >> +
>> >>>> >> >
>> >>>> >> >
>> >>>> >> > Hello, Michael,
>> >>>> >> >
>> >>>> >> > Is there any particular reason or motivation beyond modelling
>> status register flags as TCGv variables?
>> >>>> >>
>> >>>> >>
>> >>>> >>
>> >>>> >> I think it's easier this way as I don't need to convert flag
>> values to
>> >>>> >> bits or bits to flag values.
>> >>>> >
>> >>>> >
>> >>>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv
>> variable and vice versa? In other words, what value or values (out of 2^32
>> vales) of a TCGv variable mean the flag is 1? And the same question for 0.
>> >>>> >
>> >>>> > Is 0110000111000010100 one or zero?
>> >>>> >
>> >>>> > Besides, in such arrangement, how do you display the 8-bit status
>> register in gdb, if at all?
>> >>>>
>> >>>> each flag register is either 0 or 1,....
>> >>>>
>> >>>>
>> >>>>
>> >>>
>> >>> Michael,
>> >>>
>> >>> If this is true, why is there a special handling of two flags in the
>> following code:
>> >>>
>> >>>
>> >>> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
>> >>> {
>> >>> uint8_t sreg;
>> >>> sreg = (env->sregC & 0x01) << 0
>> >>> | (env->sregZ == 0 ? 1 : 0) << 1
>> >>> | (env->sregN) << 2
>> >>> | (env->sregV) << 3
>> >>> | (env->sregS) << 4
>> >>> | (env->sregH) << 5
>> >>> | (env->sregT) << 6
>> >>> | (env->sregI) << 7;
>> >>> return sreg;
>> >>> }
>> >>> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
>> >>> {
>> >>> env->sregC = (sreg >> 0) & 0x01;
>> >>> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
>> >>> env->sregN = (sreg >> 2) & 0x01;
>> >>> env->sregV = (sreg >> 3) & 0x01;
>> >>> env->sregS = (sreg >> 4) & 0x01;
>> >>> env->sregH = (sreg >> 5) & 0x01;
>> >>> env->sregT = (sreg >> 6) & 0x01;
>> >>> env->sregI = (sreg >> 7) & 0x01;
>> >>> }
>> >>>  ?
>> >>>
>> >> Aleksandar,
>> >>
>> >> If I understand your question correctly cpu_get_sreg assembles SREG
>> value to be presented by GDB, and cpu_set_sreg sets flags values when GDB
>> modifies SREG.
>> >>
>> >> Michael
>> >
>> >
>> >
>> >
>
> Why is handling of sregC and sregZ flags different than handling of other
>> flags? This contradicts your previos statement that 1 (in TCGv) means 1
>> (flag), and 0 (in TCGv) means 0 (flag)?
>> >
>> >
>> > Whatever is the explanation, ot should be included, in my opinion, in
>> code comments.
>> >
>> > Please, Michael, let's first clarify the issue from the question above.
>> >
>> > Thanks, Aleksandar
>> >
>> >
>> there is a comment here
>> https://github.com/michaelrolnik/qemu-avr/blob/master/
>> target/avr/cpu.h#L122-L129
>> >
>
>
>
> ...but it does explain WHY of my question.
>

I meant to say  "does not", not "does".

Michael, don't be discouraged by lenghty review process, just be persistent
and available for communication with reviewers.

Sincerely,
Aleksandar



>
> The reason I insist on your explanation is that when we model a cpu or a
> device in QEMU, a goal is that the model is as close to the hardware as
> possible. One may not, for pletora of reasons, succeed in reaching that
> goal, or, I can imagine, on purpose depart from that goal for some reason -
> perhaps that was the case in your implementation, where you modelled a
> single 8-bit status register with 8 TCGv variables.
>
> But, even that way of modelling was done inconsistently across bits of the
> status register. In that light, I want to know the justification for that,
> so repeat my question: Why is handling of sregC and sregZ flags different
> than handling of other flags in functions cpu_get_sreg()
> and cpu_get_sreg()? This was not explained in any comment or commit
> message. And is in contradiction with one of your previous answers.
>
> Yours,
> Aleksandar
>
>
>> >
>> >
>> >>>
>> >>> Thanks,
>> >>> A.
>> >>>>
>> >>>>
>> >>>>  they are calculated here
>> >>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/translate.c#L146-L148
>> >>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/translate.c#L166
>> >>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/translate.c#L185-L187
>> >>>> 4. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/translate.c#L205
>> >>>> 5. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/translate.c#L214-L215
>> >>>> 6. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/translate.c#L222-L223
>> >>>> The COU itself never uses SREG at all, only the flags.
>> >>>>
>> >>>> As for the GDB it's get assembled/disassembled here
>> >>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/cpu.h#L219-L243
>> >>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/gdbstub.c#L35-L37
>> >>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/
>> target/avr/gdbstub.c#L66-L68
>> >>>>
>> >>>> >
>> >>>> > A.
>> >>>> >
>> >>>> >>
>> >>>> >> >
>> >>>> >> > A.
>> >>>> >> >
>> >>>> >> >
>> >>>> >> >
>> >>>> >> >>
>> >>>> >> >> +static TCGv cpu_rampD;
>> >>>> >> >> +static TCGv cpu_rampX;
>> >>>> >> >> +static TCGv cpu_rampY;
>> >>>> >> >> +static TCGv cpu_rampZ;
>> >>>> >> >> +
>> >>>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>> >>>> >> >> +static TCGv cpu_eind;
>> >>>> >> >> +static TCGv cpu_sp;
>> >>>> >> >> +
>> >>>> >> >> +static TCGv cpu_skip;
>> >>>> >> >> +
>> >>>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>> >>>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>> >>>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>> >>>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>> >>>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>> >>>> >> >> +};
>> >>>> >> >> +#define REG(x) (cpu_r[x])
>> >>>> >> >> +
>> >>>> >> >> +enum {
>> >>>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the
>> cpu main loop.  */
>> >>>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable
>> condition exit.  */
>> >>>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single
>> condition exit.  */
>> >>>> >> >> +};
>> >>>> >> >> +
>> >>>> >> >> +typedef struct DisasContext DisasContext;
>> >>>> >> >> +
>> >>>> >> >> +/* This is the state at translation time. */
>> >>>> >> >> +struct DisasContext {
>> >>>> >> >> +    TranslationBlock *tb;
>> >>>> >> >> +
>> >>>> >> >> +    CPUAVRState *env;
>> >>>> >> >> +    CPUState *cs;
>> >>>> >> >> +
>> >>>> >> >> +    target_long npc;
>> >>>> >> >> +    uint32_t opcode;
>> >>>> >> >> +
>> >>>> >> >> +    /* Routine used to access memory */
>> >>>> >> >> +    int memidx;
>> >>>> >> >> +    int bstate;
>> >>>> >> >> +    int singlestep;
>> >>>> >> >> +
>> >>>> >> >> +    TCGv skip_var0;
>> >>>> >> >> +    TCGv skip_var1;
>> >>>> >> >> +    TCGCond skip_cond;
>> >>>> >> >> +    bool free_skip_var0;
>> >>>> >> >> +};
>> >>>> >> >> +
>> >>>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 +
>> (indx % 16); }
>> >>>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 +
>> (indx % 8); }
>> >>>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 +
>> (indx % 4) * 2; }
>> >>>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx %
>> 16) * 2; }
>> >>>> >> >> +
>> >>>> >> >> +static uint16_t next_word(DisasContext *ctx)
>> >>>> >> >> +{
>> >>>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>> >>>> >> >> +}
>> >>>> >> >> +
>> >>>> >> >> +static int append_16(DisasContext *ctx, int x)
>> >>>> >> >> +{
>> >>>> >> >> +    return x << 16 | next_word(ctx);
>> >>>> >> >> +}
>> >>>> >> >> +
>> >>>> >> >> +
>> >>>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>> >>>> >> >> +{
>> >>>> >> >> +    if (!avr_feature(ctx->env, feature)) {
>> >>>> >> >> +        gen_helper_unsupported(cpu_env);
>> >>>> >> >> +        ctx->bstate = DISAS_NORETURN;
>> >>>> >> >> +        return false;
>> >>>> >> >> +    }
>> >>>> >> >> +    return true;
>> >>>> >> >> +}
>> >>>> >> >> +
>> >>>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>> >>>> >> >> +#include "decode_insn.inc.c"
>> >>>> >> >> +
>> >>>> >> >> --
>> >>>> >> >> 2.17.2 (Apple Git-113)
>> >>>> >> >>
>> >>>> >>
>> >>>> >>
>> >>>> >> --
>> >>>> >> Best Regards,
>> >>>> >> Michael Rolnik
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Best Regards,
>> >>>> Michael Rolnik
>> >>
>> >>
>> >>
>> >> --
>> >> Best Regards,
>> >> Michael Rolnik
>>
>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
>>
>
Michael Rolnik Oct. 19, 2019, 8:18 p.m. UTC | #12 
On Fri, Oct 18, 2019 at 9:08 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
>
>
>
> On Friday, October 18, 2019, Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote:
>>
>>
>>
>> On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>>
>>> On Fri, Oct 18, 2019 at 4:23 PM Aleksandar Markovic
>>> <aleksandar.m.mail@gmail.com> wrote:
>>> >
>>> >
>>> >
>>> > On Friday, October 18, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>> >>
>>> >>
>>> >>
>>> >> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote:
>>> >>>
>>> >>>
>>> >>>
>>> >>> On Thursday, October 17, 2019, Michael Rolnik <mrolnik@gmail.com> wrote:
>>> >>>>
>>> >>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>>> >>>> <aleksandar.m.mail@gmail.com> wrote:
>>> >>>> >>
>>> >>>> >>
>>> >>>> >> >> +static TCGv cpu_Cf;
>>> >>>> >> >> +static TCGv cpu_Zf;
>>> >>>> >> >> +static TCGv cpu_Nf;
>>> >>>> >> >> +static TCGv cpu_Vf;
>>> >>>> >> >> +static TCGv cpu_Sf;
>>> >>>> >> >> +static TCGv cpu_Hf;
>>> >>>> >> >> +static TCGv cpu_Tf;
>>> >>>> >> >> +static TCGv cpu_If;
>>> >>>> >> >> +
>>> >>>> >> >
>>> >>>> >> >
>>> >>>> >> > Hello, Michael,
>>> >>>> >> >
>>> >>>> >> > Is there any particular reason or motivation beyond modelling status register flags as TCGv variables?
>>> >>>> >>
>>> >>>> >>
>>> >>>> >>
>>> >>>> >> I think it's easier this way as I don't need to convert flag values to
>>> >>>> >> bits or bits to flag values.
>>> >>>> >
>>> >>>> >
>>> >>>> > Ok. But, how do you map 0/1 flag value to the value of a TCGv variable and vice versa? In other words, what value or values (out of 2^32 vales) of a TCGv variable mean the flag is 1? And the same question for 0.
>>> >>>> >
>>> >>>> > Is 0110000111000010100 one or zero?
>>> >>>> >
>>> >>>> > Besides, in such arrangement, how do you display the 8-bit status register in gdb, if at all?
>>> >>>>
>>> >>>> each flag register is either 0 or 1,....
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>
>>> >>> Michael,
>>> >>>
>>> >>> If this is true, why is there a special handling of two flags in the following code:
>>> >>>
>>> >>>
>>> >>> static inline uint8_t cpu_get_sreg(CPUAVRState *env)
>>> >>> {
>>> >>> uint8_t sreg;
>>> >>> sreg = (env->sregC & 0x01) << 0
>>> >>> | (env->sregZ == 0 ? 1 : 0) << 1
>>> >>> | (env->sregN) << 2
>>> >>> | (env->sregV) << 3
>>> >>> | (env->sregS) << 4
>>> >>> | (env->sregH) << 5
>>> >>> | (env->sregT) << 6
>>> >>> | (env->sregI) << 7;
>>> >>> return sreg;
>>> >>> }
>>> >>> static inline void cpu_set_sreg(CPUAVRState *env, uint8_t sreg)
>>> >>> {
>>> >>> env->sregC = (sreg >> 0) & 0x01;
>>> >>> env->sregZ = (sreg >> 1) & 0x01 ? 0 : 1;
>>> >>> env->sregN = (sreg >> 2) & 0x01;
>>> >>> env->sregV = (sreg >> 3) & 0x01;
>>> >>> env->sregS = (sreg >> 4) & 0x01;
>>> >>> env->sregH = (sreg >> 5) & 0x01;
>>> >>> env->sregT = (sreg >> 6) & 0x01;
>>> >>> env->sregI = (sreg >> 7) & 0x01;
>>> >>> }
>>> >>>  ?
>>> >>>
>>> >> Aleksandar,
>>> >>
>>> >> If I understand your question correctly cpu_get_sreg assembles SREG value to be presented by GDB, and cpu_set_sreg sets flags values when GDB modifies SREG.
>>> >>
>>> >> Michael
>>> >
>>> >
>>> >
>>> >
>>>
>>> Why is handling of sregC and sregZ flags different than handling of other flags? This contradicts your previos statement that 1 (in TCGv) means 1 (flag), and 0 (in TCGv) means 0 (flag)?
>>> >
>>> >
>>> > Whatever is the explanation, ot should be included, in my opinion, in code comments.
>>> >
>>> > Please, Michael, let's first clarify the issue from the question above.
>>> >
>>> > Thanks, Aleksandar
>>> >
>>> >
>>> there is a comment here
>>> https://github.com/michaelrolnik/qemu-avr/blob/master/target/avr/cpu.h#L122-L129
>>> >
>>
>>
>>
>> ...but it does explain WHY of my question.
>
>
> I meant to say  "does not", not "does".
>
> Michael, don't be discouraged by lenghty review process, just be persistent and available for communication with reviewers.
>
> Sincerely,
> Aleksandar
>
>

Aleksandar,

I will try to explain my reasoning for the current implementation.
1. Many (or even the majority) of instructions modify flags, i.e. the
flags value should be computed regardless whether they are used or
not.
2. SREG as a whole is almost never used except
    a. GDB
    b. IN, OUT, SBI, CBI, SBIC, SBIS instructions as 1 out of 8
possible registers.

So, as we can see flags are calculated more times then they are used.
This leads us to two following conclusions
1. don't maintain SREG as one register but as a set of 8 different registers
2. try to minimize number of calculations for these flags

All flags except Z (zero) are calculated fully and kept as one bit
Z just holds the result of last computation, so Z flag is set when
sregZ register is 0 otherwise the flag is not set.
that's why there is difference between Z and others.

so, you are right, there is no need to treat C differently. I will
send an update later.

Thanks.

Michael Rolnik




>>
>>
>> The reason I insist on your explanation is that when we model a cpu or a device in QEMU, a goal is that the model is as close to the hardware as possible. One may not, for pletora of reasons, succeed in reaching that goal, or, I can imagine, on purpose depart from that goal for some reason - perhaps that was the case in your implementation, where you modelled a single 8-bit status register with 8 TCGv variables.
>>
>> But, even that way of modelling was done inconsistently across bits of the status register. In that light, I want to know the justification for that, so repeat my question: Why is handling of sregC and sregZ flags different than handling of other flags in functions cpu_get_sreg() and cpu_get_sreg()? This was not explained in any comment or commit message. And is in contradiction with one of your previous answers.
>>
>> Yours,
>> Aleksandar
>>
>>>
>>> >
>>> >
>>> >>>
>>> >>> Thanks,
>>> >>> A.
>>> >>>>
>>> >>>>
>>> >>>>  they are calculated here
>>> >>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L146-L148
>>> >>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L166
>>> >>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L185-L187
>>> >>>> 4. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L205
>>> >>>> 5. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L214-L215
>>> >>>> 6. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/translate.c#L222-L223
>>> >>>> The COU itself never uses SREG at all, only the flags.
>>> >>>>
>>> >>>> As for the GDB it's get assembled/disassembled here
>>> >>>> 1. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/cpu.h#L219-L243
>>> >>>> 2. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L35-L37
>>> >>>> 3. https://github.com/michaelrolnik/qemu-avr/blob/avr-v32/target/avr/gdbstub.c#L66-L68
>>> >>>>
>>> >>>> >
>>> >>>> > A.
>>> >>>> >
>>> >>>> >>
>>> >>>> >> >
>>> >>>> >> > A.
>>> >>>> >> >
>>> >>>> >> >
>>> >>>> >> >
>>> >>>> >> >>
>>> >>>> >> >> +static TCGv cpu_rampD;
>>> >>>> >> >> +static TCGv cpu_rampX;
>>> >>>> >> >> +static TCGv cpu_rampY;
>>> >>>> >> >> +static TCGv cpu_rampZ;
>>> >>>> >> >> +
>>> >>>> >> >> +static TCGv cpu_r[NO_CPU_REGISTERS];
>>> >>>> >> >> +static TCGv cpu_eind;
>>> >>>> >> >> +static TCGv cpu_sp;
>>> >>>> >> >> +
>>> >>>> >> >> +static TCGv cpu_skip;
>>> >>>> >> >> +
>>> >>>> >> >> +static const char reg_names[NO_CPU_REGISTERS][8] = {
>>> >>>> >> >> +    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
>>> >>>> >> >> +    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
>>> >>>> >> >> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>>> >>>> >> >> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>>> >>>> >> >> +};
>>> >>>> >> >> +#define REG(x) (cpu_r[x])
>>> >>>> >> >> +
>>> >>>> >> >> +enum {
>>> >>>> >> >> +    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main loop.  */
>>> >>>> >> >> +    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition exit.  */
>>> >>>> >> >> +    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.  */
>>> >>>> >> >> +};
>>> >>>> >> >> +
>>> >>>> >> >> +typedef struct DisasContext DisasContext;
>>> >>>> >> >> +
>>> >>>> >> >> +/* This is the state at translation time. */
>>> >>>> >> >> +struct DisasContext {
>>> >>>> >> >> +    TranslationBlock *tb;
>>> >>>> >> >> +
>>> >>>> >> >> +    CPUAVRState *env;
>>> >>>> >> >> +    CPUState *cs;
>>> >>>> >> >> +
>>> >>>> >> >> +    target_long npc;
>>> >>>> >> >> +    uint32_t opcode;
>>> >>>> >> >> +
>>> >>>> >> >> +    /* Routine used to access memory */
>>> >>>> >> >> +    int memidx;
>>> >>>> >> >> +    int bstate;
>>> >>>> >> >> +    int singlestep;
>>> >>>> >> >> +
>>> >>>> >> >> +    TCGv skip_var0;
>>> >>>> >> >> +    TCGv skip_var1;
>>> >>>> >> >> +    TCGCond skip_cond;
>>> >>>> >> >> +    bool free_skip_var0;
>>> >>>> >> >> +};
>>> >>>> >> >> +
>>> >>>> >> >> +static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
>>> >>>> >> >> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
>>> >>>> >> >> +static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) * 2; }
>>> >>>> >> >> +static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
>>> >>>> >> >> +
>>> >>>> >> >> +static uint16_t next_word(DisasContext *ctx)
>>> >>>> >> >> +{
>>> >>>> >> >> +    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>>> >>>> >> >> +}
>>> >>>> >> >> +
>>> >>>> >> >> +static int append_16(DisasContext *ctx, int x)
>>> >>>> >> >> +{
>>> >>>> >> >> +    return x << 16 | next_word(ctx);
>>> >>>> >> >> +}
>>> >>>> >> >> +
>>> >>>> >> >> +
>>> >>>> >> >> +static bool avr_have_feature(DisasContext *ctx, int feature)
>>> >>>> >> >> +{
>>> >>>> >> >> +    if (!avr_feature(ctx->env, feature)) {
>>> >>>> >> >> +        gen_helper_unsupported(cpu_env);
>>> >>>> >> >> +        ctx->bstate = DISAS_NORETURN;
>>> >>>> >> >> +        return false;
>>> >>>> >> >> +    }
>>> >>>> >> >> +    return true;
>>> >>>> >> >> +}
>>> >>>> >> >> +
>>> >>>> >> >> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>>> >>>> >> >> +#include "decode_insn.inc.c"
>>> >>>> >> >> +
>>> >>>> >> >> --
>>> >>>> >> >> 2.17.2 (Apple Git-113)
>>> >>>> >> >>
>>> >>>> >>
>>> >>>> >>
>>> >>>> >> --
>>> >>>> >> Best Regards,
>>> >>>> >> Michael Rolnik
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> --
>>> >>>> Best Regards,
>>> >>>> Michael Rolnik
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >> Best Regards,
>>> >> Michael Rolnik
>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Michael Rolnik



--
Best Regards,
Michael Rolnik
diff mbox series

Patch

diff --git a/target/avr/translate.c b/target/avr/translate.c
new file mode 100644
index 0000000000..53c9892a60
--- /dev/null
+++ b/target/avr/translate.c
@@ -0,0 +1,132 @@ 
+/*
+ * QEMU AVR CPU
+ *
+ * Copyright (c) 2019 Michael Rolnik
+ *
+ * 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.1 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/lgpl-2.1.html>
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/qemu-print.h"
+#include "tcg/tcg.h"
+#include "cpu.h"
+#include "exec/exec-all.h"
+#include "disas/disas.h"
+#include "tcg-op.h"
+#include "exec/cpu_ldst.h"
+#include "exec/helper-proto.h"
+#include "exec/helper-gen.h"
+#include "exec/log.h"
+#include "exec/gdbstub.h"
+#include "exec/translator.h"
+#include "exec/gen-icount.h"
+
+/*
+ *  Define if you want a BREAK instruction translated to a breakpoint
+ *  Active debugging connection is assumed
+ *  This is for
+ *  https://github.com/seharris/qemu-avr-tests/tree/master/instruction-tests
+ *  tests
+ */
+#undef BREAKPOINT_ON_BREAK
+
+static TCGv cpu_pc;
+
+static TCGv cpu_Cf;
+static TCGv cpu_Zf;
+static TCGv cpu_Nf;
+static TCGv cpu_Vf;
+static TCGv cpu_Sf;
+static TCGv cpu_Hf;
+static TCGv cpu_Tf;
+static TCGv cpu_If;
+
+static TCGv cpu_rampD;
+static TCGv cpu_rampX;
+static TCGv cpu_rampY;
+static TCGv cpu_rampZ;
+
+static TCGv cpu_r[NO_CPU_REGISTERS];
+static TCGv cpu_eind;
+static TCGv cpu_sp;
+
+static TCGv cpu_skip;
+
+static const char reg_names[NO_CPU_REGISTERS][8] = {
+    "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
+    "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
+    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
+    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
+};
+#define REG(x) (cpu_r[x])
+
+enum {
+    DISAS_EXIT   = DISAS_TARGET_0,  /* We want return to the cpu main loop.  */
+    DISAS_LOOKUP = DISAS_TARGET_1,  /* We have a variable condition exit.  */
+    DISAS_CHAIN  = DISAS_TARGET_2,  /* We have a single condition exit.  */
+};
+
+typedef struct DisasContext DisasContext;
+
+/* This is the state at translation time. */
+struct DisasContext {
+    TranslationBlock *tb;
+
+    CPUAVRState *env;
+    CPUState *cs;
+
+    target_long npc;
+    uint32_t opcode;
+
+    /* Routine used to access memory */
+    int memidx;
+    int bstate;
+    int singlestep;
+
+    TCGv skip_var0;
+    TCGv skip_var1;
+    TCGCond skip_cond;
+    bool free_skip_var0;
+};
+
+static int to_A(DisasContext *ctx, int indx) { return 16 + (indx % 16); }
+static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
+static int to_C(DisasContext *ctx, int indx) { return 24 + (indx % 4) * 2; }
+static int to_D(DisasContext *ctx, int indx) { return (indx % 16) * 2; }
+
+static uint16_t next_word(DisasContext *ctx)
+{
+    return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
+}
+
+static int append_16(DisasContext *ctx, int x)
+{
+    return x << 16 | next_word(ctx);
+}
+
+
+static bool avr_have_feature(DisasContext *ctx, int feature)
+{
+    if (!avr_feature(ctx->env, feature)) {
+        gen_helper_unsupported(cpu_env);
+        ctx->bstate = DISAS_NORETURN;
+        return false;
+    }
+    return true;
+}
+
+static bool decode_insn(DisasContext *ctx, uint16_t insn);
+#include "decode_insn.inc.c"
+