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

[Surya Kumari Jangala]

I have incorporated the following review comments, and committed the patch after getting approval from Peter.

Regards,
Surya

On 07/01/25 2:17 am, Peter Bergner wrote:
> 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
> 
>