[PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Harsh Prateek Bora]

This patchset introduces helper routines to enable (and does) cleaning
up of h_enter_nested() and spapr_exit_nested() routines in existing api
for nested virtualization on Power/SPAPR for better code readability / 
maintenance. No functional changes intended with this patchset.

Harsh Prateek Bora (5):
  ppc: spapr: cleanup cr get/store with helper routines.
  ppc: spapr: cleanup h_enter_nested() with helper routines.
  ppc: spapr: assert early rather late in h_enter_nested()
  ppc: spapr: cleanup spapr_exit_nested() with helper routines.
  MAINTAINERS: Adding myself in the list for ppc/spapr

 MAINTAINERS          |   1 +
 hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++-------------------
 target/ppc/cpu.c     |  17 +++
 target/ppc/cpu.h     |   2 +
 4 files changed, 161 insertions(+), 110 deletions(-)

-- 
2.31.1

[PATCH 1/5] ppc: spapr: cleanup cr get/store with helper routines.

[Harsh Prateek Bora]

The bits in cr reg are grouped into eight 4-bit fields represented
by env->crf[8] and the related calculations should be abstracted to
keep the calling routines simpler to read. This is a step towards
cleaning up the [h_enter|spapr_exit]_nested calls for better readability.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 hw/ppc/spapr_hcall.c | 18 ++----------------
 target/ppc/cpu.c     | 17 +++++++++++++++++
 target/ppc/cpu.h     |  2 ++
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ec4def62f8..124cee5e53 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1566,8 +1566,6 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     struct kvmppc_hv_guest_state hv_state;
     struct kvmppc_pt_regs *regs;
     hwaddr len;
-    uint64_t cr;
-    int i;
 
     if (spapr->nested_ptcr == 0) {
         return H_NOT_AVAILABLE;
@@ -1616,12 +1614,7 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     env->lr = regs->link;
     env->ctr = regs->ctr;
     cpu_write_xer(env, regs->xer);
-
-    cr = regs->ccr;
-    for (i = 7; i >= 0; i--) {
-        env->crf[i] = cr & 15;
-        cr >>= 4;
-    }
+    ppc_store_cr(env, regs->ccr);
 
     env->msr = regs->msr;
     env->nip = regs->nip;
@@ -1698,8 +1691,6 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     struct kvmppc_hv_guest_state *hvstate;
     struct kvmppc_pt_regs *regs;
     hwaddr len;
-    uint64_t cr;
-    int i;
 
     assert(spapr_cpu->in_nested);
 
@@ -1757,12 +1748,7 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     regs->link = env->lr;
     regs->ctr = env->ctr;
     regs->xer = cpu_read_xer(env);
-
-    cr = 0;
-    for (i = 0; i < 8; i++) {
-        cr |= (env->crf[i] & 15) << (4 * (7 - i));
-    }
-    regs->ccr = cr;
+    regs->ccr = ppc_get_cr(env);
 
     if (excp == POWERPC_EXCP_MCHECK ||
         excp == POWERPC_EXCP_RESET ||
diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 1a97b41c6b..3b444e58b5 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -67,6 +67,23 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
     return env->vscr | (sat << VSCR_SAT);
 }
 
+void ppc_store_cr(CPUPPCState *env, uint64_t cr)
+{
+    for (int i = 7; i >= 0; i--) {
+        env->crf[i] = cr & 15;
+        cr >>= 4;
+    }
+}
+
+uint64_t ppc_get_cr(CPUPPCState *env)
+{
+    uint64_t cr = 0;
+    for (int i = 0; i < 8; i++) {
+        cr |= (env->crf[i] & 15) << (4 * (7 - i));
+    }
+    return cr;
+}
+
 /* GDBstub can read and write MSR... */
 void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 557d736dab..b4c21459f1 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2773,6 +2773,8 @@ void dump_mmu(CPUPPCState *env);
 void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
 void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
 uint32_t ppc_get_vscr(CPUPPCState *env);
+void ppc_store_cr(CPUPPCState *env, uint64_t cr);
+uint64_t ppc_get_cr(CPUPPCState *env);
 
 /*****************************************************************************/
 /* Power management enable checks                                            */
-- 
2.31.1

[PATCH 2/5] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Harsh Prateek Bora]

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 | 99 +++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 39 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 124cee5e53..a13e5256ab 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1544,6 +1544,62 @@ 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_hvstate(CPUPPCState *env,
+                                            struct kvmppc_pt_regs *regs,
+                                            struct kvmppc_hv_guest_state *hv_state)
+{
+    assert(env);
+    assert(regs);
+    assert(hv_state);
+    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);
+    /* hv_state->amor is not used in api v1 */
+    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;
+}
+
 /*
  * 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 +1610,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 +1661,16 @@ 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_env_from_ptregs_hvstate(env, regs, &hv_state);
+    restore_lpcr_from_hvstate(cpu, &hv_state);
+    restore_hdec_from_hvstate(env, &hv_state, now);
 
     address_space_unmap(CPU(cpu)->as, regs, len, len, false);
 
-    env->cfar = hv_state.cfar;
-
     assert(env->spr[SPR_LPIDR] == 0);
     env->spr[SPR_LPIDR] = hv_state.lpid;
 
-    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
-    lpcr = (env->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state.lpcr & lpcr_mask);
-    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
-    lpcr &= ~LPCR_LPES0;
-    env->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
-
-    env->spr[SPR_PCR] = hv_state.pcr;
-    /* hv_state.amor is not used */
-    env->spr[SPR_DPDES] = hv_state.dpdes;
-    env->spr[SPR_HFSCR] = hv_state.hfscr;
-    hdec = hv_state.hdec_expiry - now;
     spapr_cpu->nested_tb_offset = hv_state.tb_offset;
