RE: [RFC PATCH 2/4] target/ppc: isolated SPR read/write callbacks

[Bruno Piazera Larsen]

> > The solution to move it to spr_tcg.c.inc and including it in translate.c
> > is a work in progress, any better solutions are very much appreciated.
> > Also, making the R/W functions not static is required for the next
> > commit.
>
> Looks like this could be done in the next commit then.

Sure, I can separate it like this. It was just easier to make the commit like this, since

I wouldn't need to undo all the static removals and redo them for the next commit

(I tend to work through all the code and then separate changes into commits, maybe

that's a bad habit I should drop), and also I thought it would making reviewing the

next patch easier.

> > +void spr_load_dump_spr(int sprn)
> > +{
> > +#ifdef PPC_DUMP_SPR_ACCESSES
>
> The define needs to come along.

Oops, another one that I forgot. This has the same issue as the one before, so I'm

guessing the same solution: move the define to internal, so both cpu_init.c and

translate.c can access it.

> > +/* I really see no reason to keep these gen_*_xer */
> > +/* instead of just leaving the code in the spr_*_xer */
> > +void gen_read_xer(DisasContext *ctx, TCGv dst)
> > +{
> > +    TCGv t0 = tcg_temp_new();
> > +    TCGv t1 = tcg_temp_new();
> > +    TCGv t2 = tcg_temp_new();
> > +    tcg_gen_mov_tl(dst, cpu_xer);
> > +    tcg_gen_shli_tl(t0, cpu_so, XER_SO);
> > +    tcg_gen_shli_tl(t1, cpu_ov, XER_OV);
> > +    tcg_gen_shli_tl(t2, cpu_ca, XER_CA);
> > +    tcg_gen_or_tl(t0, t0, t1);
> > +    tcg_gen_or_tl(dst, dst, t2);
> > +    tcg_gen_or_tl(dst, dst, t0);
> > +    if (is_isa300(ctx)) {
> > +        tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
> > +        tcg_gen_or_tl(dst, dst, t0);
> > +        tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
> > +        tcg_gen_or_tl(dst, dst, t0);
> > +    }
> > +    tcg_temp_free(t0);
> > +    tcg_temp_free(t1);
> > +    tcg_temp_free(t2);
> > +}
> > +
> > +void gen_write_xer(TCGv src)
> > +{
> > +    /* Write all flags, while reading back check for isa300 */
> > +    tcg_gen_andi_tl(cpu_xer, src,
> > +                    ~((1u << XER_SO) |
> > +                      (1u << XER_OV) | (1u << XER_OV32) |
> > +                      (1u << XER_CA) | (1u << XER_CA32)));
> > +    tcg_gen_extract_tl(cpu_ov32, src, XER_OV32, 1);
> > +    tcg_gen_extract_tl(cpu_ca32, src, XER_CA32, 1);
> > +    tcg_gen_extract_tl(cpu_so, src, XER_SO, 1);
> > +    tcg_gen_extract_tl(cpu_ov, src, XER_OV, 1);
> > +    tcg_gen_extract_tl(cpu_ca, src, XER_CA, 1);
> > +}
>
> These two can continue being static.

Good catch again. But my question (in the comment at the begining) remains:

Is there a good reason to keep them separate from spr_(read|write)_xer, since

they are only used by those functions and aren't much different than other

read|write functions.

> Moving a big amount of code like this to another file *and* rearranging
> the code within the file at the same time makes it harder to review and
> is error prone. I'd move the code in one patch and rearrange things in a
> separate patch if needed.

Yeah... I didn't know about the automated sed command until earlier today

so it didn't occur to me that it could be a problem. The rearranging was either

an accident, or a way to reduce ifdefs, but that can be a trivial patch later on

> > +/* prototypes for readers and writers for SPRs */
> > +
> > +#ifdef TARGET_PPC64
> > +void gen_fscr_facility_check(DisasContext *ctx, int facility_sprn,
> > +                                    int bit, int sprn, int cause);
> > +void gen_msr_facility_check(DisasContext *ctx, int facility_sprn,
> > +                                   int bit, int sprn, int cause);
> > +#endif
>
> The gen_* functions are only called from within the spr.c.inc file. You
> shouldn't need them here.
>
> > +
> > +void spr_load_dump_spr(int sprn);
> > +void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
> > +void spr_store_dump_spr(int sprn);
> > +void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> > +void gen_read_xer(DisasContext *ctx, TCGv dst);
> > +void gen_write_xer(TCGv src);
>
> Same here.

Relics of a different solution. Will remove for v2

> > -static void spr_noaccess(DisasContext *ctx, int gprn, int sprn)
> > -{
> > -#if 0
> > -    sprn = ((sprn >> 5) & 0x1F) | ((sprn & 0x1F) << 5);
> > -    printf("ERROR: try to access SPR %d !\n", sprn);
> > -#endif
> > -}
> > -#define SPR_NOACCESS (&spr_noaccess)
>
> What happens to code in translate.c that checks this? I don't think you
> can remove it.

the define was moved to internal.h (forgot to include in this patch, it's in the

next one, will fix). I don't know if that's a good solution, but it's working for now

<snip>

> >  #if defined(TARGET_PPC64)
> > -#if defined(CONFIG_USER_ONLY)
> > -#define POWERPC970_HID5_INIT 0x00000080
> > -#else
> > -#define POWERPC970_HID5_INIT 0x00000000
> > -#endif
>
> Where do these went?

Same as before:  They were added to internal.h and I forgot.

Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer