Re: [PATCH 2/5] Support Intel SHA512

[Jan Beulich <jbeulich@suse.com>]

On 14.07.2023 05:40, Jiang, Haochen wrote:
>> Up-front question on title and naming in the patch: Doc indeed says just
>> SHA512 (same for SM3 and SM4), but are you (including those who assigned
>> those names) sure that's going to stay this way by the time this is merged
>> into the SDM? Considering other ISA names, AVX-SHA512 would seem more
>> consistent to me.
> 
> SHA512 is not an ISA under AVX set.

I'm afraid I don't understand. How is it not? It uses YMM registers.
And conceivably there could be EVEX encodings of these (allowing the
full 32 register set to be used), which I'd then call AVX512-SHA512.

It's also not possible to potentially express the same thing in
legacy encodings (unlike e.g. GFNI). Even for SM3, where only 128-
bit operations are used, that's not possible, as the insns have 3
inputs (the destination is r/w).

> So AVX-SHA512 is not used.
> 
> The actual meaning in SDM/ISE is that we need to check both AVX and SHA512
> feature bit to use the instruction.
> 
> I could drop the imply in implementation and change to checking both ISA bit
> set. But since it will use xmm/ymm register, in current implementation, we
> choose to imply AVX for SHA512 for convenience.
> 
> Whether it should be AVX/AVX2 will be mentioned below.
>[...]
>>> --- a/opcodes/i386-gen.c
>>> +++ b/opcodes/i386-gen.c
>>> @@ -168,6 +168,8 @@ static const dependency isa_dependencies[] =
>>>      "LKGS" },
>>>    { "AVX_VNNI_INT16",
>>>      "AVX2" },
>>> +  { "SHA512",
>>> +    "AVX" },
>>
>> Like for the earlier patch this wants to move up a little. I also question that it's
>> AVX that's the baseline feature here. While correct for SM3, I expect it needs
>> to be AVX2 both here and for SM4, for AVX offering no real 256-bit integer
>> operations. (Obviously this wants taking care of in the doc as well.)
> 
> You got a point here.
> 
> I will check with the design and HW team since it is actually AVX2 introduces the
> 256-bit integer operations to see if this is a misuse.
> 
> One reason I can think of using AVX only is that SHA512 and SM4 actually do not
> need other integer operations to help with. It only needs VMOV, which is introduced
> by AVX. So when hardware checking XSTATE, AVX is enough.

So for a feature check requirement referencing just AVX may be okay. But
there's not going to be any SHA512 without AVX anyway, for there not
being any YMM registers without AVX; you wouldn't be able to fill the
register operands. Hence the extra feature check is redundant (and would
hence better be omitted).

As to implying baseline functionality, using AVX (rather than AVX2) makes
little sense, so even if the feature check remained (note that various
other extensions, including e.g. AVX-VNNI-INT<n>, don't have such a
secondary requirement), I'd still be fairly insistent on having the
base feature named here (and for SM4) be AVX2 (to be in line with other
similar baseline selections).

Jan