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

[Thomas Huth]

On 23/09/2022 14.07, Jason A. Donenfeld wrote:
> 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?

Thanks, but I think it's a trivial contextual conflict only - I can fix that 
up when picking up the patches.

 Thomas