Re: [PATCH] target/ppc: Fix SPE unavailable exception triggering

["david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]

On Sun, Jul 26, 2020 at 04:59:16PM +0000, Matthieu Bucchianeri wrote:
> Hello Balaton,
> 
> Thank you for your thorough review! See my response below.
> 
> > > static inline void gen_evmwsmiaa(DisasContext *ctx) {
> > > -    TCGv_i64 acc = tcg_temp_new_i64();
> > > -    TCGv_i64 tmp = tcg_temp_new_i64();
> > > +    TCGv_i64 acc;
> > > +    TCGv_i64 tmp;
> > > +
> > > +    if (unlikely(!ctx->spe_enabled)) {
> > > +        gen_exception(ctx, POWERPC_EXCP_SPEU);
> > > +        return;
> > > +    }
> >
> > Isn't this missing here:
> >
> > acc = tcg_temp_new_i64();
> > tmp = tcg_temp_new_i64();
> >
> > You've removed allocating temps but this hunk does not add it back after the
> > exception unlike others in this patch.
> 
> I should have probably mentioned somewhere that this patch also
> fixes a resource leak in that function. It is not very obvious
> when looking at it as a patch, but if you take a look at the
> original code, you will see that I removed these allocations on
> purpose:
> 
>
>https://github.com/qemu/qemu/blob/d577dbaac7553767232faabb6a3e291aebd348ae/target/ppc/translate/spe-impl.inc.c#L532

Ok, can you please split the memory leak fix into a separate patch to
make this easier to review.

> 
> > static inline void gen_evmwsmiaa(DisasContext *ctx)
> > {
> >     TCGv_i64 acc = tcg_temp_new_i64();
> >     TCGv_i64 tmp = tcg_temp_new_i64();
> >
> >     gen_evmwsmi(ctx);           /* rD := rA * rB */
> >
> >     acc = tcg_temp_new_i64();
> >     tmp = tcg_temp_new_i64();
> 
> I apologize for not making this any more clear in my description.
> 
> Let me know if this looks correct now, with the full context in mind.
> 
> Thanks.
> 
> LeoStella, LLC
> A joint venture of Thales Alenia Space and Spaceflight Industries
> 
> 12501 E Marginal Way S • Tukwila, WA 98168
> 
> Proprietary Document: This document may contain trade secrets or otherwise proprietary and confidential information owned by LeoStella LLC. It is intended only for the individual addressee and may not be copied, distributed, or otherwise disclosed without LeoStella LLC express prior written authorization.
> Export Controlled: This document may contain technical data whose export is restricted by the Arms Export Control Act (Title 22, U.S.C., Sec 2751 et seq.) or the Export Administration Act of 1979, as amended, Title 50,U.S.C., app 2401 et seq. Violation of these export laws are subject to severe criminal penalties. Recipient shall not export, re-export, or otherwise transfer or share this document to any foreign person (as defined by U.S. export laws) without advance written authorization from LeoStella LLC.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]