-    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
-    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;
-
-    cpu_ppc_hdecr_init(env);
-    cpu_ppc_store_hdecr(env, hdec);
 
     /*
      * The hv_state.vcpu_token is not needed. It is used by the KVM
-- 
2.31.1

[PATCH 3/5] ppc: spapr: assert early rather late in h_enter_nested()

[Harsh Prateek Bora]

Currently, it asserts very late in the code flow if lpid is already initialized.
Ideally, it should assert in the beginning if that is the case. This patch
brings assert check in the beginning alongwith the related initialization.

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

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index a13e5256ab..a77b4c9076 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1621,6 +1621,9 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     struct kvmppc_pt_regs *regs;
     hwaddr len;
 
+    assert(env->spr[SPR_LPIDR] == 0);
+    env->spr[SPR_LPIDR] = hv_state.lpid;
+
     if (spapr->nested_ptcr == 0) {
         return H_NOT_AVAILABLE;
     }
@@ -1667,9 +1670,6 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
 
     address_space_unmap(CPU(cpu)->as, regs, len, len, false);
 
-    assert(env->spr[SPR_LPIDR] == 0);
-    env->spr[SPR_LPIDR] = hv_state.lpid;
-
     spapr_cpu->nested_tb_offset = hv_state.tb_offset;
 
     /*
-- 
2.31.1

[PATCH 4/5] ppc: spapr: cleanup spapr_exit_nested() with helper routines.

[Harsh Prateek Bora]

Currently, in spapr_exit_nested(), it does a lot of register state
restoring from ptregs/hvstate after mapping each of those before
restoring the L1 host state. This patch breaks down those set of ops
to respective helper routines for better code readability/maintenance.

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

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index a77b4c9076..ed3a21471d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1701,36 +1701,23 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     return env->gpr[3];
 }
 
-void spapr_exit_nested(PowerPCCPU *cpu, int excp)
+static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
+                                     CPUPPCState *env, int excp)
 {
-    CPUState *cs = CPU(cpu);
-    CPUPPCState *env = &cpu->env;
-    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
-    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
-    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
-    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
-    struct kvmppc_hv_guest_state *hvstate;
-    struct kvmppc_pt_regs *regs;
-    hwaddr len;
-
-    assert(spapr_cpu->in_nested);
-
-    cpu_ppc_hdecr_exit(env);
-
-    len = sizeof(*hvstate);
-    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
-                                MEMTXATTRS_UNSPECIFIED);
-    if (len != sizeof(*hvstate)) {
-        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
-        r3_return = H_PARAMETER;
-        goto out_restore_l1;
-    }
-
-    hvstate->cfar = env->cfar;
-    hvstate->lpcr = env->spr[SPR_LPCR];
-    hvstate->pcr = env->spr[SPR_PCR];
-    hvstate->dpdes = env->spr[SPR_DPDES];
-    hvstate->hfscr = env->spr[SPR_HFSCR];
+    hvstate->cfar    = env->cfar;
+    hvstate->lpcr    = env->spr[SPR_LPCR];
+    hvstate->pcr     = env->spr[SPR_PCR];
+    hvstate->dpdes   = env->spr[SPR_DPDES];
+    hvstate->hfscr   = env->spr[SPR_HFSCR];
+    /* HEIR should be implemented for HV mode and saved here. */
+    hvstate->srr0    = env->spr[SPR_SRR0];
+    hvstate->srr1    = env->spr[SPR_SRR1];
+    hvstate->sprg[0] = env->spr[SPR_SPRG0];
+    hvstate->sprg[1] = env->spr[SPR_SPRG1];
+    hvstate->sprg[2] = env->spr[SPR_SPRG2];
+    hvstate->sprg[3] = env->spr[SPR_SPRG3];
+    hvstate->pidr    = env->spr[SPR_BOOKS_PID];
+    hvstate->ppr     = env->spr[SPR_PPR];
 
     if (excp == POWERPC_EXCP_HDSI) {
         hvstate->hdar = env->spr[SPR_HDAR];
@@ -1739,38 +1726,36 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     } else if (excp == POWERPC_EXCP_HISI) {
         hvstate->asdr = env->spr[SPR_ASDR];
     }
+}
 
-    /* HEIR should be implemented for HV mode and saved here. */
-    hvstate->srr0 = env->spr[SPR_SRR0];
-    hvstate->srr1 = env->spr[SPR_SRR1];
-    hvstate->sprg[0] = env->spr[SPR_SPRG0];
-    hvstate->sprg[1] = env->spr[SPR_SPRG1];
-    hvstate->sprg[2] = env->spr[SPR_SPRG2];
-    hvstate->sprg[3] = env->spr[SPR_SPRG3];
-    hvstate->pidr = env->spr[SPR_BOOKS_PID];
-    hvstate->ppr = env->spr[SPR_PPR];
-
-    /* Is it okay to specify write length larger than actual data written? */
-    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
+static int map_and_restore_hvstate(PowerPCCPU *cpu, int excp, target_ulong *r3)
+{
+    CPUPPCState *env = &cpu->env;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
+    struct kvmppc_hv_guest_state *hvstate;
+    hwaddr len = sizeof(*hvstate);
 
-    len = sizeof(*regs);
-    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
+    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
                                 MEMTXATTRS_UNSPECIFIED);
-    if (!regs || len != sizeof(*regs)) {
-        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
-        r3_return = H_P2;
-        goto out_restore_l1;
+    if (len != sizeof(*hvstate)) {
+        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
+        *r3 = H_PARAMETER;
+        return -1;
     }
+    restore_hvstate_from_env(hvstate, env, excp);
+    /* Is it okay to specify write length larger than actual data written? */
+    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
+    return 0;
+}
 
+static void restore_ptregs_from_env(struct kvmppc_pt_regs *regs,
+                                    CPUPPCState *env, int excp)
+{
+    hwaddr len;
     len = sizeof(env->gpr);
     assert(len == sizeof(regs->gpr));
     memcpy(regs->gpr, env->gpr, len);
-
-    regs->link = env->lr;
-    regs->ctr = env->ctr;
-    regs->xer = cpu_read_xer(env);
-    regs->ccr = ppc_get_cr(env);
-
     if (excp == POWERPC_EXCP_MCHECK ||
         excp == POWERPC_EXCP_RESET ||
         excp == POWERPC_EXCP_SYSCALL) {
@@ -1780,11 +1765,50 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
         regs->nip = env->spr[SPR_HSRR0];
         regs->msr = env->spr[SPR_HSRR1] & env->msr_mask;
     }
+    regs->link = env->lr;
+    regs->ctr  = env->ctr;
+    regs->xer  = cpu_read_xer(env);
+    regs->ccr  = ppc_get_cr(env);
+}
 
+static int map_and_restore_ptregs(PowerPCCPU *cpu, int excp, target_ulong *r3)
+{
+    CPUPPCState *env = &cpu->env;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
+    hwaddr len;
+    struct kvmppc_pt_regs *regs = NULL;
+
+    len = sizeof(*regs);
+    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
+                             MEMTXATTRS_UNSPECIFIED);
+    if (!regs || len != sizeof(*regs)) {
+        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
+        *r3 = H_P2;
+        return -1;
+    }
+    restore_ptregs_from_env(regs, env, excp);
     /* Is it okay to specify write length larger than actual data written? */
     address_space_unmap(CPU(cpu)->as, regs, len, len, true);
