Re: [RFC PATCH 4/5] target/ppc: powerpc_excp: Standardize arguments to interrupt code
On Tue, Jun 01, 2021 at 06:46:48PM -0300, Fabiano Rosas wrote:
> The next patches will split the big switch statement in powerpc_excp
> into individual functions so it would be cleaner if all variables are
> already grouped in a structure and their names consistent.
>
> This patch makes it so that the old values for MSR and NIP (from env)
> are saved at the beginning as regs.msr and regs.nip and all
> modifications are done over this regs version. At the end of the
> function regs.msr and regs.nip are saved in the SRRs and regs.new_msr
> and regs.new_nip are written to env.
>
> There are two points of interest here:
>
> - The system call code has a particularity where it needs to use
> env->nip because it might return early and the modification needs to
> be seen by the virtual hypervisor hypercall code. I have added a
> comment making this clear.
>
> - The MSR filter at the beginning is being applied to the old MSR value
> only, i.e. the one that goes into SRR1. The new_msr is taken from
> env->msr without filtering the reserved bits. This might be a bug in
> the existing code. I'm also adding a comment to point that out.
>
> Signed-off-by: Fabiano Rosas
> ---
> target/ppc/excp_helper.c | 231 +--
> 1 file changed, 125 insertions(+), 106 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index fd147e2a37..12bf829c8f 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -186,7 +186,7 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState
> *env, int excp,
> static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int
> excp,
>target_ulong msr,
>target_ulong *new_msr,
> - target_ulong *vector)
> + target_ulong *new_nip)
> {
> #if defined(TARGET_PPC64)
> CPUPPCState *env = >env;
> @@ -263,9 +263,9 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu,
> int excp_model, int excp,
>
> if (excp != POWERPC_EXCP_SYSCALL_VECTORED) {
> if (ail == 2) {
> -*vector |= 0x00018000ull;
> +*new_nip |= 0x00018000ull;
> } else if (ail == 3) {
> -*vector |= 0xc0004000ull;
> +*new_nip |= 0xc0004000ull;
> }
> } else {
> /*
> @@ -273,15 +273,15 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu,
> int excp_model, int excp,
> * only the MSR. AIL=3 replaces the 0x17000 base with 0xc...3000.
> */
> if (ail == 3) {
> -*vector &= ~0x00017000ull; /* Un-apply the base offset */
> -*vector |= 0xc0003000ull; /* Apply scv's AIL=3 offset */
> +*new_nip &= ~0x00017000ull; /* Un-apply the base offset
> */
> +*new_nip |= 0xc0003000ull; /* Apply scv's AIL=3 offset */
> }
> }
> #endif
> }
>
> -static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
> - target_ulong vector, target_ulong
> msr)
> +static inline void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong
> new_nip,
> + target_ulong new_msr)
> {
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = >env;
> @@ -294,9 +294,9 @@ static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
> * will prevent setting of the HV bit which some exceptions might need
> * to do.
> */
> -env->msr = msr & env->msr_mask;
> +env->msr = new_msr & env->msr_mask;
> hreg_compute_hflags(env);
> -env->nip = vector;
> +env->nip = new_nip;
> /* Reset exception state */
> cs->exception_index = POWERPC_EXCP_NONE;
> env->error_code = 0;
> @@ -311,6 +311,17 @@ static inline void powerpc_set_excp_state(PowerPCCPU
> *cpu,
> check_tlb_flush(env, false);
> }
>
> +struct ppc_intr_args {
Please use qemu coding style convetions for type names (CamelCase and
a typedef).
> +target_ulong nip;
> +target_ulong msr;
> +target_ulong new_nip;
> +target_ulong new_msr;
> +int sprn_srr0;
> +int sprn_srr1;
> +int sprn_asrr0;
> +int sprn_asrr1;
> +};
> +
> /*
> * Note that this function should be greatly optimized when called
> * with a constant excp, from ppc_hw_interrupt
> @@ -319,37 +330,40 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> excp_model, int excp)
> {
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = >env;
> -target_ulong msr, new_msr, vector;
> -int srr0, srr1, asrr0, asrr1, lev = -1;
> +struct ppc_intr_args regs;
> +int lev = -1;
>
> qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>" => %08x (%02x)\n", env->nip, excp, env->error_code);
>
> /* new srr1 value excluding must-be-zero bits */
> if
[RFC PATCH 4/5] target/ppc: powerpc_excp: Standardize arguments to interrupt code
The next patches will split the big switch statement in powerpc_excp
into individual functions so it would be cleaner if all variables are
already grouped in a structure and their names consistent.
This patch makes it so that the old values for MSR and NIP (from env)
are saved at the beginning as regs.msr and regs.nip and all
modifications are done over this regs version. At the end of the
function regs.msr and regs.nip are saved in the SRRs and regs.new_msr
and regs.new_nip are written to env.
There are two points of interest here:
- The system call code has a particularity where it needs to use
env->nip because it might return early and the modification needs to
be seen by the virtual hypervisor hypercall code. I have added a
comment making this clear.
- The MSR filter at the beginning is being applied to the old MSR value
only, i.e. the one that goes into SRR1. The new_msr is taken from
env->msr without filtering the reserved bits. This might be a bug in
the existing code. I'm also adding a comment to point that out.
Signed-off-by: Fabiano Rosas
---
target/ppc/excp_helper.c | 231 +--
1 file changed, 125 insertions(+), 106 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index fd147e2a37..12bf829c8f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -186,7 +186,7 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState
*env, int excp,
static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int
excp,
target_ulong msr,
target_ulong *new_msr,
- target_ulong *vector)
+ target_ulong *new_nip)
{
#if defined(TARGET_PPC64)
CPUPPCState *env = >env;
@@ -263,9 +263,9 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int
excp_model, int excp,
if (excp != POWERPC_EXCP_SYSCALL_VECTORED) {
if (ail == 2) {
-*vector |= 0x00018000ull;
+*new_nip |= 0x00018000ull;
} else if (ail == 3) {
-*vector |= 0xc0004000ull;
+*new_nip |= 0xc0004000ull;
}
} else {
/*
@@ -273,15 +273,15 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu,
int excp_model, int excp,
* only the MSR. AIL=3 replaces the 0x17000 base with 0xc...3000.
*/
if (ail == 3) {
-*vector &= ~0x00017000ull; /* Un-apply the base offset */
-*vector |= 0xc0003000ull; /* Apply scv's AIL=3 offset */
+*new_nip &= ~0x00017000ull; /* Un-apply the base offset */
+*new_nip |= 0xc0003000ull; /* Apply scv's AIL=3 offset */
}
}
#endif
}
-static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
- target_ulong vector, target_ulong
msr)
+static inline void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong
new_nip,
+ target_ulong new_msr)
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = >env;
@@ -294,9 +294,9 @@ static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
* will prevent setting of the HV bit which some exceptions might need
* to do.
*/
-env->msr = msr & env->msr_mask;
+env->msr = new_msr & env->msr_mask;
hreg_compute_hflags(env);
-env->nip = vector;
+env->nip = new_nip;
/* Reset exception state */
cs->exception_index = POWERPC_EXCP_NONE;
env->error_code = 0;
@@ -311,6 +311,17 @@ static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
check_tlb_flush(env, false);
}
+struct ppc_intr_args {
+target_ulong nip;
+target_ulong msr;
+target_ulong new_nip;
+target_ulong new_msr;
+int sprn_srr0;
+int sprn_srr1;
+int sprn_asrr0;
+int sprn_asrr1;
+};
+
/*
* Note that this function should be greatly optimized when called
* with a constant excp, from ppc_hw_interrupt
@@ -319,37 +330,40 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
excp_model, int excp)
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = >env;
-target_ulong msr, new_msr, vector;
-int srr0, srr1, asrr0, asrr1, lev = -1;
+struct ppc_intr_args regs;
+int lev = -1;
qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
" => %08x (%02x)\n", env->nip, excp, env->error_code);
/* new srr1 value excluding must-be-zero bits */
if (excp_model == POWERPC_EXCP_BOOKE) {
-msr = env->msr;
+regs.msr = env->msr;
} else {
-msr = env->msr & ~0x783fULL;
+regs.msr = env->msr & ~0x783fULL;
}
+regs.nip = env->nip;
/*
* new interrupt handler msr preserves existing HV and ME unless
* explicitly overriden
+ *
+ * XXX: should this use the filtered MSR
