qemu-ppc
[Top][All Lists]
Advanced

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

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper rout


From: Harsh Prateek Bora
Subject: Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.
Date: Tue, 2 May 2023 11:43:41 +0530
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0



On 5/2/23 10:19, Nicholas Piggin wrote:
On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
h_enter_nested() currently does a lot of register specific operations
which should be abstracted logically to simplify the code for better
readability. This patch breaks down relevant blocks into respective
helper routines to make use of them for better readability/maintenance.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
  hw/ppc/spapr_hcall.c | 117 ++++++++++++++++++++++++++++---------------
  1 file changed, 78 insertions(+), 39 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 124cee5e53..f24d4b368e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1544,6 +1544,81 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
      return H_FUNCTION;
  }
+static void restore_hdec_from_hvstate(CPUPPCState *dst,
+                                      struct kvmppc_hv_guest_state *hv_state,
+                                      target_ulong now)
+{
+    target_ulong hdec;
+
+    assert(hv_state);
+    hdec = hv_state->hdec_expiry - now;
+    cpu_ppc_hdecr_init(dst);
+    cpu_ppc_store_hdecr(dst, hdec);
+}
+
+static void restore_lpcr_from_hvstate(PowerPCCPU *cpu,
+                                      struct kvmppc_hv_guest_state *hv_state)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *dst = &cpu->env;
+    target_ulong lpcr, lpcr_mask;
+
+    assert(hv_state);
+    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
+    lpcr = (dst->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state->lpcr & lpcr_mask);
+    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
+    lpcr &= ~LPCR_LPES0;
+    dst->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
+}
+
+static void restore_env_from_ptregs(CPUPPCState *env,
+                                    struct kvmppc_pt_regs *regs)
+{
+    assert(env);
+    assert(regs);
+    assert(sizeof(env->gpr) == sizeof(regs->gpr));
+    memcpy(env->gpr, regs->gpr, sizeof(env->gpr));
+    env->nip = regs->nip;
+    env->msr = regs->msr;
+    env->lr = regs->link;
+    env->ctr = regs->ctr;
+    cpu_write_xer(env, regs->xer);
+    ppc_store_cr(env, regs->ccr);
+}
+
+static void restore_env_from_hvstate(CPUPPCState *env,
+                                     struct kvmppc_hv_guest_state *hv_state)
+{
+    assert(env);
+    assert(hv_state);
+    env->spr[SPR_HFSCR] = hv_state->hfscr;
+    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
+    env->cfar = hv_state->cfar;
+    env->spr[SPR_PCR] = hv_state->pcr;
+    env->spr[SPR_DPDES] = hv_state->dpdes;
+    env->spr[SPR_SRR0] = hv_state->srr0;
+    env->spr[SPR_SRR1] = hv_state->srr1;
+    env->spr[SPR_SPRG0] = hv_state->sprg[0];
+    env->spr[SPR_SPRG1] = hv_state->sprg[1];
+    env->spr[SPR_SPRG2] = hv_state->sprg[2];
+    env->spr[SPR_SPRG3] = hv_state->sprg[3];
+    env->spr[SPR_BOOKS_PID] = hv_state->pidr;
+    env->spr[SPR_PPR] = hv_state->ppr;
+}
+
+static inline void restore_l2_env(PowerPCCPU *cpu,
+                                 struct kvmppc_hv_guest_state *hv_state,
+                                 struct kvmppc_pt_regs *regs,
+                                 target_ulong now)
+{
+    CPUPPCState *env = &cpu->env;
+
+    restore_env_from_ptregs(env, regs);
+    restore_env_from_hvstate(env, hv_state);
+    restore_lpcr_from_hvstate(cpu, hv_state);
+    restore_hdec_from_hvstate(env, hv_state, now);
+}
+
  /*
   * When this handler returns, the environment is switched to the L2 guest
   * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1554,14 +1629,12 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
                                     target_ulong opcode,
                                     target_ulong *args)
  {
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
      CPUState *cs = CPU(cpu);
      CPUPPCState *env = &cpu->env;
      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
      target_ulong hv_ptr = args[0];
      target_ulong regs_ptr = args[1];
-    target_ulong hdec, now = cpu_ppc_load_tbl(env);
-    target_ulong lpcr, lpcr_mask;
+    target_ulong now = cpu_ppc_load_tbl(env);
      struct kvmppc_hv_guest_state *hvstate;
      struct kvmppc_hv_guest_state hv_state;
      struct kvmppc_pt_regs *regs;
@@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
          return H_P2;
      }
- len = sizeof(env->gpr);
-    assert(len == sizeof(regs->gpr));
-    memcpy(env->gpr, regs->gpr, len);
-
-    env->lr = regs->link;
-    env->ctr = regs->ctr;
-    cpu_write_xer(env, regs->xer);
-    ppc_store_cr(env, regs->ccr);
-
-    env->msr = regs->msr;
-    env->nip = regs->nip;
+    /* restore L2 env from hv_state and ptregs */
+    restore_l2_env(cpu, &hv_state, regs, now);
address_space_unmap(CPU(cpu)->as, regs, len, len, false);

I don't agree this improves readability. It also does more with the
guest address space mapped, which may not be a big deal is strictly
not an improvement.

The comment needn't just repeat what the function says, and it does
not actually restore the l2 environment. It sets some registers to
L2 values, but it also leaves other state.

I would like to see this in a larger series if it's going somewhere,
but at the moment I'd rather leave it as is.

While I agree the routine could be named restore_l2_hvstate_ptregs() as more appropriate, I think it still makes sense to have the body of enter/exit routines with as minimum LOC as possible, with the help of minimum helper routines possible. Giving semantics to the set of operations related to ptregs/hvstate register load/store is the first step towards it.

As you have guessed, this is certainly a precursor to another API version that we have been working on (still a WIP), and helps isolating the code flows for backward compatibiility. Having such changes early upstream helps stablising changes which are not a really a API/design change.

regards,
Harsh

Thanks,
Nick



reply via email to

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