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

[TAKEDA, toshiya]

Dear Jamie,

Thank you very much for comment.

Jamie Lokier wrote:
>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. */
>    }

I agree.
I will repost new patch later.

>(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).

Well, I hope I dont deal with this item now in pc98 patches...

Thanks,
TAKEDA, toshiya

>
>-- Jamie
>
>
>