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

[address@hidden]

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

signature.asc
Description: PGP signature