|
From: | Bruno Piazera Larsen |
Subject: | RE: [RFC PATCH 2/4] target/ppc: isolated SPR read/write callbacks |
Date: | Mon, 26 Apr 2021 20:38:28 +0000 |
> > 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 |
[Prev in Thread] | Current Thread | [Next in Thread] |