[PATCH] PowerPC: Add support for RFC02657 - AES acceleration, instructions

[Peter Bergner]

On 12/18/24 7:26 AM, Surya Kumari Jangala wrote:
> PowerPC: Add support for RFC02657 - AES acceleration instructions
> 
> opcodes/
> 	* ppc-opc.c (insert_m, extract_m): New functions.

Let's change the names to insert_m2 and extract_m2.  We sometimes have
multiple operand fields with the same "name", but different sizes, so
we normally tack on the field size to the name to disambiguate them.





> +static uint64_t
> +insert_m (uint64_t insn,
> +	  int64_t value,
> +	  ppc_cpu_t dialect ATTRIBUTE_UNUSED,
> +	  const char **errmsg)
> +{
> +  if (value!= 0 && value != 1 && value != 2)

Missing space ^


> +    *errmsg = _("invalid M value");
> +  return insn | ((value & 0x2) << (16 - 1)) | ((value & 0x1) << 11);
> +}

Please replace the (16 - 1) with just 15, like you did in the extract
function.



> +static int64_t
> +extract_m (uint64_t insn,
> +	   ppc_cpu_t dialect ATTRIBUTE_UNUSED,
> +	   int *invalid)
> +{
> +  int64_t value = ((insn >> 11) & 1) | ((insn >> 15) & 2);

Let's use 0x1/0x2 rather than 1/2 to match the bitmask values we used
in the insert function, just to be consistent.  I know we aren't always
consistent. :-(

Also, let's swap the order of sub-expressions so that the MSB is on the
left and the LSB on the right.  Like so:

  int64_t value = ((insn >> 15) & 0x2) | ((insn >> 11) & 0x1);



> +  if (value != 0 && value != 1 && value != 2)
> +    *invalid = 1;
> +  return value;
> +}

Let's change this to just "if (value == 3)" here, since given how we
computed value, it can only contain the values 0, 1, 2 or 3.  That's not
the case for the insert_m2 function, so I'm fine leaving that test as
is, since value could be be any 64-bit signed value there.




> +#define AESM DMEX + 1
> +  { 0x3, PPC_OPSHIFT_INV, insert_m, extract_m, 0 },

Needs updating to the new insert_m2 and extract_m2 names.
It also needs a comment.  Something like:

  /* The 2-bit M field in an AES XX2/XX3 form instruction.  This is split.  */




> +  /* The P field in Galois Field XX3 form instruction.  */
> +#define P yx
>    { 0x1, 8, NULL, NULL, 0 },

Similarly, let's change this to P1.



> +/* An XX2 form instruction with the M bits specified.  */
> +#define XX2M(op, xop, m0, m1)			\
> +  (XX2 (op, xop)				\
> +   | (((uint64_t)(m0) & 1) << 16)		\
> +   | (((uint64_t)(m1) & 1) << 11))

m0 and m1 only exist in the insn's encoding, so let's use one 2-bit
m operand rather than 2 1-bit m0 and m1 operands, so it matches the
mnemonic description.




> +#define XX3M(op, xop, m0, m1)			\
> +  (XX3 (op, xop)				\
> +   | (((uint64_t)(m0) & 1) << 16)		\
> +   | (((uint64_t)(m1) & 1) << 11))

Ditto.



> +/* The masks for XX2 AES instructions with m0, m1 bits.  */
> +#define XX2AES_MASK (XX2M (0x3f, 0x1ff, 0, 0) | (0xf << 17) | 1)
> +#define XX2AESM_MASK (XX2AES_MASK | (1 << 16) | (1 << 11))
> +
> +/* The masks for XX3 AES instructions with m0, m1 bits.  */
> +#define XX3AES_MASK (XX3M (0x3f, 0xff, 0, 0) | 1)
> +#define XX3AESM_MASK (XX3AES_MASK | (1 << 16) | (1 << 11))

...which means these need updating.




> +#define XX3GF_MASK (XX3 (0x3f, 0x1f) | (3 << 9))

This is exactly a XX3_MASK minus the P bit, so I think it's better
written as:

#define XX3GF_MASK (XX3 (0x3f, 0xff) & ~(1 << 8))


> +#define XX3GFP_MASK (XX3GF_MASK | (1 << 8))

...which means this is exactly a XX3_MASK, so this mask isn't needed.




> +{"xxaes128encp",XX3M(60,194,0,0),XX3AESM_MASK,  PPCVSX,	PPCVLE|EXT,	{XTP, XA5p, XB5p}},
[snip]

As Jan mentioned, these should all be enabled with the PPCVSXF mask instead of PPCVSX.
Otherwise, these will all be enabled for -mpower7 and later.


Thanks for working on this!

Peter