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

[Víctor Colombo]

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.

I''l work on it!
> r~

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