Re: [PATCH 03/20] target/ppc: Substitute msr_pr macro with new M_MSR_PR macro

[Víctor Colombo]

On 28/04/2022 03:46, Cédric Le Goater wrote:
> On 4/27/22 19:00, Víctor Colombo wrote:
> > Hello everyone! Thanks Zoltan and Richard for your kind reviews!
> >
> > On 26/04/2022 18:29, Richard Henderson wrote:
> > > On 4/22/22 11:54, Víctor Colombo wrote:
> > > > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> > > > ---
> > > >   hw/ppc/pegasos2.c        |  2 +-
> > > >   hw/ppc/spapr.c           |  2 +-
> > > >   target/ppc/cpu.h         |  3 ++-
> > > >   target/ppc/cpu_init.c    |  4 ++--
> > > >   target/ppc/excp_helper.c |  6 +++---
> > > >   target/ppc/mem_helper.c  |  4 ++--
> > > >   target/ppc/mmu-radix64.c |  4 ++--
> > > >   target/ppc/mmu_common.c  | 23 ++++++++++++-----------
> > > >   8 files changed, 25 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> > > > index 56bf203dfd..27ed54a71d 100644
> > > > --- a/hw/ppc/pegasos2.c
> > > > +++ b/hw/ppc/pegasos2.c
> > > > @@ -461,7 +461,7 @@ static void 
> > > > pegasos2_hypercall(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
> > > >       /* The TCG path should also be holding the BQL at this point */
> > > >       g_assert(qemu_mutex_iothread_locked());
> > > >
> > > > -    if (msr_pr) {
> > > > +    if (env->msr & M_MSR_PR) {
> > >
> > > I'm not sure I'm keen on the M_ prefix, but I'll defer to Cedric or 
> > > Daniel if they're ok
> > > with it.
> > >
> > > In general there are inconsistencies with the use of MSR_PR (1 vs 
> > > 1ull), which makes it
> > > tempting to replace MSR_PR the bit number with MSR_PR the mask and 
> > > leave off the M_
> > > prefix.  It's somewhat easy for MSR_PR, since missed conversions will 
> > > certainly result in
> > > compiler warnings for out-of-range shift (the same would not be true 
> > > with bits 0-6, LE
> > > through EP). >
> > > Another possibility would be to use hw/registerfields.h.  Missed 
> > > conversions are missing
> > > symbol errors.  You'd write FIELD_EX64(env->msr, MSR, PR) in cases 
> > > like this and
> > > R_MSR_PR_MASK in cases like cpu_init.c.  It's more verbose for single 
> > > bits like this, but
> > > much easier to work with multi-bit fields like MSR.TS.
> > Thanks for your ideas! I think I'll go with this second one. It'll allow
> > to remove the !!(val) occurrences that I introduced in this series, so
> > the code quality will probably be improved.
> >
> > It'll also be a 'safer' change that will require less rework on other
> > parts that I didn't intend to touch in this patch series.
>
>
> The registerfield API is certainly a good choice for cleanups.
>
> Is there a way to adapt the PPC_BIT macros and keep the PPC bit
> ordering ? It might be easier to forget about it. Which is what
> the Linux kernel does in many places.

Hello Cédric.

It would probably be easier to change this if we went with Zoltan's
idea. Just 'invert' the MSR_* values to match the ISA order and use
env->msr & PPC_BIT(MSR_*). However registerfield API expects it to be
in the "0 is the rightmost" order,
so we can't easily go with it and just invert the MSR_* values.

A solution I could think that might be easy is: rename PPC_BIT to
PPC_BIT_ULL (behaves like BIT_ULL but 'inverted'), and create a new
PPC_BIT macro that just inverts the bit value

#define PPC_BIT_ULL(bit) (0x8000000000000000ULL >> (bit))
#define PPC_BIT(bit) (63 - (bit))

and change MSR_* to use it

#define MSR_LE PPC_BIT(63)

> Device models are also impacted :
>
>    include/hw/pci-host/pnv_phb*_regs.h
>    include/hw/ppc/xive*_regs.h
>
> Something I have been wanting to change for a while are these macros :
>
>      static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>      {
>          return (word & mask) >> ctz64(mask);
>      }
>
>      static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>                                      uint64_t value)
>      {
>          return (word & ~mask) | ((value << ctz64(mask)) & mask);
>      }
>
> Thanks,
>
> C.

Thanks!

--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>