| Message ID | 20220422185450.107256-4-victor.colombo@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Remove hidden usages of *env | expand |
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~
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>
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.
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>
On 4/28/22 07:56, Víctor Colombo wrote: > 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)) There's also room for a big-endian set of registerfield macros. (Don't forget s390x does the same thing, so "PPC" isn't appropriate generically.) Something like #define BE_FIELD_W(reg, field, regwidth, start, length) \ FIELD(reg, field, regwidth - start - length, length) #define BE_FIELD32(reg, field, start, length) \ BE_FIELD_(reg, field, 32, start, length) #define BE_FIELD64(reg, field, start, length) \ BE_FIELD_(reg, field, 64, start, length) at which point the usual FIELD_EX* and FIELD_DP* macros will work. r~
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 */ >>>>> 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. 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. Regards, BALATON Zoltan > 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> > >
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) { qemu_log_mask(LOG_GUEST_ERROR, "Hypercall made with MSR[PR]=1\n"); env->gpr[3] = H_PRIVILEGE; } else if (env->gpr[3] == KVMPPC_H_RTAS) { diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 22569305d2..c947494099 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1269,7 +1269,7 @@ static void emulate_spapr_hypercall(PPCVirtualHypervisor *vhyp, g_assert(!vhyp_cpu_in_nested(cpu)); - if (msr_pr) { + if (env->msr & M_MSR_PR) { hcall_dprintf("Hypercall made with MSR[PR]=1\n"); env->gpr[3] = H_PRIVILEGE; } else { diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 106b555b86..2ad023e981 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -353,6 +353,8 @@ typedef enum { #define MSR_RI 1 /* Recoverable interrupt 1 */ #define MSR_LE 0 /* Little-endian mode 1 hflags */ +#define M_MSR_PR (1ull << MSR_PR) + /* PMU bits */ #define MMCR0_FC PPC_BIT(32) /* Freeze Counters */ #define MMCR0_PMAO PPC_BIT(56) /* Perf Monitor Alert Ocurred */ @@ -474,7 +476,6 @@ typedef enum { #define msr_ce ((env->msr >> MSR_CE) & 1) #define msr_ile ((env->msr >> MSR_ILE) & 1) #define msr_ee ((env->msr >> MSR_EE) & 1) -#define msr_pr ((env->msr >> MSR_PR) & 1) #define msr_fp ((env->msr >> MSR_FP) & 1) #define msr_me ((env->msr >> MSR_ME) & 1) #define msr_fe0 ((env->msr >> MSR_FE0) & 1) diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index d42e2ba8e0..6e2b23a859 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -6303,7 +6303,7 @@ static bool cpu_has_work_POWER9(CPUState *cs) if ((env->pending_interrupts & (1u << PPC_INTERRUPT_EXT)) && (env->spr[SPR_LPCR] & LPCR_EEE)) { bool heic = !!(env->spr[SPR_LPCR] & LPCR_HEIC); - if (heic == 0 || !msr_hv || msr_pr) { + if (!heic || !msr_hv || (env->msr & M_MSR_PR)) { return true; } } @@ -6517,7 +6517,7 @@ static bool cpu_has_work_POWER10(CPUState *cs) if ((env->pending_interrupts & (1u << PPC_INTERRUPT_EXT)) && (env->spr[SPR_LPCR] & LPCR_EEE)) { bool heic = !!(env->spr[SPR_LPCR] & LPCR_HEIC); - if (heic == 0 || !msr_hv || msr_pr) { + if (!heic || !msr_hv || (env->msr & M_MSR_PR)) { return true; } } diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index d3e2cfcd71..10cd381be2 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1738,7 +1738,7 @@ static void ppc_hw_interrupt(CPUPPCState *env) bool lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0); bool heic = !!(env->spr[SPR_LPCR] & LPCR_HEIC); /* HEIC blocks delivery to the hypervisor */ - if ((async_deliver && !(heic && msr_hv && !msr_pr)) || + if ((async_deliver && !(heic && msr_hv && !(env->msr & M_MSR_PR))) || (env->has_hv_mode && msr_hv == 0 && !lpes0)) { if (books_vhyp_promotes_external_to_hvirt(cpu)) { powerpc_excp(cpu, POWERPC_EXCP_HVIRT); @@ -1818,7 +1818,7 @@ static void ppc_hw_interrupt(CPUPPCState *env) * EBB exception must be taken in problem state and * with BESCR_GE set. */ - if (msr_pr == 1 && env->spr[SPR_BESCR] & BESCR_GE) { + if ((env->msr & M_MSR_PR) && (env->spr[SPR_BESCR] & BESCR_GE)) { env->pending_interrupts &= ~(1 << PPC_INTERRUPT_EBB); if (env->spr[SPR_BESCR] & BESCR_PMEO) { @@ -2094,7 +2094,7 @@ static void do_ebb(CPUPPCState *env, int ebb_excp) env->spr[SPR_BESCR] |= BESCR_EEO; } - if (msr_pr == 1) { + if (env->msr & M_MSR_PR) { powerpc_excp(cpu, ebb_excp); } else { env->pending_interrupts |= 1 << PPC_INTERRUPT_EBB; diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c index c4ff8fd632..bd219e9c9c 100644 --- a/target/ppc/mem_helper.c +++ b/target/ppc/mem_helper.c @@ -613,10 +613,10 @@ void helper_tbegin(CPUPPCState *env) (1ULL << TEXASR_FAILURE_PERSISTENT) | (1ULL << TEXASR_NESTING_OVERFLOW) | (msr_hv << TEXASR_PRIVILEGE_HV) | - (msr_pr << TEXASR_PRIVILEGE_PR) | + (!!(env->msr & M_MSR_PR) << TEXASR_PRIVILEGE_PR) | (1ULL << TEXASR_FAILURE_SUMMARY) | (1ULL << TEXASR_TFIAR_EXACT); - env->spr[SPR_TFIAR] = env->nip | (msr_hv << 1) | msr_pr; + env->spr[SPR_TFIAR] = env->nip | (msr_hv << 1) | !!(env->msr & M_MSR_PR); env->spr[SPR_TFHAR] = env->nip + 4; env->crf[0] = 0xB; /* 0b1010 = transaction failure */ } diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 5414fd63c1..d7b8b97ee7 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -191,12 +191,12 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type, } /* Determine permissions allowed by Encoded Access Authority */ - if (!partition_scoped && (pte & R_PTE_EAA_PRIV) && msr_pr) { + if (!partition_scoped && (pte & R_PTE_EAA_PRIV) && (env->msr & M_MSR_PR)) { *prot = 0; } else if (mmuidx_pr(mmu_idx) || (pte & R_PTE_EAA_PRIV) || partition_scoped) { *prot = ppc_radix64_get_prot_eaa(pte); - } else { /* !msr_pr && !(pte & R_PTE_EAA_PRIV) && !partition_scoped */ + } else { /* !M_MSR_PR && !(pte & R_PTE_EAA_PRIV) && !partition_scoped */ *prot = ppc_radix64_get_prot_eaa(pte); *prot &= ppc_radix64_get_prot_amr(cpu); /* Least combined permissions */ } diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index e9c5b14c0f..fef2b11733 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -273,8 +273,8 @@ static inline void bat_size_prot(CPUPPCState *env, target_ulong *blp, bl = (*BATu & 0x00001FFC) << 15; valid = 0; prot = 0; - if (((msr_pr == 0) && (*BATu & 0x00000002)) || - ((msr_pr != 0) && (*BATu & 0x00000001))) { + if ((!(env->msr & M_MSR_PR) && (*BATu & 0x00000002)) || + ((env->msr & M_MSR_PR) && (*BATu & 0x00000001))) { valid = 1; pp = *BATl & 0x00000003; if (pp != 0) { @@ -368,16 +368,17 @@ static int get_segment_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx, PowerPCCPU *cpu = env_archcpu(env); hwaddr hash; target_ulong vsid; - int ds, pr, target_page_bits; + int ds, target_page_bits; + bool pr; int ret; target_ulong sr, pgidx; - pr = msr_pr; + pr = env->msr & M_MSR_PR; ctx->eaddr = eaddr; sr = env->sr[eaddr >> 28]; - ctx->key = (((sr & 0x20000000) && (pr != 0)) || - ((sr & 0x40000000) && (pr == 0))) ? 1 : 0; + ctx->key = (((sr & 0x20000000) && pr) || + ((sr & 0x40000000) && !pr)) ? 1 : 0; ds = sr & 0x80000000 ? 1 : 0; ctx->nx = sr & 0x10000000 ? 1 : 0; vsid = sr & 0x00FFFFFF; @@ -386,8 +387,8 @@ static int get_segment_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx, "Check segment v=" TARGET_FMT_lx " %d " TARGET_FMT_lx " nip=" TARGET_FMT_lx " lr=" TARGET_FMT_lx " ir=%d dr=%d pr=%d %d t=%d\n", - eaddr, (int)(eaddr >> 28), sr, env->nip, env->lr, (int)msr_ir, - (int)msr_dr, pr != 0 ? 1 : 0, + eaddr, (int)(eaddr >> 28), sr, env->nip, env->lr, + (int)msr_ir, (int)msr_dr, pr ? 1 : 0, access_type == MMU_DATA_STORE, type); pgidx = (eaddr & ~SEGMENT_MASK_256M) >> target_page_bits; hash = vsid ^ pgidx; @@ -530,7 +531,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, ret = -1; raddr = (hwaddr)-1ULL; - pr = msr_pr; + pr = env->msr & M_MSR_PR; for (i = 0; i < env->nb_tlb; i++) { tlb = &env->tlb.tlbe[i]; if (ppcemb_tlb_check(env, tlb, &raddr, address, @@ -618,7 +619,7 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb, found_tlb: - if (msr_pr != 0) { + if (env->msr & M_MSR_PR) { prot2 = tlb->prot & 0xF; } else { prot2 = (tlb->prot >> 4) & 0xF; @@ -768,7 +769,7 @@ static bool mmubooke206_get_as(CPUPPCState *env, return true; } else { *as_out = msr_ds; - *pr_out = msr_pr; + *pr_out = env->msr & M_MSR_PR; return false; } }
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(-)