+    return 0;
+}
+
+void spapr_exit_nested(PowerPCCPU *cpu, int excp)
+{
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
+
+    assert(spapr_cpu->in_nested);
+
+    cpu_ppc_hdecr_exit(env);
+
+   if (!map_and_restore_hvstate(cpu, excp, &r3_return)) {
+       map_and_restore_ptregs (cpu, excp, &r3_return);
+   }
 
-out_restore_l1:
+    /* out_restore_l1 */
     memcpy(env->gpr, spapr_cpu->nested_host_state->gpr, sizeof(env->gpr));
     env->lr = spapr_cpu->nested_host_state->lr;
     env->ctr = spapr_cpu->nested_host_state->ctr;
-- 
2.31.1

[PATCH 5/5] MAINTAINERS: Adding myself in the list for ppc/spapr

[Harsh Prateek Bora]

Would like to get notified of changes in this area and review them.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9b56ccdd92..be99e5c4e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1406,6 +1406,7 @@ M: Daniel Henrique Barboza <danielhb413@gmail.com>
 R: Cédric Le Goater <clg@kaod.org>
 R: David Gibson <david@gibson.dropbear.id.au>
 R: Greg Kurz <groug@kaod.org>
+R: Harsh Prateek Bora <harshpb@linux.ibm.com>
 L: qemu-ppc@nongnu.org
 S: Odd Fixes
 F: hw/*/spapr*
-- 
2.31.1

Re: [PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Cédric Le Goater]

On 3/31/23 08:53, Harsh Prateek Bora wrote:
> This patchset introduces helper routines to enable (and does) cleaning
> up of h_enter_nested() and spapr_exit_nested() routines in existing api
> for nested virtualization on Power/SPAPR for better code readability /
> maintenance. No functional changes intended with this patchset.

Adding Nick since he did most of this work.

C.


> 
> Harsh Prateek Bora (5):
>    ppc: spapr: cleanup cr get/store with helper routines.
>    ppc: spapr: cleanup h_enter_nested() with helper routines.
>    ppc: spapr: assert early rather late in h_enter_nested()
>    ppc: spapr: cleanup spapr_exit_nested() with helper routines.
>    MAINTAINERS: Adding myself in the list for ppc/spapr
> 
>   MAINTAINERS          |   1 +
>   hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++-------------------
>   target/ppc/cpu.c     |  17 +++
>   target/ppc/cpu.h     |   2 +
>   4 files changed, 161 insertions(+), 110 deletions(-)
> 

Re: [PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Daniel Henrique Barboza]

On 3/31/23 07:39, Cédric Le Goater wrote:
> On 3/31/23 08:53, Harsh Prateek Bora wrote:
>> This patchset introduces helper routines to enable (and does) cleaning
>> up of h_enter_nested() and spapr_exit_nested() routines in existing api
>> for nested virtualization on Power/SPAPR for better code readability /
>> maintenance. No functional changes intended with this patchset.
> 
> Adding Nick since he did most of this work.


And also Fabiano.


Daniel

> 
> C.
> 
> 
>>
>> Harsh Prateek Bora (5):
>>    ppc: spapr: cleanup cr get/store with helper routines.
>>    ppc: spapr: cleanup h_enter_nested() with helper routines.
>>    ppc: spapr: assert early rather late in h_enter_nested()
>>    ppc: spapr: cleanup spapr_exit_nested() with helper routines.
>>    MAINTAINERS: Adding myself in the list for ppc/spapr
>>
>>   MAINTAINERS          |   1 +
>>   hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++-------------------
>>   target/ppc/cpu.c     |  17 +++
>>   target/ppc/cpu.h     |   2 +
>>   4 files changed, 161 insertions(+), 110 deletions(-)
>>
> 

Re: [PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Harsh Prateek Bora]

Hi Nick, Fabiano,

Any review comments, please?

regards,
Harsh

On 3/31/23 16:36, Daniel Henrique Barboza wrote:
> 
> 
> On 3/31/23 07:39, Cédric Le Goater wrote:
>> On 3/31/23 08:53, Harsh Prateek Bora wrote:
>>> This patchset introduces helper routines to enable (and does) cleaning
>>> up of h_enter_nested() and spapr_exit_nested() routines in existing api
>>> for nested virtualization on Power/SPAPR for better code readability /
>>> maintenance. No functional changes intended with this patchset.
>>
>> Adding Nick since he did most of this work.
> 
> 
> And also Fabiano.
> 
> 
> Daniel
> 
>>
>> C.
>>
>>
>>>
>>> Harsh Prateek Bora (5):
>>>    ppc: spapr: cleanup cr get/store with helper routines.
>>>    ppc: spapr: cleanup h_enter_nested() with helper routines.
>>>    ppc: spapr: assert early rather late in h_enter_nested()
>>>    ppc: spapr: cleanup spapr_exit_nested() with helper routines.
>>>    MAINTAINERS: Adding myself in the list for ppc/spapr
>>>
>>>   MAINTAINERS          |   1 +
>>>   hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++-------------------
>>>   target/ppc/cpu.c     |  17 +++
>>>   target/ppc/cpu.h     |   2 +
>>>   4 files changed, 161 insertions(+), 110 deletions(-)
>>>
>>

Re: [PATCH 1/5] ppc: spapr: cleanup cr get/store with helper routines.

[Fabiano Rosas]

Harsh Prateek Bora <harshpb@linux.ibm.com> writes:

A bit vague on the subject line. I would expect to see some mention to
nested at least.

> The bits in cr reg are grouped into eight 4-bit fields represented
> by env->crf[8] and the related calculations should be abstracted to
> keep the calling routines simpler to read. This is a step towards
> cleaning up the [h_enter|spapr_exit]_nested calls for better readability.
>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/5] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Fabiano Rosas]

Harsh Prateek Bora <harshpb@linux.ibm.com> writes:

> 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 | 99 +++++++++++++++++++++++++++-----------------
>  1 file changed, 60 insertions(+), 39 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 124cee5e53..a13e5256ab 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1544,6 +1544,62 @@ 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;

add a blank line here

> +    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;

here as well

> +    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_hvstate(CPUPPCState *env,

Take a look at how the kernel does it. It might be better to have ptregs
and hv regs separate. Also probably better to have some terms specific
to the domain (l2 state, l1 state, etc).

> +                                            struct kvmppc_pt_regs *regs,
> +                                            struct kvmppc_hv_guest_state *hv_state)
> +{
> +    assert(env);
> +    assert(regs);
> +    assert(hv_state);
> +    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);
> +    /* hv_state->amor is not used in api v1 */

That's not really an API thing. More of an oversight.

> +    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;

I would advise against the extra spacing inside functions.

Re: [PATCH 3/5] ppc: spapr: assert early rather late in h_enter_nested()

[Fabiano Rosas]

Harsh Prateek Bora <harshpb@linux.ibm.com> writes:

> Currently, it asserts very late in the code flow if lpid is already initialized.

That's not about initializing. It is about making sure the LPIDR is
0. Which has a specific meaning according to the ISA.

> Ideally, it should assert in the beginning if that is the case. This patch
> brings assert check in the beginning alongwith the related initialization.
>

Maybe just leave it where it is. There's not much to gain from moving this.

Re: [PATCH 5/5] MAINTAINERS: Adding myself in the list for ppc/spapr

[Daniel Henrique Barboza]

On 3/31/23 03:53, Harsh Prateek Bora wrote:
> Would like to get notified of changes in this area and review them.
> 
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---

All reviewers are welcome.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>



>   MAINTAINERS | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9b56ccdd92..be99e5c4e9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1406,6 +1406,7 @@ M: Daniel Henrique Barboza <danielhb413@gmail.com>
>   R: Cédric Le Goater <clg@kaod.org>
>   R: David Gibson <david@gibson.dropbear.id.au>
>   R: Greg Kurz <groug@kaod.org>
> +R: Harsh Prateek Bora <harshpb@linux.ibm.com>
>   L: qemu-ppc@nongnu.org
>   S: Odd Fixes
>   F: hw/*/spapr*

Re: [PATCH 4/5] ppc: spapr: cleanup spapr_exit_nested() with helper routines.

[Fabiano Rosas]

Harsh Prateek Bora <harshpb@linux.ibm.com> writes:

> Currently, in spapr_exit_nested(), it does a lot of register state
> restoring from ptregs/hvstate after mapping each of those before
> restoring the L1 host state. This patch breaks down those set of ops
> to respective helper routines for better code readability/maintenance.
>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  hw/ppc/spapr_hcall.c | 132 +++++++++++++++++++++++++------------------
>  1 file changed, 78 insertions(+), 54 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index a77b4c9076..ed3a21471d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1701,36 +1701,23 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>      return env->gpr[3];
>  }
>  
> -void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> +static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
> +                                     CPUPPCState *env, int excp)
>  {
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> -    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
> -    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
> -    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
> -    struct kvmppc_hv_guest_state *hvstate;
> -    struct kvmppc_pt_regs *regs;
> -    hwaddr len;
> -
> -    assert(spapr_cpu->in_nested);
> -
> -    cpu_ppc_hdecr_exit(env);
> -
> -    len = sizeof(*hvstate);
> -    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
> -                                MEMTXATTRS_UNSPECIFIED);
> -    if (len != sizeof(*hvstate)) {
> -        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
> -        r3_return = H_PARAMETER;
> -        goto out_restore_l1;
> -    }
> -
> -    hvstate->cfar = env->cfar;
> -    hvstate->lpcr = env->spr[SPR_LPCR];
> -    hvstate->pcr = env->spr[SPR_PCR];
> -    hvstate->dpdes = env->spr[SPR_DPDES];
> -    hvstate->hfscr = env->spr[SPR_HFSCR];
> +    hvstate->cfar    = env->cfar;
> +    hvstate->lpcr    = env->spr[SPR_LPCR];
> +    hvstate->pcr     = env->spr[SPR_PCR];
> +    hvstate->dpdes   = env->spr[SPR_DPDES];
> +    hvstate->hfscr   = env->spr[SPR_HFSCR];
> +    /* HEIR should be implemented for HV mode and saved here. */
> +    hvstate->srr0    = env->spr[SPR_SRR0];
> +    hvstate->srr1    = env->spr[SPR_SRR1];
> +    hvstate->sprg[0] = env->spr[SPR_SPRG0];
> +    hvstate->sprg[1] = env->spr[SPR_SPRG1];
> +    hvstate->sprg[2] = env->spr[SPR_SPRG2];
> +    hvstate->sprg[3] = env->spr[SPR_SPRG3];
> +    hvstate->pidr    = env->spr[SPR_BOOKS_PID];
> +    hvstate->ppr     = env->spr[SPR_PPR];

Just leave these lines as they were. Let's avoid spending time
discussing code style. Also, the patch became harder to review by having
these unrelated changes interspersed.

Re: [PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Fabiano Rosas]

Harsh Prateek Bora <harshpb@linux.ibm.com> writes:

> This patchset introduces helper routines to enable (and does) cleaning
> up of h_enter_nested() and spapr_exit_nested() routines in existing api
> for nested virtualization on Power/SPAPR for better code readability / 
> maintenance. No functional changes intended with this patchset.

Do we have tests for this nested virtual hypervisor code? It would be
good to have at least a smoke test. I remember someone started adding
support to qemu-ppc-boot, probably Murilo. I suggest you take a look
around and maybe see what it takes to add it.

Just a L0 TCG with a small-ish qemu inside of it and a script to start
an L2.

>
> Harsh Prateek Bora (5):
>   ppc: spapr: cleanup cr get/store with helper routines.
>   ppc: spapr: cleanup h_enter_nested() with helper routines.
>   ppc: spapr: assert early rather late in h_enter_nested()
>   ppc: spapr: cleanup spapr_exit_nested() with helper routines.
>   MAINTAINERS: Adding myself in the list for ppc/spapr
>
>  MAINTAINERS          |   1 +
>  hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++-------------------
>  target/ppc/cpu.c     |  17 +++
>  target/ppc/cpu.h     |   2 +
>  4 files changed, 161 insertions(+), 110 deletions(-)

Re: [PATCH 1/5] ppc: spapr: cleanup cr get/store with helper routines.

[Harsh Prateek Bora]

On 4/14/23 17:10, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
> 
> A bit vague on the subject line. I would expect to see some mention to
> nested at least.

Sure, will update subject line to include mention of 
[h_enter|spapr_exit]_nested routines.

> 
>> The bits in cr reg are grouped into eight 4-bit fields represented
>> by env->crf[8] and the related calculations should be abstracted to
>> keep the calling routines simpler to read. This is a step towards
>> cleaning up the [h_enter|spapr_exit]_nested calls for better readability.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>

Thanks!

Re: [PATCH 2/5] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Harsh Prateek Bora]

On 4/14/23 17:23, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
> 
>> 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 | 99 +++++++++++++++++++++++++++-----------------
>>   1 file changed, 60 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 124cee5e53..a13e5256ab 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1544,6 +1544,62 @@ 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;
> 
> add a blank line here

ok
> 
>> +    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;
> 
> here as well

ok
> 
>> +    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_hvstate(CPUPPCState *env,
> 
> Take a look at how the kernel does it. It might be better to have ptregs
> and hv regs separate. Also probably better to have some terms specific
> to the domain (l2 state, l1 state, etc).
> 

ok, will separate them out and include l2/l1 wherever applicable.

>> +                                            struct kvmppc_pt_regs *regs,
>> +                                            struct kvmppc_hv_guest_state *hv_state)
>> +{
>> +    assert(env);
>> +    assert(regs);
>> +    assert(hv_state);
>> +    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);
>> +    /* hv_state->amor is not used in api v1 */
> 
> That's not really an API thing. More of an oversight.
> 
yeh, that comment was retained from existing source, will remove.

>> +    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;
> 
> I would advise against the extra spacing inside functions.
> 
sure, will keep the spacing as-is inside functions.

Thanks
Harsh

Re: [PATCH 3/5] ppc: spapr: assert early rather late in h_enter_nested()

[Harsh Prateek Bora]

On 4/14/23 17:25, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
> 
>> Currently, it asserts very late in the code flow if lpid is already initialized.
> 
> That's not about initializing. It is about making sure the LPIDR is
> 0. Which has a specific meaning according to the ISA.
> 
Yes, I could rephrase the commit log, but I am okay with your below 
suggestion for now.

>> Ideally, it should assert in the beginning if that is the case. This patch
>> brings assert check in the beginning alongwith the related initialization.
>>
> 
> Maybe just leave it where it is. There's not much to gain from moving this.
> 
Will drop this change for now as value-add is relatively lesser.

Re: [PATCH 4/5] ppc: spapr: cleanup spapr_exit_nested() with helper routines.

[Harsh Prateek Bora]

On 4/14/23 17:28, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
> 
>> Currently, in spapr_exit_nested(), it does a lot of register state
>> restoring from ptregs/hvstate after mapping each of those before
>> restoring the L1 host state. This patch breaks down those set of ops
>> to respective helper routines for better code readability/maintenance.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   hw/ppc/spapr_hcall.c | 132 +++++++++++++++++++++++++------------------
>>   1 file changed, 78 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index a77b4c9076..ed3a21471d 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1701,36 +1701,23 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>       return env->gpr[3];
>>   }
>>   
>> -void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>> +static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
>> +                                     CPUPPCState *env, int excp)
>>   {
>> -    CPUState *cs = CPU(cpu);
>> -    CPUPPCState *env = &cpu->env;
>> -    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> -    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
>> -    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
>> -    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
>> -    struct kvmppc_hv_guest_state *hvstate;
>> -    struct kvmppc_pt_regs *regs;
>> -    hwaddr len;
>> -
>> -    assert(spapr_cpu->in_nested);
>> -
>> -    cpu_ppc_hdecr_exit(env);
>> -
>> -    len = sizeof(*hvstate);
>> -    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
>> -                                MEMTXATTRS_UNSPECIFIED);
>> -    if (len != sizeof(*hvstate)) {
>> -        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
>> -        r3_return = H_PARAMETER;
>> -        goto out_restore_l1;
>> -    }
>> -
>> -    hvstate->cfar = env->cfar;
>> -    hvstate->lpcr = env->spr[SPR_LPCR];
>> -    hvstate->pcr = env->spr[SPR_PCR];
>> -    hvstate->dpdes = env->spr[SPR_DPDES];
>> -    hvstate->hfscr = env->spr[SPR_HFSCR];
>> +    hvstate->cfar    = env->cfar;
>> +    hvstate->lpcr    = env->spr[SPR_LPCR];
>> +    hvstate->pcr     = env->spr[SPR_PCR];
>> +    hvstate->dpdes   = env->spr[SPR_DPDES];
>> +    hvstate->hfscr   = env->spr[SPR_HFSCR];
>> +    /* HEIR should be implemented for HV mode and saved here. */
>> +    hvstate->srr0    = env->spr[SPR_SRR0];
>> +    hvstate->srr1    = env->spr[SPR_SRR1];
>> +    hvstate->sprg[0] = env->spr[SPR_SPRG0];
>> +    hvstate->sprg[1] = env->spr[SPR_SPRG1];
>> +    hvstate->sprg[2] = env->spr[SPR_SPRG2];
>> +    hvstate->sprg[3] = env->spr[SPR_SPRG3];
>> +    hvstate->pidr    = env->spr[SPR_BOOKS_PID];
>> +    hvstate->ppr     = env->spr[SPR_PPR];
> 
> Just leave these lines as they were. Let's avoid spending time
> discussing code style. Also, the patch became harder to review by having
> these unrelated changes interspersed.
> 
Ok, will keep the spacing as-is inside helper routine 
restore_hvstate_from_env() as well.

