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

[Richard Henderson]

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.


r~