* [PATCH v3 0/9] target/ppc: Assorted ppc target fixes
@ 2023-05-15 9:26 Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs Nicholas Piggin
` (9 more replies)
0 siblings, 10 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc; +Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza
Hopefully these are getting close to ready now. There is still the
question about doing better with adding test cases for all this, I
haven't exactly got a good answer yet but I do have kvm-unit-tests
for most at least.
Thanks,
Nick
Nicholas Piggin (9):
target/ppc: Fix width of some 32-bit SPRs
target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation
target/ppc: Fix instruction loading endianness in alignment interrupt
target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward
target/ppc: Change partition-scope translate interface
target/ppc: Add SRR1 prefix indication to interrupt handlers
target/ppc: Implement HEIR SPR
target/ppc: Add ISA v3.1 LEV indication in SRR1 for system call
interrupts
target/ppc: Better CTRL SPR implementation
target/ppc/cpu.h | 1 +
target/ppc/cpu_init.c | 41 +++++++++++++----
target/ppc/excp_helper.c | 98 ++++++++++++++++++++++++++++++++++++----
target/ppc/helper_regs.c | 2 +-
target/ppc/misc_helper.c | 4 +-
target/ppc/mmu-radix64.c | 38 +++++++++++-----
target/ppc/power8-pmu.c | 6 ++-
target/ppc/translate.c | 9 +++-
8 files changed, 164 insertions(+), 35 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-15 10:14 ` Harsh Prateek Bora
2023-05-15 12:03 ` Mark Cave-Ayland
2023-05-15 9:26 ` [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation Nicholas Piggin
` (8 subsequent siblings)
9 siblings, 2 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc; +Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza
Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
targets.
This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
This only goes by the 32/64 classification in the architecture, it
does not try to implement finer details of SPR implementation (e.g.,
not all bits implemented as simple read/write storage).
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: no change.
target/ppc/cpu_init.c | 18 +++++++++---------
target/ppc/helper_regs.c | 2 +-
target/ppc/misc_helper.c | 4 ++--
target/ppc/power8-pmu.c | 2 +-
target/ppc/translate.c | 2 +-
5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 0ce2e3c91d..5aa0b3f0f1 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
}
spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
- &spr_read_generic, &spr_write_generic,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
+ &spr_read_generic, &spr_write_generic32,
KVM_REG_PPC_VRSAVE, 0x00000000);
}
@@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
SPR_NOACCESS, SPR_NOACCESS,
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
KVM_REG_PPC_DAWRX, 0x00000000);
spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
SPR_NOACCESS, SPR_NOACCESS,
@@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
spr_register_hv(env, SPR_TSCR, "TSCR",
SPR_NOACCESS, SPR_NOACCESS,
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
0x00000000);
spr_register_hv(env, SPR_HMER, "HMER",
SPR_NOACCESS, SPR_NOACCESS,
@@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
spr_register_hv(env, SPR_MMCRC, "MMCRC",
SPR_NOACCESS, SPR_NOACCESS,
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
0x00000000);
spr_register_hv(env, SPR_MMCRH, "MMCRH",
SPR_NOACCESS, SPR_NOACCESS,
@@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
spr_register_hv(env, SPR_HDSISR, "HDSISR",
SPR_NOACCESS, SPR_NOACCESS,
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
0x00000000);
spr_register_hv(env, SPR_HRMOR, "HRMOR",
SPR_NOACCESS, SPR_NOACCESS,
@@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
KVM_REG_PPC_ACOP, 0);
spr_register_kvm(env, SPR_BOOKS_PID, "PID",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
KVM_REG_PPC_PID, 0);
#endif
}
@@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
{
spr_register(env, SPR_DEXCR, "DEXCR",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
0);
spr_register(env, SPR_UDEXCR, "DEXCR",
@@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
SPR_NOACCESS, SPR_NOACCESS,
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
0);
spr_register(env, SPR_UHDEXCR, "HDEXCR",
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 779e7db513..fb351c303f 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
/* Exception processing */
spr_register_kvm(env, SPR_DSISR, "DSISR",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_generic32,
KVM_REG_PPC_DSISR, 0x00000000);
spr_register_kvm(env, SPR_DAR, "DAR",
SPR_NOACCESS, SPR_NOACCESS,
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index a9bc1522e2..40ddc5c08c 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
void helper_store_pidr(CPUPPCState *env, target_ulong val)
{
- env->spr[SPR_BOOKS_PID] = val;
+ env->spr[SPR_BOOKS_PID] = (uint32_t)val;
tlb_flush(env_cpu(env));
}
void helper_store_lpidr(CPUPPCState *env, target_ulong val)
{
- env->spr[SPR_LPIDR] = val;
+ env->spr[SPR_LPIDR] = (uint32_t)val;
/*
* We need to flush the TLB on LPID changes as we only tag HV vs
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 1381072b9e..64a64865d7 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
{
pmu_update_cycles(env);
- env->spr[sprn] = value;
+ env->spr[sprn] = (uint32_t)value;
pmc_update_overflow_timer(env, sprn);
}
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index f603f1a939..c03a6bdc9a 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
{
- spr_write_generic(ctx, sprn, gprn);
+ spr_write_generic32(ctx, sprn, gprn);
/*
* SPR_CTRL writes must force a new translation block,
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-16 9:32 ` Daniel Henrique Barboza
2023-05-15 9:26 ` [PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt Nicholas Piggin
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc; +Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza
A store to MMCR0 with PMCjCE=1 fails to update hflags correctly and
results in hflags mismatch:
qemu: fatal: TCG hflags mismatch (current:0x2408003d rebuilt:0x240a003d)
This can be reproduced by running perf on a recent machine.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: new patch.
target/ppc/power8-pmu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 64a64865d7..29e0012ed6 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -236,14 +236,16 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
{
bool hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
bool hflags_pmcc1 = (value & MMCR0_PMCC1) != 0;
+ bool hflags_pmcjce = (value & MMCR0_PMCjCE) != 0;
pmu_update_cycles(env);
env->spr[SPR_POWER_MMCR0] = value;
- /* MMCR0 writes can change HFLAGS_PMCC[01] and HFLAGS_INSN_CNT */
+ /* MMCR0 writes can change HFLAGS_PMCC[01], PMCjCE, and HFLAGS_INSN_CNT */
env->hflags = deposit32(env->hflags, HFLAGS_PMCC0, 1, hflags_pmcc0);
env->hflags = deposit32(env->hflags, HFLAGS_PMCC1, 1, hflags_pmcc1);
+ env->hflags = deposit32(env->hflags, HFLAGS_PMCJCE, 1, hflags_pmcjce);
pmu_update_summaries(env);
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-16 15:46 ` Daniel Henrique Barboza
2023-05-15 9:26 ` [PATCH v3 4/9] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward Nicholas Piggin
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc
Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza, Fabiano Rosas
powerpc ifetch endianness depends on MSR[LE] so it has to byteswap
after cpu_ldl_code(). This corrects DSISR bits in alignment
interrupts when running in little endian mode.
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: no change.
target/ppc/excp_helper.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 199328f4b6..bc2be4a726 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -133,6 +133,24 @@ static void dump_hcall(CPUPPCState *env)
env->nip);
}
+/* Return true iff byteswap is needed in a scalar memop */
+static inline bool need_byteswap(CPUArchState *env)
+{
+ /* SOFTMMU builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
+ return !!(env->msr & ((target_ulong)1 << MSR_LE));
+}
+
+static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr)
+{
+ uint32_t insn = cpu_ldl_code(env, addr);
+
+ if (need_byteswap(env)) {
+ insn = bswap32(insn);
+ }
+
+ return insn;
+}
+
static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
{
const char *es;
@@ -3097,7 +3115,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
/* Restore state and reload the insn we executed, for filling in DSISR. */
cpu_restore_state(cs, retaddr);
- insn = cpu_ldl_code(env, env->nip);
+ insn = ppc_ldl_code(env, env->nip);
switch (env->mmu_model) {
case POWERPC_MMU_SOFT_4xx:
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 4/9] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (2 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 5/9] target/ppc: Change partition-scope translate interface Nicholas Piggin
` (5 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc
Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza, Fabiano Rosas
This optional behavior was removed from the ISA in v3.0, see
Summary of Changes preface:
Data Storage Interrupt Status Register for Alignment Interrupt:
Simplifies the Alignment interrupt by remov- ing the Data Storage
Interrupt Status Register (DSISR) from the set of registers modified
by the Alignment interrupt.
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: no change.
target/ppc/excp_helper.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index bc2be4a726..453750a9d6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1449,13 +1449,16 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
break;
}
case POWERPC_EXCP_ALIGN: /* Alignment exception */
- /* Get rS/rD and rA from faulting opcode */
- /*
- * Note: the opcode fields will not be set properly for a
- * direct store load/store, but nobody cares as nobody
- * actually uses direct store segments.
- */
- env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
+ /* Optional DSISR update was removed from ISA v3.0 */
+ if (!(env->insns_flags2 & PPC2_ISA300)) {
+ /* Get rS/rD and rA from faulting opcode */
+ /*
+ * Note: the opcode fields will not be set properly for a
+ * direct store load/store, but nobody cares as nobody
+ * actually uses direct store segments.
+ */
+ env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
+ }
break;
case POWERPC_EXCP_PROGRAM: /* Program exception */
switch (env->error_code & ~0xF) {
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 5/9] target/ppc: Change partition-scope translate interface
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (3 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 4/9] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 6/9] target/ppc: Add SRR1 prefix indication to interrupt handlers Nicholas Piggin
` (4 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc; +Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza
Rather than always performing partition scope page table translation
with access type of 0 (MMU_DATA_LOAD), pass through the processor
access type which first initiated the translation sequence. Process-
scoped page table loads are then set to MMU_DATA_LOAD access type in
the xlate function.
This will allow more information to be passed to the exception
handler in the next patch.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: new patch to deal with bug in patch 6.
target/ppc/mmu-radix64.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 031efda0df..1fc1ba3ecf 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -380,6 +380,14 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
hwaddr pte_addr;
uint64_t pte;
+ if (pde_addr) {
+ /*
+ * Translation of process-scoped tables/directories is performed as
+ * a read-access.
+ */
+ access_type = MMU_DATA_LOAD;
+ }
+
qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx
" mmu_idx %u 0x%"HWADDR_PRIx"\n",
__func__, access_str(access_type),
@@ -477,10 +485,10 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
* is only used to translate the effective addresses of the
* process table entries.
*/
- ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr,
- pate, &h_raddr, &h_prot,
- &h_page_size, true,
- /* mmu_idx is 5 because we're translating from hypervisor scope */
+ /* mmu_idx is 5 because we're translating from hypervisor scope */
+ ret = ppc_radix64_partition_scoped_xlate(cpu, access_type, eaddr,
+ prtbe_addr, pate, &h_raddr,
+ &h_prot, &h_page_size, true,
5, guest_visible);
if (ret) {
return ret;
@@ -519,11 +527,11 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
* translation
*/
do {
- ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr,
- pate, &h_raddr, &h_prot,
- &h_page_size, true,
/* mmu_idx is 5 because we're translating from hypervisor scope */
- 5, guest_visible);
+ ret = ppc_radix64_partition_scoped_xlate(cpu, access_type, eaddr,
+ pte_addr, pate, &h_raddr,
+ &h_prot, &h_page_size,
+ true, 5, guest_visible);
if (ret) {
return ret;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 6/9] target/ppc: Add SRR1 prefix indication to interrupt handlers
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (4 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 5/9] target/ppc: Change partition-scope translate interface Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 7/9] target/ppc: Implement HEIR SPR Nicholas Piggin
` (3 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc
Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza, Fabiano Rosas
ISA v3.1 introduced prefix instructions. Among the changes, various
synchronous interrupts report whether they were caused by a prefix
instruction in (H)SRR1.
The case of instruction fetch that causes an HDSI due to access of a
process-scoped table faulting on the partition scoped translation is the
tricky one. As with ISIs and HISIs, this does not try to set the prefix
bit because there is no instruction image to be loaded. The HDSI needs
the originating access type to be passed through to the handler to
distinguish this from HDSIs that fault translating process scoped tables
originating from a load or store instruction (in that case the prefix
bit should be provided).
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2:
- Fix a bug where ifetch access causing HDSI due to process scoped table
access failing on partition scope translation would go into infinite
recursive fault attempting to load the instruction image. This
requires more data from the page fault to distinguish this from the
same case but initiated by a load or store (in which case we can load
the instruction image).
target/ppc/excp_helper.c | 44 ++++++++++++++++++++++++++++++++++++++++
target/ppc/mmu-radix64.c | 14 ++++++++++---
2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 453750a9d6..27d321c15f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1346,12 +1346,21 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
return false;
}
+static bool is_prefix_excp(CPUPPCState *env, uint32_t insn)
+{
+ if (!(env->insns_flags2 & PPC2_ISA310)) {
+ return false;
+ }
+ return ((insn & 0xfc000000) == 0x04000000);
+}
+
static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
int srr0, srr1, lev = -1;
+ uint32_t insn = 0;
/* new srr1 value excluding must-be-zero bits */
msr = env->msr & ~0x783f0000ULL;
@@ -1390,6 +1399,41 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
vector |= env->excp_prefix;
+ switch (excp) {
+ case POWERPC_EXCP_HDSI:
+ /* HDSI PRTABLE_FAULT has the originating access type in error_code */
+ if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
+ (env->error_code == MMU_INST_FETCH)) {
+ /*
+ * Fetch failed due to partition scope translation, so prefix
+ * indication is not relevant (and attempting to load the
+ * instruction at NIP would cause recursive faults with the same
+ * translation).
+ */
+ break;
+ }
+ /* fall through */
+ case POWERPC_EXCP_MCHECK:
+ case POWERPC_EXCP_DSI:
+ case POWERPC_EXCP_DSEG:
+ case POWERPC_EXCP_ALIGN:
+ case POWERPC_EXCP_PROGRAM:
+ case POWERPC_EXCP_FPU:
+ case POWERPC_EXCP_TRACE:
+ case POWERPC_EXCP_HV_EMU:
+ case POWERPC_EXCP_VPU:
+ case POWERPC_EXCP_VSXU:
+ case POWERPC_EXCP_FU:
+ case POWERPC_EXCP_HV_FU:
+ insn = ppc_ldl_code(env, env->nip);
+ if (is_prefix_excp(env, insn)) {
+ msr |= PPC_BIT(34);
+ }
+ break;
+ default:
+ break;
+ }
+
switch (excp) {
case POWERPC_EXCP_MCHECK: /* Machine check exception */
if (!FIELD_EX64(env->msr, MSR, ME)) {
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 1fc1ba3ecf..920084bd8f 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -145,6 +145,13 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type,
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
+ env->error_code = 0;
+ if (cause & DSISR_PRTABLE_FAULT) {
+ /* HDSI PRTABLE_FAULT gets the originating access type in error_code */
+ env->error_code = access_type;
+ access_type = MMU_DATA_LOAD;
+ }
+
qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx" 0x%"
HWADDR_PRIx" cause %08x\n",
__func__, access_str(access_type),
@@ -166,7 +173,6 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type,
env->spr[SPR_HDSISR] = cause;
env->spr[SPR_HDAR] = eaddr;
env->spr[SPR_ASDR] = g_raddr;
- env->error_code = 0;
break;
default:
g_assert_not_reached();
@@ -369,13 +375,14 @@ static bool validate_pate(PowerPCCPU *cpu, uint64_t lpid, ppc_v3_pate_t *pate)
}
static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
- MMUAccessType access_type,
+ MMUAccessType orig_access_type,
vaddr eaddr, hwaddr g_raddr,
ppc_v3_pate_t pate,
hwaddr *h_raddr, int *h_prot,
int *h_page_size, bool pde_addr,
int mmu_idx, bool guest_visible)
{
+ MMUAccessType access_type = orig_access_type;
int fault_cause = 0;
hwaddr pte_addr;
uint64_t pte;
@@ -404,7 +411,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
fault_cause |= DSISR_PRTABLE_FAULT;
}
if (guest_visible) {
- ppc_radix64_raise_hsi(cpu, access_type, eaddr, g_raddr, fault_cause);
+ ppc_radix64_raise_hsi(cpu, orig_access_type,
+ eaddr, g_raddr, fault_cause);
}
return 1;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 7/9] target/ppc: Implement HEIR SPR
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (5 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 6/9] target/ppc: Add SRR1 prefix indication to interrupt handlers Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 8/9] target/ppc: Add ISA v3.1 LEV indication in SRR1 for system call interrupts Nicholas Piggin
` (2 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc
Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza, Michael Neuling
The hypervisor emulation assistance interrupt modifies HEIR to
contain the value of the instruction which caused the exception.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2:
- Fix ordering of prefix/suffix words in the register, as pointed
out by Mikey.
target/ppc/cpu.h | 1 +
target/ppc/cpu_init.c | 23 +++++++++++++++++++++++
target/ppc/excp_helper.c | 13 ++++++++++++-
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1c02596d9f..73851aa7a3 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1653,6 +1653,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
#define SPR_HMER (0x150)
#define SPR_HMEER (0x151)
#define SPR_PCR (0x152)
+#define SPR_HEIR (0x153)
#define SPR_BOOKE_LPIDR (0x152)
#define SPR_BOOKE_TCR (0x154)
#define SPR_BOOKE_TLB0PS (0x158)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 5aa0b3f0f1..ff73be1812 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -1629,6 +1629,7 @@ static void register_8xx_sprs(CPUPPCState *env)
* HSRR0 => SPR 314 (Power 2.04 hypv)
* HSRR1 => SPR 315 (Power 2.04 hypv)
* LPIDR => SPR 317 (970)
+ * HEIR => SPR 339 (Power 2.05 hypv) (64-bit reg from 3.1)
* EPR => SPR 702 (Power 2.04 emb)
* perf => 768-783 (Power 2.04)
* perf => 784-799 (Power 2.04)
@@ -5522,6 +5523,24 @@ static void register_power6_common_sprs(CPUPPCState *env)
0x00000000);
}
+static void register_HEIR32_spr(CPUPPCState *env)
+{
+ spr_register_hv(env, SPR_HEIR, "HEIR",
+ SPR_NOACCESS, SPR_NOACCESS,
+ SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_generic, &spr_write_generic32,
+ 0x00000000);
+}
+
+static void register_HEIR64_spr(CPUPPCState *env)
+{
+ spr_register_hv(env, SPR_HEIR, "HEIR",
+ SPR_NOACCESS, SPR_NOACCESS,
+ SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_generic, &spr_write_generic,
+ 0x00000000);
+}
+
static void register_power8_tce_address_control_sprs(CPUPPCState *env)
{
spr_register_kvm(env, SPR_TAR, "TAR",
@@ -5950,6 +5969,7 @@ static void init_proc_POWER7(CPUPPCState *env)
register_power5p_ear_sprs(env);
register_power5p_tb_sprs(env);
register_power6_common_sprs(env);
+ register_HEIR32_spr(env);
register_power6_dbg_sprs(env);
register_power7_book4_sprs(env);
@@ -6072,6 +6092,7 @@ static void init_proc_POWER8(CPUPPCState *env)
register_power5p_ear_sprs(env);
register_power5p_tb_sprs(env);
register_power6_common_sprs(env);
+ register_HEIR32_spr(env);
register_power6_dbg_sprs(env);
register_power8_tce_address_control_sprs(env);
register_power8_ids_sprs(env);
@@ -6234,6 +6255,7 @@ static void init_proc_POWER9(CPUPPCState *env)
register_power5p_ear_sprs(env);
register_power5p_tb_sprs(env);
register_power6_common_sprs(env);
+ register_HEIR32_spr(env);
register_power6_dbg_sprs(env);
register_power8_tce_address_control_sprs(env);
register_power8_ids_sprs(env);
@@ -6409,6 +6431,7 @@ static void init_proc_POWER10(CPUPPCState *env)
register_power5p_ear_sprs(env);
register_power5p_tb_sprs(env);
register_power6_common_sprs(env);
+ register_HEIR64_spr(env);
register_power6_dbg_sprs(env);
register_power8_tce_address_control_sprs(env);
register_power8_ids_sprs(env);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 27d321c15f..529a4513a3 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1608,13 +1608,24 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_HDECR: /* Hypervisor decrementer exception */
case POWERPC_EXCP_HDSI: /* Hypervisor data storage exception */
case POWERPC_EXCP_SDOOR_HV: /* Hypervisor Doorbell interrupt */
- case POWERPC_EXCP_HV_EMU:
case POWERPC_EXCP_HVIRT: /* Hypervisor virtualization */
srr0 = SPR_HSRR0;
srr1 = SPR_HSRR1;
new_msr |= (target_ulong)MSR_HVB;
new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
break;
+ case POWERPC_EXCP_HV_EMU:
+ env->spr[SPR_HEIR] = insn;
+ if (is_prefix_excp(env, insn)) {
+ uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
+ env->spr[SPR_HEIR] <<= 32;
+ env->spr[SPR_HEIR] |= insn2;
+ }
+ srr0 = SPR_HSRR0;
+ srr1 = SPR_HSRR1;
+ new_msr |= (target_ulong)MSR_HVB;
+ new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
+ break;
case POWERPC_EXCP_VPU: /* Vector unavailable exception */
case POWERPC_EXCP_VSXU: /* VSX unavailable exception */
case POWERPC_EXCP_FU: /* Facility unavailable exception */
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 8/9] target/ppc: Add ISA v3.1 LEV indication in SRR1 for system call interrupts
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (6 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 7/9] target/ppc: Implement HEIR SPR Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 9/9] target/ppc: Better CTRL SPR implementation Nicholas Piggin
2023-05-27 18:05 ` [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Daniel Henrique Barboza
9 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc; +Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza
System call interrupts in ISA v3.1 CPUs add a LEV indication in SRR1
that corresponds with the LEV field of the instruction that caused the
interrupt.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: new patch.
target/ppc/excp_helper.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 529a4513a3..25f9fa9d3f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1559,6 +1559,10 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
vhc->hypercall(cpu->vhyp, cpu);
return;
}
+ if (env->insns_flags2 & PPC2_ISA310) {
+ /* ISAv3.1 puts LEV into SRR1 */
+ msr |= lev << 20;
+ }
if (lev == 1) {
new_msr |= (target_ulong)MSR_HVB;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 9/9] target/ppc: Better CTRL SPR implementation
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (7 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 8/9] target/ppc: Add ISA v3.1 LEV indication in SRR1 for system call interrupts Nicholas Piggin
@ 2023-05-15 9:26 ` Nicholas Piggin
2023-05-27 18:05 ` [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Daniel Henrique Barboza
9 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 9:26 UTC (permalink / raw)
To: qemu-ppc; +Cc: Nicholas Piggin, qemu-devel, Daniel Henrique Barboza
The CTRL register is able to write bit zero, and that is reflected in a
bit field in the register that reflects the state of all threads in the
core.
TCG does not implement SMT, so this just requires mirroring that bit into
the first bit of the thread state field.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v2: unchanged (moved to end of series).
target/ppc/translate.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index c03a6bdc9a..b5b9a0bcaa 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -413,7 +413,14 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
{
- spr_write_generic32(ctx, sprn, gprn);
+ /* This does not implement >1 thread */
+ TCGv t0 = tcg_temp_new();
+ TCGv t1 = tcg_temp_new();
+ tcg_gen_extract_tl(t0, cpu_gpr[gprn], 0, 1); /* Extract RUN field */
+ tcg_gen_shli_tl(t1, t0, 8); /* Duplicate the bit in TS */
+ tcg_gen_or_tl(t1, t1, t0);
+ gen_store_spr(sprn, t1);
+ spr_store_dump_spr(sprn);
/*
* SPR_CTRL writes must force a new translation block,
--
2.40.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 9:26 ` [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs Nicholas Piggin
@ 2023-05-15 10:14 ` Harsh Prateek Bora
2023-05-15 11:14 ` Nicholas Piggin
2023-05-15 12:03 ` Mark Cave-Ayland
1 sibling, 1 reply; 24+ messages in thread
From: Harsh Prateek Bora @ 2023-05-15 10:14 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 5/15/23 14:56, Nicholas Piggin wrote:
> Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
> targets.
>
> This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
> HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
>
> This only goes by the 32/64 classification in the architecture, it
> does not try to implement finer details of SPR implementation (e.g.,
> not all bits implemented as simple read/write storage).
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since v2: no change.
>
> target/ppc/cpu_init.c | 18 +++++++++---------
> target/ppc/helper_regs.c | 2 +-
> target/ppc/misc_helper.c | 4 ++--
> target/ppc/power8-pmu.c | 2 +-
> target/ppc/translate.c | 2 +-
> 5 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 0ce2e3c91d..5aa0b3f0f1 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
> }
>
> spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> - &spr_read_generic, &spr_write_generic,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_VRSAVE, 0x00000000);
>
> }
This change broke linux-user build, could you please check once?
[1776/2718] Compiling C object
libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o
FAILED: libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o
gcc -m64 -mcx16 -Ilibqemu-ppc64le-linux-user.fa.p -I. -I.. -Itarget/ppc
-I../target/ppc -I../common-user/host/x86_64
-I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user
-I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui
-Iui/shader -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto
-Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
/home/travis/build/Harsh-Prateek-Bora/qemu/linux-headers -isystem
linux-headers -iquote . -iquote
/home/travis/build/Harsh-Prateek-Bora/qemu -iquote
/home/travis/build/Harsh-Prateek-Bora/qemu/include -iquote
/home/travis/build/Harsh-Prateek-Bora/qemu/tcg/i386 -pthread
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
-fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes
-Wstrict-prototypes -Wredundant-decls -Wold-style-declaration
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wmissing-format-attribute -Wno-missing-include-dirs
-Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE
-isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H
'-DCONFIG_TARGET="ppc64le-linux-user-config-target.h"'
'-DCONFIG_DEVICES="ppc64le-linux-user-config-devices.h"' -MD -MQ
libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o -MF
libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o.d -o
libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o -c
../target/ppc/cpu_init.c
In file included from ../target/ppc/cpu_init.c:46:
../target/ppc/cpu_init.c: In function ‘register_book3s_altivec_sprs’:
../target/ppc/cpu_init.c:5089:42: error: ‘spr_write_generic32’
undeclared (first use in this function); did you mean ‘spr_write_generic’?
5089 | &spr_read_generic, &spr_write_generic32,
| ^~~~~~~~~~~~~~~~~~~
../target/ppc/spr_common.h:25:24: note: in definition of macro ‘USR_ARG’
25 | # define USR_ARG(X) X,
| ^
../target/ppc/spr_common.h:66:5: note: in expansion of macro
‘spr_register_kvm_hv’
66 | spr_register_kvm_hv(env, num, name, uea_read, uea_write,
oea_read, \
| ^~~~~~~~~~~~~~~~~~~
../target/ppc/cpu_init.c:5088:5: note: in expansion of macro
‘spr_register_kvm’
5088 | spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
| ^~~~~~~~~~~~~~~~
../target/ppc/cpu_init.c:5089:42: note: each undeclared identifier is
reported only once for each function it appears in
5089 | &spr_read_generic, &spr_write_generic32,
| ^~~~~~~~~~~~~~~~~~~
../target/ppc/spr_common.h:25:24: note: in definition of macro ‘USR_ARG’
25 | # define USR_ARG(X) X,
| ^
../target/ppc/spr_common.h:66:5: note: in expansion of macro
‘spr_register_kvm_hv’
66 | spr_register_kvm_hv(env, num, name, uea_read, uea_write,
oea_read, \
| ^~~~~~~~~~~~~~~~~~~
../target/ppc/cpu_init.c:5088:5: note: in expansion of macro
‘spr_register_kvm’
5088 | spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
| ^~~~~~~~~~~~~~~~
[1777/2718] Compiling C object
libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu-models.c.o
[1778/2718] Compiling C object
libqemu-ppc64le-linux-user.fa.p/target_ppc_fpu_helper.c.o
ninja: build stopped: subcommand failed.
make: *** [Makefile:165: run-ninja] Error 1
regards,
Harsh
> @@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
> spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_DAWRX, 0x00000000);
> spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_TSCR, "TSCR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0x00000000);
> spr_register_hv(env, SPR_HMER, "HMER",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_MMCRC, "MMCRC",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0x00000000);
> spr_register_hv(env, SPR_MMCRH, "MMCRH",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_HDSISR, "HDSISR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0x00000000);
> spr_register_hv(env, SPR_HRMOR, "HRMOR",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
> KVM_REG_PPC_ACOP, 0);
> spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_PID, 0);
> #endif
> }
> @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> {
> spr_register(env, SPR_DEXCR, "DEXCR",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0);
>
> spr_register(env, SPR_UDEXCR, "DEXCR",
> @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0);
>
> spr_register(env, SPR_UHDEXCR, "HDEXCR",
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 779e7db513..fb351c303f 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
> /* Exception processing */
> spr_register_kvm(env, SPR_DSISR, "DSISR",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_DSISR, 0x00000000);
> spr_register_kvm(env, SPR_DAR, "DAR",
> SPR_NOACCESS, SPR_NOACCESS,
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index a9bc1522e2..40ddc5c08c 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
>
> void helper_store_pidr(CPUPPCState *env, target_ulong val)
> {
> - env->spr[SPR_BOOKS_PID] = val;
> + env->spr[SPR_BOOKS_PID] = (uint32_t)val;
> tlb_flush(env_cpu(env));
> }
>
> void helper_store_lpidr(CPUPPCState *env, target_ulong val)
> {
> - env->spr[SPR_LPIDR] = val;
> + env->spr[SPR_LPIDR] = (uint32_t)val;
>
> /*
> * We need to flush the TLB on LPID changes as we only tag HV vs
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 1381072b9e..64a64865d7 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> {
> pmu_update_cycles(env);
>
> - env->spr[sprn] = value;
> + env->spr[sprn] = (uint32_t)value;
>
> pmc_update_overflow_timer(env, sprn);
> }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index f603f1a939..c03a6bdc9a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>
> void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> {
> - spr_write_generic(ctx, sprn, gprn);
> + spr_write_generic32(ctx, sprn, gprn);
>
> /*
> * SPR_CTRL writes must force a new translation block,
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 10:14 ` Harsh Prateek Bora
@ 2023-05-15 11:14 ` Nicholas Piggin
2023-05-15 11:43 ` Harsh Prateek Bora
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 11:14 UTC (permalink / raw)
To: Harsh Prateek Bora, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On Mon May 15, 2023 at 8:14 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 5/15/23 14:56, Nicholas Piggin wrote:
> > Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
> > targets.
> >
> > This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
> > HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
> >
> > This only goes by the 32/64 classification in the architecture, it
> > does not try to implement finer details of SPR implementation (e.g.,
> > not all bits implemented as simple read/write storage).
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Since v2: no change.
> >
> > target/ppc/cpu_init.c | 18 +++++++++---------
> > target/ppc/helper_regs.c | 2 +-
> > target/ppc/misc_helper.c | 4 ++--
> > target/ppc/power8-pmu.c | 2 +-
> > target/ppc/translate.c | 2 +-
> > 5 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index 0ce2e3c91d..5aa0b3f0f1 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
> > }
> >
> > spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> > - &spr_read_generic, &spr_write_generic,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_VRSAVE, 0x00000000);
> >
> > }
>
> This change broke linux-user build, could you please check once?
Sorry I did notice you reported that already, must have lost it
along the way somewhere. This incremental patch should work?
Thanks,
Nick
---
diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h
index 8437eb0340..4c0f2bed77 100644
--- a/target/ppc/spr_common.h
+++ b/target/ppc/spr_common.h
@@ -81,6 +81,7 @@ void _spr_register(CPUPPCState *env, int num, const char *name,
void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
+void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
@@ -109,7 +110,6 @@ void spr_write_PMC14_ureg(DisasContext *ctx, int sprn, int gprn);
void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn);
#ifndef CONFIG_USER_ONLY
-void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
void spr_write_clear(DisasContext *ctx, int sprn, int gprn);
void spr_access_nop(DisasContext *ctx, int sprn, int gprn);
void spr_read_decr(DisasContext *ctx, int gprn, int sprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index c03a6bdc9a..f5cf1457db 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -411,6 +411,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
spr_store_dump_spr(sprn);
}
+void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
+{
+#ifdef TARGET_PPC64
+ TCGv t0 = tcg_temp_new();
+ tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]);
+ gen_store_spr(sprn, t0);
+ spr_store_dump_spr(sprn);
+#else
+ spr_write_generic(ctx, sprn, gprn);
+#endif
+}
+
void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
{
spr_write_generic32(ctx, sprn, gprn);
@@ -424,18 +436,6 @@ void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
}
#if !defined(CONFIG_USER_ONLY)
-void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
-{
-#ifdef TARGET_PPC64
- TCGv t0 = tcg_temp_new();
- tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]);
- gen_store_spr(sprn, t0);
- spr_store_dump_spr(sprn);
-#else
- spr_write_generic(ctx, sprn, gprn);
-#endif
-}
-
void spr_write_clear(DisasContext *ctx, int sprn, int gprn)
{
TCGv t0 = tcg_temp_new();
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 11:14 ` Nicholas Piggin
@ 2023-05-15 11:43 ` Harsh Prateek Bora
0 siblings, 0 replies; 24+ messages in thread
From: Harsh Prateek Bora @ 2023-05-15 11:43 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 5/15/23 16:44, Nicholas Piggin wrote:
> On Mon May 15, 2023 at 8:14 PM AEST, Harsh Prateek Bora wrote:
>>
>>
>> On 5/15/23 14:56, Nicholas Piggin wrote:
>>> Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
>>> targets.
>>>
>>> This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
>>> HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
>>>
>>> This only goes by the 32/64 classification in the architecture, it
>>> does not try to implement finer details of SPR implementation (e.g.,
>>> not all bits implemented as simple read/write storage).
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> Since v2: no change.
>>>
>>> target/ppc/cpu_init.c | 18 +++++++++---------
>>> target/ppc/helper_regs.c | 2 +-
>>> target/ppc/misc_helper.c | 4 ++--
>>> target/ppc/power8-pmu.c | 2 +-
>>> target/ppc/translate.c | 2 +-
>>> 5 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>>> index 0ce2e3c91d..5aa0b3f0f1 100644
>>> --- a/target/ppc/cpu_init.c
>>> +++ b/target/ppc/cpu_init.c
>>> @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
>>> }
>>>
>>> spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
>>> - &spr_read_generic, &spr_write_generic,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> + &spr_read_generic, &spr_write_generic32,
>>> KVM_REG_PPC_VRSAVE, 0x00000000);
>>>
>>> }
>>
>> This change broke linux-user build, could you please check once?
>
> Sorry I did notice you reported that already, must have lost it
> along the way somewhere. This incremental patch should work?
>
> Thanks,
> Nick
>
> ---
> diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h
> index 8437eb0340..4c0f2bed77 100644
> --- a/target/ppc/spr_common.h
> +++ b/target/ppc/spr_common.h
> @@ -81,6 +81,7 @@ void _spr_register(CPUPPCState *env, int num, const char *name,
> void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
> void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
> void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
> void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
> void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
> @@ -109,7 +110,6 @@ void spr_write_PMC14_ureg(DisasContext *ctx, int sprn, int gprn);
> void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn);
>
> #ifndef CONFIG_USER_ONLY
> -void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
> void spr_write_clear(DisasContext *ctx, int sprn, int gprn);
> void spr_access_nop(DisasContext *ctx, int sprn, int gprn);
> void spr_read_decr(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index c03a6bdc9a..f5cf1457db 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -411,6 +411,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> spr_store_dump_spr(sprn);
> }
>
> +void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> +{
> +#ifdef TARGET_PPC64
> + TCGv t0 = tcg_temp_new();
> + tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]);
> + gen_store_spr(sprn, t0);
> + spr_store_dump_spr(sprn);
> +#else
> + spr_write_generic(ctx, sprn, gprn);
> +#endif
> +}
> +
> void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> {
> spr_write_generic32(ctx, sprn, gprn);
> @@ -424,18 +436,6 @@ void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> }
>
> #if !defined(CONFIG_USER_ONLY)
> -void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> -{
> -#ifdef TARGET_PPC64
> - TCGv t0 = tcg_temp_new();
> - tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]);
> - gen_store_spr(sprn, t0);
> - spr_store_dump_spr(sprn);
> -#else
> - spr_write_generic(ctx, sprn, gprn);
> -#endif
> -}
> -
> void spr_write_clear(DisasContext *ctx, int sprn, int gprn)
> {
> TCGv t0 = tcg_temp_new();
thanks, this resolves the build break, please squash it in.
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 9:26 ` [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs Nicholas Piggin
2023-05-15 10:14 ` Harsh Prateek Bora
@ 2023-05-15 12:03 ` Mark Cave-Ayland
2023-05-15 12:51 ` Nicholas Piggin
2023-05-15 15:19 ` Nicholas Piggin
1 sibling, 2 replies; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-05-15 12:03 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 15/05/2023 10:26, Nicholas Piggin wrote:
> Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
> targets.
>
> This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
> HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
>
> This only goes by the 32/64 classification in the architecture, it
> does not try to implement finer details of SPR implementation (e.g.,
> not all bits implemented as simple read/write storage).
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since v2: no change.
>
> target/ppc/cpu_init.c | 18 +++++++++---------
> target/ppc/helper_regs.c | 2 +-
> target/ppc/misc_helper.c | 4 ++--
> target/ppc/power8-pmu.c | 2 +-
> target/ppc/translate.c | 2 +-
> 5 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 0ce2e3c91d..5aa0b3f0f1 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
> }
>
> spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> - &spr_read_generic, &spr_write_generic,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_VRSAVE, 0x00000000);
>
> }
> @@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
> spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_DAWRX, 0x00000000);
> spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_TSCR, "TSCR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0x00000000);
> spr_register_hv(env, SPR_HMER, "HMER",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_MMCRC, "MMCRC",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0x00000000);
> spr_register_hv(env, SPR_MMCRH, "MMCRH",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_HDSISR, "HDSISR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0x00000000);
> spr_register_hv(env, SPR_HRMOR, "HRMOR",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
> KVM_REG_PPC_ACOP, 0);
> spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_PID, 0);
> #endif
> }
> @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> {
> spr_register(env, SPR_DEXCR, "DEXCR",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0);
>
> spr_register(env, SPR_UDEXCR, "DEXCR",
> @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> 0);
>
> spr_register(env, SPR_UHDEXCR, "HDEXCR",
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 779e7db513..fb351c303f 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
> /* Exception processing */
> spr_register_kvm(env, SPR_DSISR, "DSISR",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_generic32,
> KVM_REG_PPC_DSISR, 0x00000000);
> spr_register_kvm(env, SPR_DAR, "DAR",
> SPR_NOACCESS, SPR_NOACCESS,
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index a9bc1522e2..40ddc5c08c 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
>
> void helper_store_pidr(CPUPPCState *env, target_ulong val)
> {
> - env->spr[SPR_BOOKS_PID] = val;
> + env->spr[SPR_BOOKS_PID] = (uint32_t)val;
> tlb_flush(env_cpu(env));
> }
>
> void helper_store_lpidr(CPUPPCState *env, target_ulong val)
> {
> - env->spr[SPR_LPIDR] = val;
> + env->spr[SPR_LPIDR] = (uint32_t)val;
>
> /*
> * We need to flush the TLB on LPID changes as we only tag HV vs
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 1381072b9e..64a64865d7 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> {
> pmu_update_cycles(env);
>
> - env->spr[sprn] = value;
> + env->spr[sprn] = (uint32_t)value;
>
> pmc_update_overflow_timer(env, sprn);
> }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index f603f1a939..c03a6bdc9a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>
> void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> {
> - spr_write_generic(ctx, sprn, gprn);
> + spr_write_generic32(ctx, sprn, gprn);
>
> /*
> * SPR_CTRL writes must force a new translation block,
Just out of curiosity, is this the same as the problem described at [1] for DECAR?
ATB,
Mark.
[1] https://lists.nongnu.org/archive/html/qemu-ppc/2023-03/msg00451.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 12:03 ` Mark Cave-Ayland
@ 2023-05-15 12:51 ` Nicholas Piggin
2023-05-15 15:19 ` Nicholas Piggin
1 sibling, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 12:51 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On Mon May 15, 2023 at 10:03 PM AEST, Mark Cave-Ayland wrote:
> On 15/05/2023 10:26, Nicholas Piggin wrote:
>
> > Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
> > targets.
> >
> > This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
> > HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
> >
> > This only goes by the 32/64 classification in the architecture, it
> > does not try to implement finer details of SPR implementation (e.g.,
> > not all bits implemented as simple read/write storage).
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Since v2: no change.
> >
> > target/ppc/cpu_init.c | 18 +++++++++---------
> > target/ppc/helper_regs.c | 2 +-
> > target/ppc/misc_helper.c | 4 ++--
> > target/ppc/power8-pmu.c | 2 +-
> > target/ppc/translate.c | 2 +-
> > 5 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index 0ce2e3c91d..5aa0b3f0f1 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
> > }
> >
> > spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> > - &spr_read_generic, &spr_write_generic,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_VRSAVE, 0x00000000);
> >
> > }
> > @@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
> > spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_DAWRX, 0x00000000);
> > spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_TSCR, "TSCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0x00000000);
> > spr_register_hv(env, SPR_HMER, "HMER",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_MMCRC, "MMCRC",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0x00000000);
> > spr_register_hv(env, SPR_MMCRH, "MMCRH",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_HDSISR, "HDSISR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0x00000000);
> > spr_register_hv(env, SPR_HRMOR, "HRMOR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
> > KVM_REG_PPC_ACOP, 0);
> > spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_PID, 0);
> > #endif
> > }
> > @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> > {
> > spr_register(env, SPR_DEXCR, "DEXCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0);
> >
> > spr_register(env, SPR_UDEXCR, "DEXCR",
> > @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0);
> >
> > spr_register(env, SPR_UHDEXCR, "HDEXCR",
> > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> > index 779e7db513..fb351c303f 100644
> > --- a/target/ppc/helper_regs.c
> > +++ b/target/ppc/helper_regs.c
> > @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
> > /* Exception processing */
> > spr_register_kvm(env, SPR_DSISR, "DSISR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_DSISR, 0x00000000);
> > spr_register_kvm(env, SPR_DAR, "DAR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> > index a9bc1522e2..40ddc5c08c 100644
> > --- a/target/ppc/misc_helper.c
> > +++ b/target/ppc/misc_helper.c
> > @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
> >
> > void helper_store_pidr(CPUPPCState *env, target_ulong val)
> > {
> > - env->spr[SPR_BOOKS_PID] = val;
> > + env->spr[SPR_BOOKS_PID] = (uint32_t)val;
> > tlb_flush(env_cpu(env));
> > }
> >
> > void helper_store_lpidr(CPUPPCState *env, target_ulong val)
> > {
> > - env->spr[SPR_LPIDR] = val;
> > + env->spr[SPR_LPIDR] = (uint32_t)val;
> >
> > /*
> > * We need to flush the TLB on LPID changes as we only tag HV vs
> > diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> > index 1381072b9e..64a64865d7 100644
> > --- a/target/ppc/power8-pmu.c
> > +++ b/target/ppc/power8-pmu.c
> > @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> > {
> > pmu_update_cycles(env);
> >
> > - env->spr[sprn] = value;
> > + env->spr[sprn] = (uint32_t)value;
> >
> > pmc_update_overflow_timer(env, sprn);
> > }
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index f603f1a939..c03a6bdc9a 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> >
> > void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> > {
> > - spr_write_generic(ctx, sprn, gprn);
> > + spr_write_generic32(ctx, sprn, gprn);
> >
> > /*
> > * SPR_CTRL writes must force a new translation block,
>
> Just out of curiosity, is this the same as the problem described at [1] for DECAR?
>
>
> ATB,
>
> Mark.
>
> [1] https://lists.nongnu.org/archive/html/qemu-ppc/2023-03/msg00451.html
Hmm, good thinking. But I'm not sure. DECAR is 32-bits so it should
probably use spr_write_generic32 (I didn't convert any BookE specific
regs). But the test sets 32-bit value for DECAR so I'm not sure if
that would be the issue. That said I don't see where the value is
getting sign extended.
Thanks,
Nick
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 12:03 ` Mark Cave-Ayland
2023-05-15 12:51 ` Nicholas Piggin
@ 2023-05-15 15:19 ` Nicholas Piggin
2023-05-16 7:02 ` Mark Cave-Ayland
1 sibling, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-15 15:19 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On Mon May 15, 2023 at 10:03 PM AEST, Mark Cave-Ayland wrote:
> On 15/05/2023 10:26, Nicholas Piggin wrote:
>
> > Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
> > targets.
> >
> > This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
> > HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
> >
> > This only goes by the 32/64 classification in the architecture, it
> > does not try to implement finer details of SPR implementation (e.g.,
> > not all bits implemented as simple read/write storage).
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Since v2: no change.
> >
> > target/ppc/cpu_init.c | 18 +++++++++---------
> > target/ppc/helper_regs.c | 2 +-
> > target/ppc/misc_helper.c | 4 ++--
> > target/ppc/power8-pmu.c | 2 +-
> > target/ppc/translate.c | 2 +-
> > 5 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index 0ce2e3c91d..5aa0b3f0f1 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
> > }
> >
> > spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> > - &spr_read_generic, &spr_write_generic,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_VRSAVE, 0x00000000);
> >
> > }
> > @@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
> > spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_DAWRX, 0x00000000);
> > spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_TSCR, "TSCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0x00000000);
> > spr_register_hv(env, SPR_HMER, "HMER",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_MMCRC, "MMCRC",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0x00000000);
> > spr_register_hv(env, SPR_MMCRH, "MMCRH",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_HDSISR, "HDSISR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0x00000000);
> > spr_register_hv(env, SPR_HRMOR, "HRMOR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
> > KVM_REG_PPC_ACOP, 0);
> > spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_PID, 0);
> > #endif
> > }
> > @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> > {
> > spr_register(env, SPR_DEXCR, "DEXCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0);
> >
> > spr_register(env, SPR_UDEXCR, "DEXCR",
> > @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > 0);
> >
> > spr_register(env, SPR_UHDEXCR, "HDEXCR",
> > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> > index 779e7db513..fb351c303f 100644
> > --- a/target/ppc/helper_regs.c
> > +++ b/target/ppc/helper_regs.c
> > @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
> > /* Exception processing */
> > spr_register_kvm(env, SPR_DSISR, "DSISR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - &spr_read_generic, &spr_write_generic,
> > + &spr_read_generic, &spr_write_generic32,
> > KVM_REG_PPC_DSISR, 0x00000000);
> > spr_register_kvm(env, SPR_DAR, "DAR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> > index a9bc1522e2..40ddc5c08c 100644
> > --- a/target/ppc/misc_helper.c
> > +++ b/target/ppc/misc_helper.c
> > @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
> >
> > void helper_store_pidr(CPUPPCState *env, target_ulong val)
> > {
> > - env->spr[SPR_BOOKS_PID] = val;
> > + env->spr[SPR_BOOKS_PID] = (uint32_t)val;
> > tlb_flush(env_cpu(env));
> > }
> >
> > void helper_store_lpidr(CPUPPCState *env, target_ulong val)
> > {
> > - env->spr[SPR_LPIDR] = val;
> > + env->spr[SPR_LPIDR] = (uint32_t)val;
> >
> > /*
> > * We need to flush the TLB on LPID changes as we only tag HV vs
> > diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> > index 1381072b9e..64a64865d7 100644
> > --- a/target/ppc/power8-pmu.c
> > +++ b/target/ppc/power8-pmu.c
> > @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> > {
> > pmu_update_cycles(env);
> >
> > - env->spr[sprn] = value;
> > + env->spr[sprn] = (uint32_t)value;
> >
> > pmc_update_overflow_timer(env, sprn);
> > }
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index f603f1a939..c03a6bdc9a 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> >
> > void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> > {
> > - spr_write_generic(ctx, sprn, gprn);
> > + spr_write_generic32(ctx, sprn, gprn);
> >
> > /*
> > * SPR_CTRL writes must force a new translation block,
>
> Just out of curiosity, is this the same as the problem described at [1] for DECAR?
>
>
> ATB,
>
> Mark.
>
> [1] https://lists.nongnu.org/archive/html/qemu-ppc/2023-03/msg00451.html
Oh if it's a 64-bit target running in 32-bit mode, then the compiled
code might use something like li reg,-1 to set the 0xffffffff value,
but that gets sign extended to 64-bits. Storing that to DECAR then
does cause it to get stored to DECR. So DECAR should use
spr_write_generic32.
But all the store_decr calculations are unsigned and DECR gets clamped
to 32-bits, at least when reading it back. The problem seems to be the
timer ends up getting set for a negative expire time.
So storing to DECR directly seems like it would have the same problems
as via DECAR. This should help.
Thanks,
Nick
---
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 4e816c68c7..35a1410c4d 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -794,14 +794,18 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
CPUPPCState *env = &cpu->env;
ppc_tb_t *tb_env = env->tb_env;
uint64_t now, next;
+ uint64_t unsigned_value;
+ uint64_t unsigned_decr;
int64_t signed_value;
int64_t signed_decr;
/* Truncate value to decr_width and sign extend for simplicity */
+ unsigned_value = extract64(value, 0, nr_bits);
+ unsigned_decr = extract64(decr, 0, nr_bits);
signed_value = sextract64(value, 0, nr_bits);
signed_decr = sextract64(decr, 0, nr_bits);
- trace_ppc_decr_store(nr_bits, decr, value);
+ trace_ppc_decr_store(nr_bits, unsigned_decr, unsigned_value);
if (kvm_enabled()) {
/* KVM handles decrementer exceptions, we don't need our own timer */
@@ -821,7 +825,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
* On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
* an edge interrupt, so raise it here too.
*/
- if ((value < 3) ||
+ if ((unsigned_value < 3) ||
((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
&& signed_decr >= 0)) {
@@ -836,7 +840,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
/* Calculate the next timer event */
now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- next = now + muldiv64(value, NANOSECONDS_PER_SECOND, tb_env->decr_freq);
+ next = now + muldiv64(unsigned_value, NANOSECONDS_PER_SECOND,
+ tb_env->decr_freq);
*nextp = next;
/* Adjust timer */
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-15 15:19 ` Nicholas Piggin
@ 2023-05-16 7:02 ` Mark Cave-Ayland
2023-05-16 9:39 ` Nicholas Piggin
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-05-16 7:02 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza, sdicaro
On 15/05/2023 16:19, Nicholas Piggin wrote:
> On Mon May 15, 2023 at 10:03 PM AEST, Mark Cave-Ayland wrote:
>> On 15/05/2023 10:26, Nicholas Piggin wrote:
>>
>>> Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
>>> targets.
>>>
>>> This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
>>> HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
>>>
>>> This only goes by the 32/64 classification in the architecture, it
>>> does not try to implement finer details of SPR implementation (e.g.,
>>> not all bits implemented as simple read/write storage).
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> Since v2: no change.
>>>
>>> target/ppc/cpu_init.c | 18 +++++++++---------
>>> target/ppc/helper_regs.c | 2 +-
>>> target/ppc/misc_helper.c | 4 ++--
>>> target/ppc/power8-pmu.c | 2 +-
>>> target/ppc/translate.c | 2 +-
>>> 5 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>>> index 0ce2e3c91d..5aa0b3f0f1 100644
>>> --- a/target/ppc/cpu_init.c
>>> +++ b/target/ppc/cpu_init.c
>>> @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
>>> }
>>>
>>> spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
>>> - &spr_read_generic, &spr_write_generic,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> + &spr_read_generic, &spr_write_generic32,
>>> KVM_REG_PPC_VRSAVE, 0x00000000);
>>>
>>> }
>>> @@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
>>> spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> KVM_REG_PPC_DAWRX, 0x00000000);
>>> spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
>>> spr_register_hv(env, SPR_TSCR, "TSCR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> 0x00000000);
>>> spr_register_hv(env, SPR_HMER, "HMER",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
>>> spr_register_hv(env, SPR_MMCRC, "MMCRC",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> 0x00000000);
>>> spr_register_hv(env, SPR_MMCRH, "MMCRH",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
>>> spr_register_hv(env, SPR_HDSISR, "HDSISR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> 0x00000000);
>>> spr_register_hv(env, SPR_HRMOR, "HRMOR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
>>> KVM_REG_PPC_ACOP, 0);
>>> spr_register_kvm(env, SPR_BOOKS_PID, "PID",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> KVM_REG_PPC_PID, 0);
>>> #endif
>>> }
>>> @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
>>> {
>>> spr_register(env, SPR_DEXCR, "DEXCR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> 0);
>>>
>>> spr_register(env, SPR_UDEXCR, "DEXCR",
>>> @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
>>> spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> 0);
>>>
>>> spr_register(env, SPR_UHDEXCR, "HDEXCR",
>>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
>>> index 779e7db513..fb351c303f 100644
>>> --- a/target/ppc/helper_regs.c
>>> +++ b/target/ppc/helper_regs.c
>>> @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
>>> /* Exception processing */
>>> spr_register_kvm(env, SPR_DSISR, "DSISR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> - &spr_read_generic, &spr_write_generic,
>>> + &spr_read_generic, &spr_write_generic32,
>>> KVM_REG_PPC_DSISR, 0x00000000);
>>> spr_register_kvm(env, SPR_DAR, "DAR",
>>> SPR_NOACCESS, SPR_NOACCESS,
>>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>>> index a9bc1522e2..40ddc5c08c 100644
>>> --- a/target/ppc/misc_helper.c
>>> +++ b/target/ppc/misc_helper.c
>>> @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
>>>
>>> void helper_store_pidr(CPUPPCState *env, target_ulong val)
>>> {
>>> - env->spr[SPR_BOOKS_PID] = val;
>>> + env->spr[SPR_BOOKS_PID] = (uint32_t)val;
>>> tlb_flush(env_cpu(env));
>>> }
>>>
>>> void helper_store_lpidr(CPUPPCState *env, target_ulong val)
>>> {
>>> - env->spr[SPR_LPIDR] = val;
>>> + env->spr[SPR_LPIDR] = (uint32_t)val;
>>>
>>> /*
>>> * We need to flush the TLB on LPID changes as we only tag HV vs
>>> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
>>> index 1381072b9e..64a64865d7 100644
>>> --- a/target/ppc/power8-pmu.c
>>> +++ b/target/ppc/power8-pmu.c
>>> @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
>>> {
>>> pmu_update_cycles(env);
>>>
>>> - env->spr[sprn] = value;
>>> + env->spr[sprn] = (uint32_t)value;
>>>
>>> pmc_update_overflow_timer(env, sprn);
>>> }
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index f603f1a939..c03a6bdc9a 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>>>
>>> void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
>>> {
>>> - spr_write_generic(ctx, sprn, gprn);
>>> + spr_write_generic32(ctx, sprn, gprn);
>>>
>>> /*
>>> * SPR_CTRL writes must force a new translation block,
>>
>> Just out of curiosity, is this the same as the problem described at [1] for DECAR?
>>
>>
>> ATB,
>>
>> Mark.
>>
>> [1] https://lists.nongnu.org/archive/html/qemu-ppc/2023-03/msg00451.html
>
> Oh if it's a 64-bit target running in 32-bit mode, then the compiled
> code might use something like li reg,-1 to set the 0xffffffff value,
> but that gets sign extended to 64-bits. Storing that to DECAR then
> does cause it to get stored to DECR. So DECAR should use
> spr_write_generic32.
>
> But all the store_decr calculations are unsigned and DECR gets clamped
> to 32-bits, at least when reading it back. The problem seems to be the
> timer ends up getting set for a negative expire time.
>
> So storing to DECR directly seems like it would have the same problems
> as via DECAR. This should help.
>
> Thanks,
> Nick
> ---
>
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 4e816c68c7..35a1410c4d 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -794,14 +794,18 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
> CPUPPCState *env = &cpu->env;
> ppc_tb_t *tb_env = env->tb_env;
> uint64_t now, next;
> + uint64_t unsigned_value;
> + uint64_t unsigned_decr;
> int64_t signed_value;
> int64_t signed_decr;
>
> /* Truncate value to decr_width and sign extend for simplicity */
> + unsigned_value = extract64(value, 0, nr_bits);
> + unsigned_decr = extract64(decr, 0, nr_bits);
> signed_value = sextract64(value, 0, nr_bits);
> signed_decr = sextract64(decr, 0, nr_bits);
>
> - trace_ppc_decr_store(nr_bits, decr, value);
> + trace_ppc_decr_store(nr_bits, unsigned_decr, unsigned_value);
>
> if (kvm_enabled()) {
> /* KVM handles decrementer exceptions, we don't need our own timer */
> @@ -821,7 +825,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
> * On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
> * an edge interrupt, so raise it here too.
> */
> - if ((value < 3) ||
> + if ((unsigned_value < 3) ||
> ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
> ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
> && signed_decr >= 0)) {
> @@ -836,7 +840,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
>
> /* Calculate the next timer event */
> now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - next = now + muldiv64(value, NANOSECONDS_PER_SECOND, tb_env->decr_freq);
> + next = now + muldiv64(unsigned_value, NANOSECONDS_PER_SECOND,
> + tb_env->decr_freq);
> *nextp = next;
>
> /* Adjust timer */
Thanks Nick! I've added the original reporter on CC to see if they can provide
testing and feedback.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation
2023-05-15 9:26 ` [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation Nicholas Piggin
@ 2023-05-16 9:32 ` Daniel Henrique Barboza
2023-05-16 10:44 ` Nicholas Piggin
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-16 9:32 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 5/15/23 06:26, Nicholas Piggin wrote:
> A store to MMCR0 with PMCjCE=1 fails to update hflags correctly and
> results in hflags mismatch:
>
> qemu: fatal: TCG hflags mismatch (current:0x2408003d rebuilt:0x240a003d)
>
> This can be reproduced by running perf on a recent machine.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
Fixes: c2eff582a32f ("target/ppc: PMU basic cycle count for pseries TCG")
(not sure why I didn't hit this back in 2021)
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Since v2: new patch.
>
> target/ppc/power8-pmu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 64a64865d7..29e0012ed6 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -236,14 +236,16 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> {
> bool hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
> bool hflags_pmcc1 = (value & MMCR0_PMCC1) != 0;
> + bool hflags_pmcjce = (value & MMCR0_PMCjCE) != 0;
>
> pmu_update_cycles(env);
>
> env->spr[SPR_POWER_MMCR0] = value;
>
> - /* MMCR0 writes can change HFLAGS_PMCC[01] and HFLAGS_INSN_CNT */
> + /* MMCR0 writes can change HFLAGS_PMCC[01], PMCjCE, and HFLAGS_INSN_CNT */
> env->hflags = deposit32(env->hflags, HFLAGS_PMCC0, 1, hflags_pmcc0);
> env->hflags = deposit32(env->hflags, HFLAGS_PMCC1, 1, hflags_pmcc1);
> + env->hflags = deposit32(env->hflags, HFLAGS_PMCJCE, 1, hflags_pmcjce);
>
> pmu_update_summaries(env);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
2023-05-16 7:02 ` Mark Cave-Ayland
@ 2023-05-16 9:39 ` Nicholas Piggin
0 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-16 9:39 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza, sdicaro
On Tue May 16, 2023 at 5:02 PM AEST, Mark Cave-Ayland wrote:
> On 15/05/2023 16:19, Nicholas Piggin wrote:
>
> > On Mon May 15, 2023 at 10:03 PM AEST, Mark Cave-Ayland wrote:
> >> On 15/05/2023 10:26, Nicholas Piggin wrote:
> >>
> >>> Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
> >>> targets.
> >>>
> >>> This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
> >>> HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.
> >>>
> >>> This only goes by the 32/64 classification in the architecture, it
> >>> does not try to implement finer details of SPR implementation (e.g.,
> >>> not all bits implemented as simple read/write storage).
> >>>
> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >>> ---
> >>> Since v2: no change.
> >>>
> >>> target/ppc/cpu_init.c | 18 +++++++++---------
> >>> target/ppc/helper_regs.c | 2 +-
> >>> target/ppc/misc_helper.c | 4 ++--
> >>> target/ppc/power8-pmu.c | 2 +-
> >>> target/ppc/translate.c | 2 +-
> >>> 5 files changed, 14 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> >>> index 0ce2e3c91d..5aa0b3f0f1 100644
> >>> --- a/target/ppc/cpu_init.c
> >>> +++ b/target/ppc/cpu_init.c
> >>> @@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
> >>> }
> >>>
> >>> spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> >>> - &spr_read_generic, &spr_write_generic,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> KVM_REG_PPC_VRSAVE, 0x00000000);
> >>>
> >>> }
> >>> @@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
> >>> spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> KVM_REG_PPC_DAWRX, 0x00000000);
> >>> spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> >>> spr_register_hv(env, SPR_TSCR, "TSCR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> 0x00000000);
> >>> spr_register_hv(env, SPR_HMER, "HMER",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> >>> spr_register_hv(env, SPR_MMCRC, "MMCRC",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> 0x00000000);
> >>> spr_register_hv(env, SPR_MMCRH, "MMCRH",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
> >>> spr_register_hv(env, SPR_HDSISR, "HDSISR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> 0x00000000);
> >>> spr_register_hv(env, SPR_HRMOR, "HRMOR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
> >>> KVM_REG_PPC_ACOP, 0);
> >>> spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> KVM_REG_PPC_PID, 0);
> >>> #endif
> >>> }
> >>> @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> >>> {
> >>> spr_register(env, SPR_DEXCR, "DEXCR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> 0);
> >>>
> >>> spr_register(env, SPR_UDEXCR, "DEXCR",
> >>> @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
> >>> spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> 0);
> >>>
> >>> spr_register(env, SPR_UHDEXCR, "HDEXCR",
> >>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> >>> index 779e7db513..fb351c303f 100644
> >>> --- a/target/ppc/helper_regs.c
> >>> +++ b/target/ppc/helper_regs.c
> >>> @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
> >>> /* Exception processing */
> >>> spr_register_kvm(env, SPR_DSISR, "DSISR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> - &spr_read_generic, &spr_write_generic,
> >>> + &spr_read_generic, &spr_write_generic32,
> >>> KVM_REG_PPC_DSISR, 0x00000000);
> >>> spr_register_kvm(env, SPR_DAR, "DAR",
> >>> SPR_NOACCESS, SPR_NOACCESS,
> >>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> >>> index a9bc1522e2..40ddc5c08c 100644
> >>> --- a/target/ppc/misc_helper.c
> >>> +++ b/target/ppc/misc_helper.c
> >>> @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
> >>>
> >>> void helper_store_pidr(CPUPPCState *env, target_ulong val)
> >>> {
> >>> - env->spr[SPR_BOOKS_PID] = val;
> >>> + env->spr[SPR_BOOKS_PID] = (uint32_t)val;
> >>> tlb_flush(env_cpu(env));
> >>> }
> >>>
> >>> void helper_store_lpidr(CPUPPCState *env, target_ulong val)
> >>> {
> >>> - env->spr[SPR_LPIDR] = val;
> >>> + env->spr[SPR_LPIDR] = (uint32_t)val;
> >>>
> >>> /*
> >>> * We need to flush the TLB on LPID changes as we only tag HV vs
> >>> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> >>> index 1381072b9e..64a64865d7 100644
> >>> --- a/target/ppc/power8-pmu.c
> >>> +++ b/target/ppc/power8-pmu.c
> >>> @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> >>> {
> >>> pmu_update_cycles(env);
> >>>
> >>> - env->spr[sprn] = value;
> >>> + env->spr[sprn] = (uint32_t)value;
> >>>
> >>> pmc_update_overflow_timer(env, sprn);
> >>> }
> >>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> >>> index f603f1a939..c03a6bdc9a 100644
> >>> --- a/target/ppc/translate.c
> >>> +++ b/target/ppc/translate.c
> >>> @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> >>>
> >>> void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> >>> {
> >>> - spr_write_generic(ctx, sprn, gprn);
> >>> + spr_write_generic32(ctx, sprn, gprn);
> >>>
> >>> /*
> >>> * SPR_CTRL writes must force a new translation block,
> >>
> >> Just out of curiosity, is this the same as the problem described at [1] for DECAR?
> >>
> >>
> >> ATB,
> >>
> >> Mark.
> >>
> >> [1] https://lists.nongnu.org/archive/html/qemu-ppc/2023-03/msg00451.html
> >
> > Oh if it's a 64-bit target running in 32-bit mode, then the compiled
> > code might use something like li reg,-1 to set the 0xffffffff value,
> > but that gets sign extended to 64-bits. Storing that to DECAR then
> > does cause it to get stored to DECR. So DECAR should use
> > spr_write_generic32.
> >
> > But all the store_decr calculations are unsigned and DECR gets clamped
> > to 32-bits, at least when reading it back. The problem seems to be the
> > timer ends up getting set for a negative expire time.
> >
> > So storing to DECR directly seems like it would have the same problems
> > as via DECAR. This should help.
> >
> > Thanks,
> > Nick
> > ---
> >
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index 4e816c68c7..35a1410c4d 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -794,14 +794,18 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
> > CPUPPCState *env = &cpu->env;
> > ppc_tb_t *tb_env = env->tb_env;
> > uint64_t now, next;
> > + uint64_t unsigned_value;
> > + uint64_t unsigned_decr;
> > int64_t signed_value;
> > int64_t signed_decr;
> >
> > /* Truncate value to decr_width and sign extend for simplicity */
> > + unsigned_value = extract64(value, 0, nr_bits);
> > + unsigned_decr = extract64(decr, 0, nr_bits);
> > signed_value = sextract64(value, 0, nr_bits);
> > signed_decr = sextract64(decr, 0, nr_bits);
> >
> > - trace_ppc_decr_store(nr_bits, decr, value);
> > + trace_ppc_decr_store(nr_bits, unsigned_decr, unsigned_value);
> >
> > if (kvm_enabled()) {
> > /* KVM handles decrementer exceptions, we don't need our own timer */
> > @@ -821,7 +825,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
> > * On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
> > * an edge interrupt, so raise it here too.
> > */
> > - if ((value < 3) ||
> > + if ((unsigned_value < 3) ||
> > ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
> > ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
> > && signed_decr >= 0)) {
> > @@ -836,7 +840,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
> >
> > /* Calculate the next timer event */
> > now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > - next = now + muldiv64(value, NANOSECONDS_PER_SECOND, tb_env->decr_freq);
> > + next = now + muldiv64(unsigned_value, NANOSECONDS_PER_SECOND,
> > + tb_env->decr_freq);
> > *nextp = next;
> >
> > /* Adjust timer */
>
> Thanks Nick! I've added the original reporter on CC to see if they can provide
> testing and feedback.
Oops, thanks. To be clear making DECAR a 32-bit register did also solve
the problem I could reproduce by avoiding that sign extended GPR setting
it to -1LL. But if the test case didn't use DECAR instead wrote -1 to
DEC in the interrupt handler, I think the above patch is needed. So we
should do both patches (assuming this fixes for sdicaro@).
Thanks,
Nick
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation
2023-05-16 9:32 ` Daniel Henrique Barboza
@ 2023-05-16 10:44 ` Nicholas Piggin
2023-05-16 11:07 ` Daniel Henrique Barboza
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-16 10:44 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On Tue May 16, 2023 at 7:32 PM AEST, Daniel Henrique Barboza wrote:
>
>
> On 5/15/23 06:26, Nicholas Piggin wrote:
> > A store to MMCR0 with PMCjCE=1 fails to update hflags correctly and
> > results in hflags mismatch:
> >
> > qemu: fatal: TCG hflags mismatch (current:0x2408003d rebuilt:0x240a003d)
> >
> > This can be reproduced by running perf on a recent machine.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
>
> Fixes: c2eff582a32f ("target/ppc: PMU basic cycle count for pseries TCG")
Or is it this one? 0625c7760d54 ("target/ppc: do not call
hreg_compute_hflags() in helper_store_mmcr0()")
Ah, neither! It looks like 8b3d1c49a9f0 ("target/ppc: Add new PMC
HFLAGS"). But that shows I have probably missed HFLAGS_PMC_OTHER
here.
Let me do a bit more investigation and send an updated patch if
necessary.
Thanks,
Nick
>
> (not sure why I didn't hit this back in 2021)
>
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation
2023-05-16 10:44 ` Nicholas Piggin
@ 2023-05-16 11:07 ` Daniel Henrique Barboza
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-16 11:07 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 5/16/23 07:44, Nicholas Piggin wrote:
> On Tue May 16, 2023 at 7:32 PM AEST, Daniel Henrique Barboza wrote:
>>
>>
>> On 5/15/23 06:26, Nicholas Piggin wrote:
>>> A store to MMCR0 with PMCjCE=1 fails to update hflags correctly and
>>> results in hflags mismatch:
>>>
>>> qemu: fatal: TCG hflags mismatch (current:0x2408003d rebuilt:0x240a003d)
>>>
>>> This can be reproduced by running perf on a recent machine.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>
>> Fixes: c2eff582a32f ("target/ppc: PMU basic cycle count for pseries TCG")
>
> Or is it this one? 0625c7760d54 ("target/ppc: do not call
> hreg_compute_hflags() in helper_store_mmcr0()")
>
> Ah, neither! It looks like 8b3d1c49a9f0 ("target/ppc: Add new PMC
> HFLAGS"). But that shows I have probably missed HFLAGS_PMC_OTHER
> here.
>
> Let me do a bit more investigation and send an updated patch if
> necessary.
Sure, let's hold this one for now.
Daniel
>
> Thanks,
> Nick
>
>>
>> (not sure why I didn't hit this back in 2021)
>>
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt
2023-05-15 9:26 ` [PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt Nicholas Piggin
@ 2023-05-16 15:46 ` Daniel Henrique Barboza
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-16 15:46 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc
Cc: qemu-devel, Daniel Henrique Barboza, Fabiano Rosas
Either this patch or patch 6 broke a gitlab KVM builder (cross-ppc64el-kvm-only)
as follows:
[1441/2019] Compiling C object libqemu-ppc-softmmu.fa.p/target_ppc_excp_helper.c.o
FAILED: libqemu-ppc-softmmu.fa.p/target_ppc_excp_helper.c.o
powerpc64le-linux-gnu-gcc -m64 -mlittle-endian -Ilibqemu-ppc-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/capstone -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/glib-2.0 -I/usr/lib/powerpc64le-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc-softmmu-config-devices.h"' -MD -MQ libqemu-ppc-softmmu.fa.p/target_ppc_excp_helper.c.o -MF libqemu-ppc-softmmu.fa.p/target_ppc_excp_helper.c.o.d -o libqemu-ppc-softmmu.fa.p/target_ppc_excp_helper.c.o -c ../target/ppc/excp_helper.c
../target/ppc/excp_helper.c:143:49: error: unknown type name ‘abi_ptr’; did you mean ‘si_ptr’?
143 | static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr)
| ^~~~~~~
| si_ptr
[1442/2019] Compiling C object libqemu-ppc-softmmu.fa.p/target_ppc_cpu-models.c.o
ninja: build stopped: subcommand failed.
make: *** [Makefile:165: run-ninja] Error 1
More details here:
https://gitlab.com/danielhb/qemu/-/jobs/4293326431
I suppose we're missing an ifdef somewhere to gate this code from KVM code. 'abi_ptr'
is a TCG pointer afaik.
Thanks,
Daniel
On 5/15/23 06:26, Nicholas Piggin wrote:
> powerpc ifetch endianness depends on MSR[LE] so it has to byteswap
> after cpu_ldl_code(). This corrects DSISR bits in alignment
> interrupts when running in little endian mode.
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since v2: no change.
>
> target/ppc/excp_helper.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 199328f4b6..bc2be4a726 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -133,6 +133,24 @@ static void dump_hcall(CPUPPCState *env)
> env->nip);
> }
>
> +/* Return true iff byteswap is needed in a scalar memop */
> +static inline bool need_byteswap(CPUArchState *env)
> +{
> + /* SOFTMMU builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
> + return !!(env->msr & ((target_ulong)1 << MSR_LE));
> +}
> +
> +static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr)
> +{
> + uint32_t insn = cpu_ldl_code(env, addr);
> +
> + if (need_byteswap(env)) {
> + insn = bswap32(insn);
> + }
> +
> + return insn;
> +}
> +
> static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
> {
> const char *es;
> @@ -3097,7 +3115,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>
> /* Restore state and reload the insn we executed, for filling in DSISR. */
> cpu_restore_state(cs, retaddr);
> - insn = cpu_ldl_code(env, env->nip);
> + insn = ppc_ldl_code(env, env->nip);
>
> switch (env->mmu_model) {
> case POWERPC_MMU_SOFT_4xx:
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/9] target/ppc: Assorted ppc target fixes
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
` (8 preceding siblings ...)
2023-05-15 9:26 ` [PATCH v3 9/9] target/ppc: Better CTRL SPR implementation Nicholas Piggin
@ 2023-05-27 18:05 ` Daniel Henrique Barboza
2023-05-29 1:47 ` Nicholas Piggin
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-27 18:05 UTC (permalink / raw)
To: Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 5/15/23 06:26, Nicholas Piggin wrote:
> Hopefully these are getting close to ready now. There is still the
> question about doing better with adding test cases for all this, I
> haven't exactly got a good answer yet but I do have kvm-unit-tests
> for most at least.
Patches 1 and 4 queued to ppc-next. Thanks,
Daniel
>
> Thanks,
> Nick
>
> Nicholas Piggin (9):
> target/ppc: Fix width of some 32-bit SPRs
> target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation
> target/ppc: Fix instruction loading endianness in alignment interrupt
> target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward
> target/ppc: Change partition-scope translate interface
> target/ppc: Add SRR1 prefix indication to interrupt handlers
> target/ppc: Implement HEIR SPR
> target/ppc: Add ISA v3.1 LEV indication in SRR1 for system call
> interrupts
> target/ppc: Better CTRL SPR implementation
>
> target/ppc/cpu.h | 1 +
> target/ppc/cpu_init.c | 41 +++++++++++++----
> target/ppc/excp_helper.c | 98 ++++++++++++++++++++++++++++++++++++----
> target/ppc/helper_regs.c | 2 +-
> target/ppc/misc_helper.c | 4 +-
> target/ppc/mmu-radix64.c | 38 +++++++++++-----
> target/ppc/power8-pmu.c | 6 ++-
> target/ppc/translate.c | 9 +++-
> 8 files changed, 164 insertions(+), 35 deletions(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/9] target/ppc: Assorted ppc target fixes
2023-05-27 18:05 ` [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Daniel Henrique Barboza
@ 2023-05-29 1:47 ` Nicholas Piggin
0 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2023-05-29 1:47 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On Sun May 28, 2023 at 4:05 AM AEST, Daniel Henrique Barboza wrote:
>
>
> On 5/15/23 06:26, Nicholas Piggin wrote:
> > Hopefully these are getting close to ready now. There is still the
> > question about doing better with adding test cases for all this, I
> > haven't exactly got a good answer yet but I do have kvm-unit-tests
> > for most at least.
>
> Patches 1 and 4 queued to ppc-next. Thanks,
Thanks Daniel, been taking a bit of time fixing up your your and
other comments for the other patches sorry. Much appreciate the help
so far.
Thanks,
Nick
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-05-29 1:48 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-15 9:26 [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs Nicholas Piggin
2023-05-15 10:14 ` Harsh Prateek Bora
2023-05-15 11:14 ` Nicholas Piggin
2023-05-15 11:43 ` Harsh Prateek Bora
2023-05-15 12:03 ` Mark Cave-Ayland
2023-05-15 12:51 ` Nicholas Piggin
2023-05-15 15:19 ` Nicholas Piggin
2023-05-16 7:02 ` Mark Cave-Ayland
2023-05-16 9:39 ` Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation Nicholas Piggin
2023-05-16 9:32 ` Daniel Henrique Barboza
2023-05-16 10:44 ` Nicholas Piggin
2023-05-16 11:07 ` Daniel Henrique Barboza
2023-05-15 9:26 ` [PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt Nicholas Piggin
2023-05-16 15:46 ` Daniel Henrique Barboza
2023-05-15 9:26 ` [PATCH v3 4/9] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 5/9] target/ppc: Change partition-scope translate interface Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 6/9] target/ppc: Add SRR1 prefix indication to interrupt handlers Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 7/9] target/ppc: Implement HEIR SPR Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 8/9] target/ppc: Add ISA v3.1 LEV indication in SRR1 for system call interrupts Nicholas Piggin
2023-05-15 9:26 ` [PATCH v3 9/9] target/ppc: Better CTRL SPR implementation Nicholas Piggin
2023-05-27 18:05 ` [PATCH v3 0/9] target/ppc: Assorted ppc target fixes Daniel Henrique Barboza
2023-05-29 1:47 ` Nicholas Piggin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).