[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 14/28] m68k: replace cpu_m68k_init() with cpu_ge
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [PATCH 14/28] m68k: replace cpu_m68k_init() with cpu_generic_init() |
Date: |
Mon, 14 Aug 2017 20:23:10 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
Le 14/08/2017 à 10:00, Igor Mammedov a écrit :
> On Mon, 17 Jul 2017 17:23:22 +0200
> Igor Mammedov <address@hidden> wrote:
>
>> On Mon, 17 Jul 2017 17:05:15 +0200
>> Andreas Färber <address@hidden> wrote:
>>
>>> Am 17.07.2017 um 12:41 schrieb Igor Mammedov:
>>>> On Sat, 15 Jul 2017 08:08:58 -1000
>>>> Richard Henderson <address@hidden> wrote:
>>>>
>>>>> On 07/14/2017 03:52 AM, Igor Mammedov wrote:
>>>>>> @@ -230,6 +230,8 @@ static void m68k_cpu_realizefn(DeviceState *dev,
>>>>>> Error **errp)
>>>>>> M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
>>>>>> Error *local_err = NULL;
>>>>>>
>>>>>> + register_m68k_insns(&cpu->env);
>>>>>> +
>>>>>
>>>>> I think it would make more sense to do this during m68k_tcg_init.
>>>>>
>>>> it seems that m68k_cpu_initfn accesses 'env' via some global,
>>>> while cpu_mk68k_init() used to access concrete pointer of just created cpu,
>>>>
>>>> how about moving register_m68k_insns() to m68k_cpu_initfn(), instead?
>>>> it should be equivalent to what cpu_mk68k_init() used to do.
>>>
>>> As a general note, realize should be re-entrant. Can't tell from the
>>> above diff whether that is the case here.
>> Looking at
>>
>> void register_m68k_insns (CPUM68KState *env)
>>
>> {
>>
>> /* Build the opcode table only once to avoid
>>
>> multithreading issues. */
>>
>> if (opcode_table[0] != NULL) {
>>
>> return;
>>
>> }
>>
>> it is save to use multiple times,
>>
>> also looking further in it:
>>
>> #define BASE(name, opcode, mask) \
>>
>> register_opcode(disas_##name, 0x##opcode, 0x##mask)
>>
>> #define INSN(name, opcode, mask, feature) do { \
>>
>> if (m68k_feature(env, M68K_FEATURE_##feature)) \
>>
>> BASE(name, opcode, mask); \
>>
>> } while(0)
>>
>> BASE(undef, 0000, 0000);
>>
>> INSN(arith_im, 0080, fff8, CF_ISA_A);
>>
>> INSN macro depends on enabled features, it might work with current code that
>> has no user settable features but it will break once that is available.
>>
>> So I retract my suggestion to move register_m68k_insns() into
>> m68k_cpu_initfn()
>> and keep it as it's in this patch (in m68k_cpu_realizefn()),
>> that way features theoretically set between initfn(and m68k_tcg_init) and
>> realize() will
>> have effect on created cpu and we won't have to fix it in future.
>
> Richard, Laurent,
>
> Do you agree with keeping register_m68k_insns() in realize()?
>
I agree.
Acked-by: Laurent Vivier <address@hidden>