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

[David Hildenbrand]

On 23.09.22 08:23, Thomas Huth wrote:
> On 22/09/2022 19.18, David Hildenbrand wrote:
> > On 22.09.22 17:55, Thomas Huth wrote:
> > > On 22/09/2022 17.38, David Hildenbrand wrote:
> > > > From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> > > >
> > > > In order to fully support MSA_EXT_5, we have to support the SHA-512
> > > > special instructions. So implement those.
> > > >
> > > > The implementation began as something TweetNacl-like, and then was
> > > > adjusted to be useful here. It's not very beautiful, but it is quite
> > > > short and compact, which is what we're going for.
> > > ...
> > > > @@ -52,6 +278,9 @@ uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1,
> > > > uint32_t r2, uint32_t r3,
> > > >                 cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
> > > >             }
> > > >             break;
> > > > +    case 3: /* CPACF_*_SHA_512 */
> > > > +        return cpacf_sha512(env, ra, env->regs[1], &env->regs[r2],
> > > > +                            &env->regs[r2 + 1], type);
> > >
> > > I have to say that I liked Jason's v8 better here. Code 3 is also used for
> > > other instructions with completely different meaning, e.g. PCKMO uses 3 for
> > > TDEA-192 ... so having the "type" check here made more sense.
> > > (meta comment: maybe we should split up the msa function and stop using just
> > > one function for all crypto/rng related instructions? ... but that's of
> > > course something for a different patch series)
> >
> > It kind-f made sense in v8 where we actually had different functions. We no
> > longer have that here.
>
> But the point is that the "msa" helper is also called for other instructions
> like PCKMO which can also be called with code 3. And there it means
> something completely different. ... unless I completely misunderstood the
> code, of course.

test_be_bit() fences everything off we don't support. Simply falling 
through here and returning 0 at the end doesn't make any sense either.

> I think I'll go with Jason's v8 (+ compat machine handling on top) for now,
> so that we better record his state of the patch, and then we can do cleanup
> patches on top later.

Feel free to commit what you want (I'll be happy to see Jason's work 
upstream for good), but note that

1) I don't feel confident to give the original patch my ack/rb
2) I am not a supporter of committing code that has known issues
3) I won't follow up with additional cleanup patches because I already 
spent more time on this than I originally planned.

--
Thanks,

David / dhildenb