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

[David Hildenbrand]

On 22.09.22 16:35, Jason A. Donenfeld wrote:
> On Thu, Sep 22, 2022 at 03:07:13PM +0200, David Hildenbrand wrote:
> > +        if (processed >= MAX_BLOCKS_PER_RUN * 128) {
> > +            break;
>
> If you make this a `goto out` or similar instead of a break, then you
> can
>
> > +    if (type == S390_FEAT_TYPE_KLMD && len < 128) {
>
> change that to `if (len)`.


Thanks, I'll do this on top:

diff --git a/target/s390x/tcg/crypto_helper.c b/target/s390x/tcg/crypto_helper.c
index 67133ba33a..c1505b27a4 100644
--- a/target/s390x/tcg/crypto_helper.c
+++ b/target/s390x/tcg/crypto_helper.c
@@ -190,7 +190,7 @@ static int cpacf_sha512(CPUS390XState *env, uintptr_t ra, 
uint64_t param_addr,
         uint64_t w[16];
 
         if (processed >= MAX_BLOCKS_PER_RUN * 128) {
-            break;
+            goto write_ocv;
         }
 
         sha512_read_block(env, *message_reg + processed, w, ra);
@@ -198,7 +198,7 @@ static int cpacf_sha512(CPUS390XState *env, uintptr_t ra, 
uint64_t param_addr,
     }
 
     /* KMLD: Process partial/empty block last. */
-    if (type == S390_FEAT_TYPE_KLMD && len < 128) {
+    if (len) {
         uint8_t x[128];
 
         /* Read the remainder of the message byte-per-byte. */
@@ -237,6 +237,7 @@ static int cpacf_sha512(CPUS390XState *env, uintptr_t ra, 
uint64_t param_addr,
      * TODO: if writing fails halfway through (e.g., when crossing page
      * boundaries), we're in trouble. We'd need something like 
access_prepare().
      */
+write_ocv:
     sha512_write_ocv(env, param_addr, a, ra);
     *message_reg = deposit64(*message_reg, 0, message_reg_len,
                              *message_reg + processed);


> > +        /*
> > +         * Pad the remainder with zero and place magic value 128 after the
> > +         * message.
> > +         */
> > +        memset(x + len, 0, 128 - len);
> > +        x[len] = 128;
>
> "magic value 128" ==> "set the top bit"

Yes, thanks. I missed that detail in the PoP. (not sure if it's documented at 
all ...)

"Pad the remainder with zero and set the top bit."

> Aside from these nits, this refactoring looks fine. I haven't tested
> this or checked the crypto carefully, but if it passes all the Linux
> test vectors, hopefully things are still fine.


Thanks. I'll resend this patch only as reply to your original one,
so Thomas can easily pick it up (or add more feedback :)).

--
Thanks,

David / dhildenb