[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 03/20] target/ppc: Substitute msr_pr macro with new M_MSR_PR macro
Date: Thu, 28 Apr 2022 23:45:53 +0200 (CEST)

On Thu, 28 Apr 2022, Víctor Colombo wrote:
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 */

-    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.

One thing I'm a bit worried about with registerfields macros is that they use deposit64 and extract64 which have an IMO unneeded assert so this means it adds an expression evaluation at every invocation of these (hopefully the function overhead is optimisied out by inlining) which might have some performance impact. So I still prefer the PPC_BIT macro but changing the MSR_* defines might introduce bugs when not done carefully so I'm nor sure it worths it.

Do we have some performance benchmarks that could be used to evaluate the changes for performance impact? There was some Summer of Code project for this but I think it was abandoned. It would be useful to run that as part of CI testing maybe.


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 :


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);




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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]