Re: [PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Harsh Prateek Bora]

On 4/14/23 17:34, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
> 
>> This patchset introduces helper routines to enable (and does) cleaning
>> up of h_enter_nested() and spapr_exit_nested() routines in existing api
>> for nested virtualization on Power/SPAPR for better code readability /
>> maintenance. No functional changes intended with this patchset.
> 
> Do we have tests for this nested virtual hypervisor code? It would be
> good to have at least a smoke test. I remember someone started adding
> support to qemu-ppc-boot, probably Murilo. I suggest you take a look
> around and maybe see what it takes to add it.
> 
> Just a L0 TCG with a small-ish qemu inside of it and a script to start
> an L2.
> 

This is something I already have in my to-do list, taking a note of your 
suggestions.

Thanks
Harsh

>>
>> Harsh Prateek Bora (5):
>>    ppc: spapr: cleanup cr get/store with helper routines.
>>    ppc: spapr: cleanup h_enter_nested() with helper routines.
>>    ppc: spapr: assert early rather late in h_enter_nested()
>>    ppc: spapr: cleanup spapr_exit_nested() with helper routines.
>>    MAINTAINERS: Adding myself in the list for ppc/spapr
>>
>>   MAINTAINERS          |   1 +
>>   hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++-------------------
>>   target/ppc/cpu.c     |  17 +++
>>   target/ppc/cpu.h     |   2 +
>>   4 files changed, 161 insertions(+), 110 deletions(-)

