Re: [PATCH 2/7] target/ppc: Add support for prefixed load/store instructions

[Gustavo Romero]

Hi David,

On 10/8/20 9:50 PM, David Gibson wrote:
> On Mon, Oct 05, 2020 at 01:03:14AM -0300, Gustavo Romero wrote:
> > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > This commit adds support for the following prefixed load and store
> > instructions for GPRs:
> >
> >    plbz, plh{a,z}, plw{a,z}, pld, plq
> >    pstb, psth, pstw, pstd, pstq
> >    paddi
>
> Are there architecture docs publically available yet which describe
> these instructions?

Yep, just to recap, it's pointed out in the following thread:

https://lists.nongnu.org/archive/html/qemu-ppc/2020-10/msg00095.html


> > Signed-off-by: Michael Roth <mroth@lamentation.net>
> > [ gromero: - fix for gen_addr_imm34_index()
> >             - removed redundant PREFIX_R bit helper
> >             - changes in commit log ]
> > Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> > ---
> >   target/ppc/internal.h  |   6 +
> >   target/ppc/translate.c | 259 +++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 265 insertions(+)
> >
> > diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> > index d03d691a44..dc4aff863c 100644
> > --- a/target/ppc/internal.h
> > +++ b/target/ppc/internal.h
> > @@ -159,6 +159,12 @@ EXTRACT_HELPER(PREFIX_TYPE, 24, 2);
> >   EXTRACT_HELPER(PREFIX_ST1, 23, 1);
> >   /* 4-bit sub-type */
> >   EXTRACT_HELPER(PREFIX_ST4, 20, 4);
> > +/* 18 bits signed immediate value */
> > +EXTRACT_SHELPER(SIMM18, 0, 18);
> > +/* 18 bits unsigned immediate value */
> > +EXTRACT_HELPER(UIMM18, 0, 18);
> > +/* relative to CIA (current instruction address) */
> > +EXTRACT_HELPER(R, 20, 1);
> >   
> >   #ifndef CONFIG_USER_ONLY
> >   EXTRACT_HELPER(SR, 16, 4);
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 96c2997d3f..82376b50ff 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -2495,6 +2495,34 @@ static inline void gen_addr_add(DisasContext *ctx, TCGv 
> > ret, TCGv arg1,
> >       }
> >   }
> >   
> > +/* returns false if exception was generated */
>
> Probably better to return a bool, then.

Right. And the comment should read true instead of false.
Fixed for v2.  

> > +static inline int gen_addr_imm34_index(DisasContext *ctx, TCGv EA)
> > +{
> > +    uint64_t offset;
> > +
> > +    if (R(ctx->prefix) == 1) {
>
> I'm having trouble tracking down this 'R' macro to work out what
> that's doing.

R macro just extracts bit R from prefixed instruction of Type 00 Prefix.
The R bit tells if CIA (current instruction address) must be added to
imm34 (d0||d1) when computing the EA.

I've added comments to gen_addr_imm34 and above the R macro definition
to clarify that for v2.


> > +        if (unlikely(rA(ctx->opcode) != 0)) {
> > +            gen_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> > +            return 1;
> > +        }
> > +        // To find out the address of a prefixed instruction
> > +        // it's necessary to rewind 8 bytes because they are
> > +        // twice the size of non-prefixed instructions.
> > +        tcg_gen_movi_tl(EA, ctx->base.pc_next - 8);

I also took the chance to improve that comment ^ and use /* */.


Thanks,
Gustavo