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>