Re: [PATCH 5/5] MAINTAINERS: Adding myself in the list for ppc/spapr

[Harsh Prateek Bora]

On 4/14/23 17:27, Daniel Henrique Barboza wrote:
> 
> 
> On 3/31/23 03:53, Harsh Prateek Bora wrote:
>> Would like to get notified of changes in this area and review them.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
> 
> All reviewers are welcome.
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 

Thanks for the warm welcome, Daniel.

regards,
Harsh

> 
> 
>>   MAINTAINERS | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9b56ccdd92..be99e5c4e9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1406,6 +1406,7 @@ M: Daniel Henrique Barboza <danielhb413@gmail.com>
>>   R: Cédric Le Goater <clg@kaod.org>
>>   R: David Gibson <david@gibson.dropbear.id.au>
>>   R: Greg Kurz <groug@kaod.org>
>> +R: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>   L: qemu-ppc@nongnu.org
>>   S: Odd Fixes
>>   F: hw/*/spapr*

Re: [PATCH 0/5] Cleanup [h_enter|spapr_exit]_nested routines

[Cédric Le Goater]

On 4/17/23 09:21, Harsh Prateek Bora wrote:
> 
> 
> On 4/14/23 17:34, Fabiano Rosas wrote:
>> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
>>
>>> This patchset introduces helper routines to enable (and does) cleaning
>>> up of h_enter_nested() and spapr_exit_nested() routines in existing api
>>> for nested virtualization on Power/SPAPR for better code readability /
>>> maintenance. No functional changes intended with this patchset.
>>
>> Do we have tests for this nested virtual hypervisor code? It would be
>> good to have at least a smoke test. I remember someone started adding
>> support to qemu-ppc-boot, probably Murilo. I suggest you take a look
>> around and maybe see what it takes to add it.
>>
>> Just a L0 TCG with a small-ish qemu inside of it and a script to start
>> an L2.
>>
> 
> This is something I already have in my to-do list, taking a note of your suggestions.

When developing KVM support under the emulated PowerNV QEMU machines,
I was using this buildroot target :

   https://github.com/legoater/buildroot/commit/307ecbf7409e6fb060d7bbcc41201fdcacf0b0ca

I then used the same image to test the L2 support.

C.