Re: [PATCH v2 27/28] target/ppc: Remove id_tlbs flag from CPU env

[Nicholas Piggin]

On Wed May 8, 2024 at 2:02 AM AEST, BALATON Zoltan wrote:
> On Tue, 7 May 2024, Nicholas Piggin wrote:
> > On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote:
> >> This flag for split instruction/data TLBs is only set for 6xx soft TLB
> >> MMU model and not used otherwise so no need to have a separate flag
> >> for that.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  hw/ppc/pegasos2.c        |  2 +-
> >>  target/ppc/cpu.h         |  1 -
> >>  target/ppc/cpu_init.c    | 19 +++++--------------
> >>  target/ppc/helper_regs.c |  1 -
> >>  target/ppc/mmu_common.c  | 10 ++--------
> >>  target/ppc/mmu_helper.c  | 12 ++----------
> >>  6 files changed, 10 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> >> index 04d6decb2b..dfc6fab180 100644
> >> --- a/hw/ppc/pegasos2.c
> >> +++ b/hw/ppc/pegasos2.c
> >> @@ -984,7 +984,7 @@ static void *build_fdt(MachineState *machine, int 
> >> *fdt_size)
> >>                            cpu->env.icache_line_size);
> >>      qemu_fdt_setprop_cell(fdt, cp, "i-cache-line-size",
> >>                            cpu->env.icache_line_size);
> >> -    if (cpu->env.id_tlbs) {
> >> +    if (cpu->env.tlb_type == TLB_6XX) {
> >
> > Want to just add the standard comment here?
> >
> >    /* 6xx has separate TLBs for instructions and data */
>
> I think that comment would be redundant here because it's clear from the 
> i-tlb, d-tlb this adds so I can do without a comment in this machine if 
> you don't mind. (If this was not in my machine I would not mind adding a 
> comment but I'd keep this one simple.) I think comments should only be 
> added for things that are not clear from code.

Yes. "Obvious" stuff just builds up until it's not.

If you make a simple inline function to test if tlb is split and it can
est TLB_6XX then you don't need the comment.

Thanks,
Nick