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

["Cédric Le Goater" <clg@kaod.org>]

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.


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.