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

[Michael Rolnik]

On Fri, Oct 18, 2019 at 9:08 PM Aleksandar Markovic
<address@hidden> wrote:
>
>
>
> On Friday, October 18, 2019, Aleksandar Markovic <address@hidden> wrote:
>>
>>
>>
>> On Friday, October 18, 2019, Michael Rolnik <address@hidden> wrote:
>>>
>>> On Fri, Oct 18, 2019 at 4:23 PM Aleksandar Markovic
>>> <address@hidden> wrote:
>>> >
>>> >
>>> >
>>> > On Friday, October 18, 2019, Michael Rolnik <address@hidden> wrote:
>>> >>
>>> >>
>>> >>
>>> >> On Fri, Oct 18, 2019 at 11:52 AM Aleksandar Markovic <address@hidden> 
>>> >> wrote:
>>> >>>
>>> >>>
>>> >>>
>>> >>> On Thursday, October 17, 2019, Michael Rolnik <address@hidden> wrote:
>>> >>>>
>>> >>>> On Thu, Oct 17, 2019 at 11:17 PM Aleksandar Markovic
>>> >>>> <address@hidden> 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