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

[Jason A. Donenfeld]

On Fri, Sep 23, 2022 at 2:05 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 23/09/2022 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
> > 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.
>
> Ok, great, if you're fine with the rework, I'll go with David's v8.1
> instead. (keeping an entry on my TODO list to rework that ugly generic "msa"
> helper function one day - this really kept me being confused for much of my
> patch review time)

Okay, sure. Can one of you just look to see if that g_assert() is
going to be a DoS vector, though, or if it'll never be reached if the
prior code goes well? That's the one remaining thing I'm not sure
about.

Do you want me to rebase 2/2 on top of v8.1?

Jason