Re: [Qemu-devel] [PATCH v2] target-arm: Implement ARMv8 VSEL instruction.

[Peter Maydell]

On 3 October 2013 15:37, Peter Maydell <address@hidden> wrote:
> Ah, that means the ARM ARM table is incorrect, because it implies
> that VSEL is conditional (which it definitely isn't). I need to look
> at where the new insns are in the T32/A32 encodings in more
> detail, then, which I don't have time for just at the moment.

Yes, these are in what would be the CDP2 space in both T32
and A32. So, quick sketch of what I think we should do:
 * move the disas_vfp_insn() calls outside disas_coproc_insn()
   (and in the thumb decode case, to before the "if bit 28 set
   then goto illegal_op" check)
   (basically what you have in this patch is fine)
 * add a call to disas_vfp_insn() in the unconditional code
   (what you have there in this patch is fine, but remember that
   QEMU coding style mandates braces; use scripts/checkpatch.pl.)
 * in disas_vfp_insn(), just after the "is vfp disabled?" check, add:

 if (extract32(insn, 28, 4) == 0xf) {
    /* Encodings with T=1 (Thumb) or unconditional (ARM):
     * only used in v8 and above
     */
    return 1;
 }

That all goes into patch 1 of 2, which is just doing refactoring
and makes no changes in behaviour.

 * then in patch 2 of the series, actually add the VSEL
   support, by replacing that 'return 1' with
   'return disas_vfp_v8_insn(env, s, insn);'
   and implementing that function with the VSEL support.
   [It seems better to me to have this separate rather than
   fully integrated into the existing logic of disas_vfp_insn
   because we know that no new insn is ever going to use the
   legacy/deprecated vfp vector support. And the function is
   already 800 lines long...]

thanks
-- PMM