Re: [PATCH v8.1 1/2] target/s390x: support SHA-512 extensions

[David Hildenbrand <david@redhat.com>]

On 23.09.22 13:46, Jason A. Donenfeld wrote:
> Hi David,
> 
> On Fri, Sep 23, 2022 at 1:35 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 23.09.22 13:19, Jason A. Donenfeld wrote:
>>> On Fri, Sep 23, 2022 at 12:47 PM David Hildenbrand <david@redhat.com> wrote:
>>>> You must be fortunate if "one afternoon" is not a significant time
>>>> investment. For me it is a significant investment.
>>>
>>> For me too, to say the least of the multiple afternoons I've spent on
>>> this patch set. Getting back to technical content:
>>>
>>>> and sort out the remaining issues. I thought we (Thomas included) had an
>>>> agreement that that's the way we are going to do it. Apparently I was wrong.
>>>> Most probably I lived in the kernel space too long such that we don't
>>>> rush something upstream. For that reason *I* sent out a patch with
>>>> Here I am, getting told by Thomas that we now do it differently now.
>>>> What I really tried to express here is: if Thomas wants to commit things
>>>> differently now, maybe he can just separate the cleanup parts. I really
>>>> *don't want* to send out a multi-patch series to cleanup individual
>>>> parts -- that takes significantly more time. Especially not if something
>>>> is not committed yet.
>>>
>>> To recap what's been fixed in your v8.1, versus what's been refactored
>>> out of style preference:
>>>
>>> 1) It handles the machine versioning.
>>> 2) It throws an exception on length alignment in KIMD mode:
>>> +    /* KIMD: length has to be properly aligned. */
>>> +    if (type == S390_FEAT_TYPE_KIMD && !QEMU_IS_ALIGNED(len, 128)) {
>>> +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>> +    }
>>> 3) It asserts if type is neither KIMD vs KLMD, with:
>>> +    g_assert(type == S390_FEAT_TYPE_KIMD || type == S390_FEAT_TYPE_KLMD);
>>>
>>
>> One important part is
>>
>> 4) No memory modifications before all inputs were read
> 
> Ahhh, which v8's klmd doesn't do, since it updates the parameter block
> before doing the last block. Is this a requirement of the spec? If

Right, and not only the parameter block, but also registers IIRC.


It depend on the instruction and exception. IIRC, exceptions can be 
nullifying, terminating, suppressing, and partially-completing ...

Suppression: no state modification. PSW updated. Exception triggered. 
"The contents of any result fields, including the condition code, are 
not changed."

Nullification: no state modification. PSW not incremented. Exception 
triggered.

Termination: state partially changed. Bad (e.g., machine check). 
Exception triggered.

Partial completion only applies to "Interruptible Instructions". Instead 
of triggering an exception, the instruction exits (with CC=3 or so) and 
modified the state accordingly. There are only a handful of such 
instructions.



There is a chapter called "Types of Instruction Ending" in the PoP 
that's an interesting read.


So in case of Suppression/Nullification, the expectation is that no 
state (memory, register content) was updated when the exception triggers.


Depending on the way the instruction is used and how exceptions are 
handled, that can be a real issue, for example, if the program assumes 
that registers were not updated, or that memory wasn't updated -- but 
they actually were.

> not, then it doesn't matter. But if so, v8's approach is truly
> hopeless, and we'd be remiss to not go with your refactoring that
> accomplishes this.
I wouldn't call it hopeless, but that was the real reason why a 
restructured your code at all and why I had to play with the code myself 
to see if it can be done any better. All the moving stuff into other 
functions were just attempts to keep the code readable when unifying 
both functions :)

As I said, handling state update is non-trivial, and that instruction is 
especially hard to implement.

I *assume* that we never fail that way in the Linux kernel use case, 
because we don't rely on exceptions at all. So one might argue that v8 
is "good enough" for that.

-- 
Thanks,

David / dhildenb