On 21 June 2013 00:25, Måns Rullgård <m...@mansr.com> wrote:
> Peter Maydell <peter.mayd...@linaro.org> writes:
>> On 18 June 2013 15:30, Mans Rullgard <m...@mansr.com> wrote:
>>> @@ -6699,6 +6742,12 @@ static void disas_arm_insn(CPUARMState * env,
>>> DisasContext *s)
>>> }
>>> return; /* v7MP: Unallocated memory hint: must NOP */
>>> }
>>> + if ((insn & 0x0f000010) == 0x0e000000) {
>>> + /* cdp2 */
>>> + if (disas_coproc_insn(env, s, insn))
>>> + goto illegal_op;
>>> + return;
>>> + }
>>
>> This hunk is oddly placed, because it's neither next to the neon
>> decode (which is further up) nor the mrc2/mcr2 decode (which is
>> further down).
>
> That's because it is neither. It is CDP2, previously not decoded at all.
> This seemed as logical a place as any to me. If you disagree, please
> say where you'd prefer that it go.
Well, if we're treating it in the same way as Neon (ie pulling
the VFP decode out of the decode_coproc() function as I suggest),
then the logical place to put VFP decode is next to the Neon
decode.
At that point the need to actually decode CDP2 into a call
to disas_coproc_insn() goes away, but if we did want to include
it then the logical place to put that decode is next to the
MCR2/MRC2 decode, because both CDP2 and MCR2/MRC2 deal with
coprocessor instructions and are in the same table in the ARM ARM.
-- PMM
