Re: [Qemu-devel] [PATCH V4 03/18] target-i386: support a20 mask for NEC PC-9812

[Jamie Lokier <jamie@shareable.org>]

TAKEDA, toshiya wrote:
> @@ -940,7 +966,15 @@ void cpu_x86_set_a20(CPUX86State *env, int a20_state)
>          /* when a20 is changed, all the MMU mappings are invalid, so
>             we must flush everything */
>          tlb_flush(env, 1);
> -        env->a20_mask = ~(1 << 20) | (a20_state << 20);
> +        if (env->private_features & PRIVATE_FEATURE_PC98_A20MASK) {
> +            if (a20_state) {
> +                env->a20_mask = ~0x0;
> +            } else {
> +                env->a20_mask = 0xfffff;
> +            }
> +        } else {
> +            env->a20_mask = ~(1 << 20) | (a20_state << 20);
> +        }
>      }
>  }

It seems strange to mix different styles in that way.  How about this,
which I think makes the PC98 vs. PC difference clearer too:

    if (a20_state) {
        env->a20_mask = ~0x0;
    } else if (env->private_features & PRIVATE_FEATURE_PC98_A20MASK) {
        env->a20_mask = (1 << 20) - 1;	/* A20 masks all bits >= 20. */
    } else {
        env->a20_mask = ~(1 << 20);	/* A20 masks only bit 20. */
    }

(When I say clearer, it's not _that_ obvious that these or the
original expressions store the right thing into "uint64
env->a20_mask", given these expressions are all of type "int", but, in
fact, they do owing to C type promotion rules.  Same for the save/load
state).

-- Jamie