Re: [Qemu-devel] [PATCH 02/10] target-avr: adding AVR CPU features/flavors

[Richard Henderson <rth@twiddle.net>]

On 06/02/2016 01:06 PM, Michael Rolnik wrote:
> Signed-off-by: Michael Rolnik <mrolnik@gmail.com>
> ---
>  target-avr/cpu.c | 326 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-avr/cpu.h |  59 ++++++++++
>  2 files changed, 383 insertions(+), 2 deletions(-)
>
> diff --git a/target-avr/cpu.c b/target-avr/cpu.c
> index ff26018..9be0a1d 100644
> --- a/target-avr/cpu.c
> +++ b/target-avr/cpu.c
> @@ -31,7 +31,7 @@ static void         avr_cpu_set_pc(
>  {
>      AVRCPU   *cpu = AVR_CPU(cs);
>
> -    cpu->env.pc = value / 2;    /*  internaly PC points to words, not bytes */
> +    cpu->env.pc = value / 2;    /*  internally PC points to words   */
>  }
>
>  static bool         avr_cpu_has_work(
> @@ -52,7 +52,7 @@ static void         avr_cpu_synchronize_from_tb(
>      AVRCPU      *cpu = AVR_CPU(cs);
>      CPUAVRState *env = &cpu->env;
>
> -    env->pc     = tb->pc / 2;
> +    env->pc     = tb->pc / 2;   /*  internally PC points to words   */

Fold these fixups into the previous patch.

> @@ -61,12 +61,14 @@ static void         avr_cpu_reset(
>      AVRCPU         *cpu = AVR_CPU(s);
>      AVRCPUClass    *mcc = AVR_CPU_GET_CLASS(cpu);
>      CPUAVRState    *env = &cpu->env;
> +    uint32_t        features    = env->features;
>
>      mcc->parent_reset(s);
>
>      memset(env, 0, sizeof(CPUAVRState));
>      env->pc         = 0;
>      env->sregI      = 1;
> +    env->features   = features;

As I said re patch 1, this is fixed by only clearing to before features.

> +}
> +static void         avr_avr6_initfn(

Blank line between functions.  Many examples.

> +static inline int avr_feature(
> +                                CPUAVRState        *env,
> +                                int                 feature)
> +{
> +    return (env->features & (1UL << feature)) != 0;

features is type uint32_t; you don't need UL, just U.

> +static inline void  avr_del_feature(
> +                                CPUAVRState        *env,
> +                                int                 feature)
> +{
> +    env->features   &= ~(1Ul << feature);
> +}

When would you ever delete a feature?  Seems like this would be forever unused.


r~