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

[Jason A. Donenfeld]

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);


With (1), Thomas added that to v8.

With (3), doesn't the upper layer of the tcg dispatcher already check
that, and return an error back to the guest? If so, then your change
doesn't do anything. If not, then your change introduces a DoS, since
a guest can now crash the host process by triggering that g_assert(),
right? I had assumed the tcg dispatcher was checking that.

With (2), I found this text:

4. For COMPUTE INTERMEDIATE MESSAGE
DIGEST, the second-operand length is not a multiple
of the data block size of the designated
function (see Figure 7-65 on page 7-102 for
COMPUTE INTERMEDIATE MESSAGE
DIGEST functions). This specification-exception
condition does not apply to the query function,
nor does it apply to COMPUTE LAST MESSAGE
DIGEST.

This part seems like the most important delta between Thomas' plan and
what you posted in v8.1.

With that said, your style refactorings are probably a nice thing to
keep around too.

Jason