[PATCH 0/4] ppc: various fixes for 9.2

[PATCH 0/4] ppc: various fixes for 9.2

[Nicholas Piggin]

Some small last-minute fixes that mostly fix recently added
or exposed issues. I'd like to send a PR with them for 9.2 in
the next couple of days unless there are objections.

Thanks,
Nick

Glenn Miles (1):
  target/ppc: Fix THREAD_SIBLING_FOREACH for mult-socket

Nicholas Piggin (3):
  target/ppc: Fix non-maskable interrupt while halted
  ppc/pnv: Fix direct controls quiesce
  ppc/pnv: Add xscom- prefix to pervasive-control region name

 target/ppc/cpu.h            |  8 ++++++--
 hw/ppc/pnv_core.c           | 11 +++++++++--
 hw/ppc/pnv_nest_pervasive.c |  2 +-
 hw/ppc/spapr_cpu_core.c     |  1 +
 target/ppc/excp_helper.c    |  7 +++++++
 5 files changed, 24 insertions(+), 5 deletions(-)

-- 
2.45.2

[PATCH 1/4] target/ppc: Fix non-maskable interrupt while halted

[Nicholas Piggin]

The ppc (pnv and spapr) NMI injection code does not go through the
asynchronous interrupt path and set a bit in env->pending_interrupts
and raise an interrupt request that the cpu_exec() loop can see.
Instead it injects the exception directly into registers.

This can lead to cpu_exec() missing that the thread has work to do,
if a NMI is injected while it was idle.

Fix this by clearing halted when injecting the interrupt. Probably
NMI injection should be reworked to use the interrupt request interface,
but this seems to work as a minimal fix.

Fixes: 3431648272d3 ("spapr: Add support for new NMI interface")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 70daa5076a..9f811af0a4 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2495,10 +2495,16 @@ static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
     }
 }
 
+/*
+ * system reset is not delivered via normal irq method, so have to set
+ * halted = 0 to resume CPU running if it was halted. Possibly we should
+ * move it over to using PPC_INTERRUPT_RESET rather than async_run_on_cpu.
+ */
 void ppc_cpu_do_system_reset(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
 
+    cs->halted = 0;
     powerpc_excp(cpu, POWERPC_EXCP_RESET);
 }
 
@@ -2520,6 +2526,7 @@ void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
 
     /* Anything for nested required here? MSR[HV] bit? */
 
+    cs->halted = 0;
     powerpc_set_excp_state(cpu, vector, msr);
 }
 
-- 
2.45.2

Re: [PATCH 1/4] target/ppc: Fix non-maskable interrupt while halted

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

On Mon, 2024-11-25 at 23:20 +1000, Nicholas Piggin wrote:
> The ppc (pnv and spapr) NMI injection code does not go through the
> asynchronous interrupt path and set a bit in env->pending_interrupts
> and raise an interrupt request that the cpu_exec() loop can see.
> Instead it injects the exception directly into registers.
> 
> This can lead to cpu_exec() missing that the thread has work to do,
> if a NMI is injected while it was idle.
> 
> Fix this by clearing halted when injecting the interrupt. Probably
> NMI injection should be reworked to use the interrupt request
> interface,
> but this seems to work as a minimal fix.
> 
> Fixes: 3431648272d3 ("spapr: Add support for new NMI interface")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/excp_helper.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 70daa5076a..9f811af0a4 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -2495,10 +2495,16 @@ static void ppc_deliver_interrupt(CPUPPCState
> *env, int interrupt)
>      }
>  }
>  
> +/*
> + * system reset is not delivered via normal irq method, so have to
> set
> + * halted = 0 to resume CPU running if it was halted. Possibly we
> should
> + * move it over to using PPC_INTERRUPT_RESET rather than
> async_run_on_cpu.
> + */
>  void ppc_cpu_do_system_reset(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> +    cs->halted = 0;
>      powerpc_excp(cpu, POWERPC_EXCP_RESET);
>  }
>  
> @@ -2520,6 +2526,7 @@ void ppc_cpu_do_fwnmi_machine_check(CPUState
> *cs, target_ulong vector)
>  
>      /* Anything for nested required here? MSR[HV] bit? */
>  
> +    cs->halted = 0;
>      powerpc_set_excp_state(cpu, vector, msr);
>  }
>  

[PATCH 2/4] ppc/pnv: Fix direct controls quiesce

[Nicholas Piggin]

powernv CPUs have a set of control registers that can stop, start, and
do other things to control a thread's execution.

Using this interface to stop a thread puts it into a particular state
that can be queried, and is distinguishable from other things that
might stop the CPU (e.g., going idle, or being debugged via gdb, or
stopped by the monitor).

Add a new flag that can speficially distinguish this state where it
is stopped with control registers. This solves some hangs when
rebooting powernv machines when skiboot is modified to allow QEMU
to use the CPU control facility (that uses controls to bring all
secondaries to a known state).

Fixes: c8891955086 ("ppc/pnv: Implement POWER10 PC xscom registers for direct controls")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

There might still be a bigger issue with how we handle CPU stop
requests. Multiple different sources may want to stop a CPU, there
may be situations where one of them resumes a CPU before all agree?
A stop_request mask or refcount might be a nice way to consolidate
all these.
---
 target/ppc/cpu.h  | 1 +
 hw/ppc/pnv_core.c | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 945af07a64..0b4f1013b8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1355,6 +1355,7 @@ struct CPUArchState {
      * special way (such as routing some resume causes to 0x100, i.e. sreset).
      */
     bool resume_as_sreset;
+    bool quiesced;
 #endif
 
     /* These resources are used only in TCG */
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index a30693990b..cbfac49862 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -217,8 +217,8 @@ static uint64_t pnv_core_power10_xscom_read(void *opaque, hwaddr addr,
     case PNV10_XSCOM_EC_CORE_RAS_STATUS:
         for (i = 0; i < nr_threads; i++) {
             PowerPCCPU *cpu = pc->threads[i];
-            CPUState *cs = CPU(cpu);
-            if (cs->stopped) {
+            CPUPPCState *env = &cpu->env;
+            if (env->quiesced) {
                 val |= PPC_BIT(0 + 8 * i) | PPC_BIT(1 + 8 * i);
             }
         }
@@ -244,20 +244,25 @@ static void pnv_core_power10_xscom_write(void *opaque, hwaddr addr,
         for (i = 0; i < nr_threads; i++) {
             PowerPCCPU *cpu = pc->threads[i];
             CPUState *cs = CPU(cpu);
+            CPUPPCState *env = &cpu->env;
 
             if (val & PPC_BIT(7 + 8 * i)) { /* stop */
                 val &= ~PPC_BIT(7 + 8 * i);
                 cpu_pause(cs);
+                env->quiesced = true;
             }
             if (val & PPC_BIT(6 + 8 * i)) { /* start */
                 val &= ~PPC_BIT(6 + 8 * i);
+                env->quiesced = false;
                 cpu_resume(cs);
             }
             if (val & PPC_BIT(4 + 8 * i)) { /* sreset */
                 val &= ~PPC_BIT(4 + 8 * i);
+                env->quiesced = false;
                 pnv_cpu_do_nmi_resume(cs);
             }
             if (val & PPC_BIT(3 + 8 * i)) { /* clear maint */
+                env->quiesced = false;
                 /*
                  * Hardware has very particular cases for where clear maint
                  * must be used and where start must be used to resume a
-- 
2.45.2

Re: [PATCH 2/4] ppc/pnv: Fix direct controls quiesce

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

On Mon, 2024-11-25 at 23:20 +1000, Nicholas Piggin wrote:
> powernv CPUs have a set of control registers that can stop, start,
> and
> do other things to control a thread's execution.
> 
> Using this interface to stop a thread puts it into a particular state
> that can be queried, and is distinguishable from other things that
> might stop the CPU (e.g., going idle, or being debugged via gdb, or
> stopped by the monitor).
> 
> Add a new flag that can speficially distinguish this state where it
> is stopped with control registers. This solves some hangs when
> rebooting powernv machines when skiboot is modified to allow QEMU
> to use the CPU control facility (that uses controls to bring all
> secondaries to a known state).
> 
> Fixes: c8891955086 ("ppc/pnv: Implement POWER10 PC xscom registers
> for direct controls")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> There might still be a bigger issue with how we handle CPU stop
> requests. Multiple different sources may want to stop a CPU, there
> may be situations where one of them resumes a CPU before all agree?
> A stop_request mask or refcount might be a nice way to consolidate
> all these.
> ---
>  target/ppc/cpu.h  | 1 +
>  hw/ppc/pnv_core.c | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 945af07a64..0b4f1013b8 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1355,6 +1355,7 @@ struct CPUArchState {
>       * special way (such as routing some resume causes to 0x100,
> i.e. sreset).
>       */
>      bool resume_as_sreset;
> +    bool quiesced;
>  #endif
>  
>      /* These resources are used only in TCG */
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index a30693990b..cbfac49862 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -217,8 +217,8 @@ static uint64_t pnv_core_power10_xscom_read(void
> *opaque, hwaddr addr,
>      case PNV10_XSCOM_EC_CORE_RAS_STATUS:
>          for (i = 0; i < nr_threads; i++) {
>              PowerPCCPU *cpu = pc->threads[i];
> -            CPUState *cs = CPU(cpu);
> -            if (cs->stopped) {
> +            CPUPPCState *env = &cpu->env;
> +            if (env->quiesced) {
>                  val |= PPC_BIT(0 + 8 * i) | PPC_BIT(1 + 8 * i);
>              }
>          }
> @@ -244,20 +244,25 @@ static void pnv_core_power10_xscom_write(void
> *opaque, hwaddr addr,
>          for (i = 0; i < nr_threads; i++) {
>              PowerPCCPU *cpu = pc->threads[i];
>              CPUState *cs = CPU(cpu);
> +            CPUPPCState *env = &cpu->env;
>  
>              if (val & PPC_BIT(7 + 8 * i)) { /* stop */
>                  val &= ~PPC_BIT(7 + 8 * i);
>                  cpu_pause(cs);
> +                env->quiesced = true;
>              }
>              if (val & PPC_BIT(6 + 8 * i)) { /* start */
>                  val &= ~PPC_BIT(6 + 8 * i);
> +                env->quiesced = false;
>                  cpu_resume(cs);
>              }
>              if (val & PPC_BIT(4 + 8 * i)) { /* sreset */
>                  val &= ~PPC_BIT(4 + 8 * i);
> +                env->quiesced = false;
>                  pnv_cpu_do_nmi_resume(cs);
>              }
>              if (val & PPC_BIT(3 + 8 * i)) { /* clear maint */
> +                env->quiesced = false;
>                  /*
>                   * Hardware has very particular cases for where
> clear maint
>                   * must be used and where start must be used to
> resume a

[PATCH 3/4] target/ppc: Fix THREAD_SIBLING_FOREACH for mult-socket

[Nicholas Piggin]

From: Glenn Miles <milesg@linux.ibm.com>

The THREAD_SIBLING_FOREACH macro wasn't excluding threads from
other chips. Add chip_index field to the thread state and add
a check for the new field in the macro.

Fixes: b769d4c8f4c6 ("target/ppc: Add initial flags and helpers for SMT support")
Signed-off-by: Glenn Miles <milesg@linux.ibm.com>
[npiggin: set chip_index for spapr too]
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h        | 7 +++++--
 hw/ppc/pnv_core.c       | 2 ++
 hw/ppc/spapr_cpu_core.c | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0b4f1013b8..2ffac2ed03 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1253,6 +1253,7 @@ struct CPUArchState {
     /* For SMT processors */
     bool has_smt_siblings;
     int core_index;
+    int chip_index;
 
 #if !defined(CONFIG_USER_ONLY)
     /* MMU context, only relevant for full system emulation */
@@ -1412,8 +1413,10 @@ struct CPUArchState {
 
 #define THREAD_SIBLING_FOREACH(cs, cs_sibling)                  \
     CPU_FOREACH(cs_sibling)                                     \
-        if (POWERPC_CPU(cs)->env.core_index ==                  \
-            POWERPC_CPU(cs_sibling)->env.core_index)
+        if ((POWERPC_CPU(cs)->env.chip_index ==                 \
+             POWERPC_CPU(cs_sibling)->env.chip_index) &&        \
+            (POWERPC_CPU(cs)->env.core_index ==                 \
+             POWERPC_CPU(cs_sibling)->env.core_index))
 
 #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
 do {                                            \
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index cbfac49862..e6b02294b1 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -322,6 +322,8 @@ static void pnv_core_cpu_realize(PnvCore *pc, PowerPCCPU *cpu, Error **errp,
     pir_spr->default_value = pir;
     tir_spr->default_value = tir;
 
+    env->chip_index = pc->chip->chip_id;
+
     if (pc->big_core) {
         /* 2 "small cores" get the same core index for SMT operations */
         env->core_index = core_hwid >> 1;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ada439e831..135f86a622 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -313,6 +313,7 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
         return NULL;
     }
 
+    env->chip_index = sc->node_id;
     env->core_index = cc->core_id;
 
     cpu->node_id = sc->node_id;
-- 
2.45.2

Re: [PATCH 3/4] target/ppc: Fix THREAD_SIBLING_FOREACH for mult-socket

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

On Mon, 2024-11-25 at 23:20 +1000, Nicholas Piggin wrote:
> From: Glenn Miles <milesg@linux.ibm.com>
> 
> The THREAD_SIBLING_FOREACH macro wasn't excluding threads from
> other chips. Add chip_index field to the thread state and add
> a check for the new field in the macro.
> 
> Fixes: b769d4c8f4c6 ("target/ppc: Add initial flags and helpers for
> SMT support")
> Signed-off-by: Glenn Miles <milesg@linux.ibm.com>
> [npiggin: set chip_index for spapr too]
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/cpu.h        | 7 +++++--
>  hw/ppc/pnv_core.c       | 2 ++
>  hw/ppc/spapr_cpu_core.c | 1 +
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 0b4f1013b8..2ffac2ed03 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1253,6 +1253,7 @@ struct CPUArchState {
>      /* For SMT processors */
>      bool has_smt_siblings;
>      int core_index;
> +    int chip_index;
>  
>  #if !defined(CONFIG_USER_ONLY)
>      /* MMU context, only relevant for full system emulation */
> @@ -1412,8 +1413,10 @@ struct CPUArchState {
>  
>  #define THREAD_SIBLING_FOREACH(cs, cs_sibling)                  \
>      CPU_FOREACH(cs_sibling)                                     \
> -        if (POWERPC_CPU(cs)->env.core_index ==                  \
> -            POWERPC_CPU(cs_sibling)->env.core_index)
> +        if ((POWERPC_CPU(cs)->env.chip_index ==                 \
> +             POWERPC_CPU(cs_sibling)->env.chip_index) &&        \
> +            (POWERPC_CPU(cs)->env.core_index ==                 \
> +             POWERPC_CPU(cs_sibling)->env.core_index))
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
>  do {                                            \
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index cbfac49862..e6b02294b1 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -322,6 +322,8 @@ static void pnv_core_cpu_realize(PnvCore *pc,
> PowerPCCPU *cpu, Error **errp,
>      pir_spr->default_value = pir;
>      tir_spr->default_value = tir;
>  
> +    env->chip_index = pc->chip->chip_id;
> +
>      if (pc->big_core) {
>          /* 2 "small cores" get the same core index for SMT
> operations */
>          env->core_index = core_hwid >> 1;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index ada439e831..135f86a622 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -313,6 +313,7 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore
> *sc, int i, Error **errp)
>          return NULL;
>      }
>  
> +    env->chip_index = sc->node_id;
>      env->core_index = cc->core_id;
>  
>      cpu->node_id = sc->node_id;

[PATCH 4/4] ppc/pnv: Add xscom- prefix to pervasive-control region name

[Nicholas Piggin]

By convention, xscom regions get a xscom- prefix.

Fixes: 1adf24708bf7 ("hw/ppc: Add pnv nest pervasive common chiplet model")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/pnv_nest_pervasive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/pnv_nest_pervasive.c b/hw/ppc/pnv_nest_pervasive.c
index 77476753a4..780fa69dde 100644
--- a/hw/ppc/pnv_nest_pervasive.c
+++ b/hw/ppc/pnv_nest_pervasive.c
@@ -177,7 +177,7 @@ static void pnv_nest_pervasive_realize(DeviceState *dev, Error **errp)
     pnv_xscom_region_init(&nest_pervasive->xscom_ctrl_regs_mr,
                           OBJECT(nest_pervasive),
                           &pnv_nest_pervasive_control_xscom_ops,
-                          nest_pervasive, "pervasive-control",
+                          nest_pervasive, "xscom-pervasive-control",
                           PNV10_XSCOM_CHIPLET_CTRL_REGS_SIZE);
 }
 
-- 
2.45.2

Re: [PATCH 4/4] ppc/pnv: Add xscom- prefix to pervasive-control region name

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

On Mon, 2024-11-25 at 23:20 +1000, Nicholas Piggin wrote:
> By convention, xscom regions get a xscom- prefix.
> 
> Fixes: 1adf24708bf7 ("hw/ppc: Add pnv nest pervasive common chiplet
> model")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/pnv_nest_pervasive.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv_nest_pervasive.c
> b/hw/ppc/pnv_nest_pervasive.c
> index 77476753a4..780fa69dde 100644
> --- a/hw/ppc/pnv_nest_pervasive.c
> +++ b/hw/ppc/pnv_nest_pervasive.c
> @@ -177,7 +177,7 @@ static void
> pnv_nest_pervasive_realize(DeviceState *dev, Error **errp)
>      pnv_xscom_region_init(&nest_pervasive->xscom_ctrl_regs_mr,
>                            OBJECT(nest_pervasive),
>                            &pnv_nest_pervasive_control_xscom_ops,
> -                          nest_pervasive, "pervasive-control",
> +                          nest_pervasive, "xscom-pervasive-control",
>                            PNV10_XSCOM_CHIPLET_CTRL_REGS_SIZE);
>  }
>  

Re: [PATCH 4/4] ppc/pnv: Add xscom- prefix to pervasive-control region name

[Philippe Mathieu-Daudé]

Hi,

On 25/11/24 14:20, Nicholas Piggin wrote:
> By convention, xscom regions get a xscom- prefix.
> 
> Fixes: 1adf24708bf7 ("hw/ppc: Add pnv nest pervasive common chiplet model")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/ppc/pnv_nest_pervasive.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv_nest_pervasive.c b/hw/ppc/pnv_nest_pervasive.c
> index 77476753a4..780fa69dde 100644
> --- a/hw/ppc/pnv_nest_pervasive.c
> +++ b/hw/ppc/pnv_nest_pervasive.c
> @@ -177,7 +177,7 @@ static void pnv_nest_pervasive_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&nest_pervasive->xscom_ctrl_regs_mr,
>                             OBJECT(nest_pervasive),
>                             &pnv_nest_pervasive_control_xscom_ops,
> -                          nest_pervasive, "pervasive-control",
> +                          nest_pervasive, "xscom-pervasive-control",

Could this break migration stream? Or only RAM regions need to
have a stable name? I don't remember, but try be be cautions with
such cosmetic change just before the release ;)

>                             PNV10_XSCOM_CHIPLET_CTRL_REGS_SIZE);
>   }
>   

Re: [PATCH 4/4] ppc/pnv: Add xscom- prefix to pervasive-control region name

[Nicholas Piggin]

On Tue Nov 26, 2024 at 5:28 AM AEST, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 25/11/24 14:20, Nicholas Piggin wrote:
> > By convention, xscom regions get a xscom- prefix.
> > 
> > Fixes: 1adf24708bf7 ("hw/ppc: Add pnv nest pervasive common chiplet model")
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/ppc/pnv_nest_pervasive.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/pnv_nest_pervasive.c b/hw/ppc/pnv_nest_pervasive.c
> > index 77476753a4..780fa69dde 100644
> > --- a/hw/ppc/pnv_nest_pervasive.c
> > +++ b/hw/ppc/pnv_nest_pervasive.c
> > @@ -177,7 +177,7 @@ static void pnv_nest_pervasive_realize(DeviceState *dev, Error **errp)
> >       pnv_xscom_region_init(&nest_pervasive->xscom_ctrl_regs_mr,
> >                             OBJECT(nest_pervasive),
> >                             &pnv_nest_pervasive_control_xscom_ops,
> > -                          nest_pervasive, "pervasive-control",
> > +                          nest_pervasive, "xscom-pervasive-control",
>
> Could this break migration stream? Or only RAM regions need to
> have a stable name? I don't remember, but try be be cautions with
> such cosmetic change just before the release ;)

Hey Phil,

Thanks for always somehow being across everything :)

For the powernv machine we are okay since we don't support migration
at all. It's on the wishlist, but at the moment we have lots of
hardware models that don't provide vmstate. So I think we are okay.

Thanks,
Nick

Re: [PATCH 4/4] ppc/pnv: Add xscom- prefix to pervasive-control region name

[Philippe Mathieu-Daudé]

On 26/11/24 04:54, Nicholas Piggin wrote:
> On Tue Nov 26, 2024 at 5:28 AM AEST, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> On 25/11/24 14:20, Nicholas Piggin wrote:
>>> By convention, xscom regions get a xscom- prefix.
>>>
>>> Fixes: 1adf24708bf7 ("hw/ppc: Add pnv nest pervasive common chiplet model")
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    hw/ppc/pnv_nest_pervasive.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/pnv_nest_pervasive.c b/hw/ppc/pnv_nest_pervasive.c
>>> index 77476753a4..780fa69dde 100644
>>> --- a/hw/ppc/pnv_nest_pervasive.c
>>> +++ b/hw/ppc/pnv_nest_pervasive.c
>>> @@ -177,7 +177,7 @@ static void pnv_nest_pervasive_realize(DeviceState *dev, Error **errp)
>>>        pnv_xscom_region_init(&nest_pervasive->xscom_ctrl_regs_mr,
>>>                              OBJECT(nest_pervasive),
>>>                              &pnv_nest_pervasive_control_xscom_ops,
>>> -                          nest_pervasive, "pervasive-control",
>>> +                          nest_pervasive, "xscom-pervasive-control",
>>
>> Could this break migration stream? Or only RAM regions need to
>> have a stable name? I don't remember, but try be be cautions with
>> such cosmetic change just before the release ;)
> 
> Hey Phil,
> 
> Thanks for always somehow being across everything :)

;)

Answering myself, I/O regions are migrated within the device state,
per field, via DeviceClass::vmsd structure.

IIRC RAM regions are the ones tied to their name, which is built
using the object owner, which is why we can't change migrated RAM
MR owners.

> For the powernv machine we are okay since we don't support migration
> at all. It's on the wishlist, but at the moment we have lots of
> hardware models that don't provide vmstate. So I think we are okay.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/4] ppc/pnv: Add xscom- prefix to pervasive-control region name

[Peter Xu]

On Tue, Nov 26, 2024 at 06:19:52AM +0100, Philippe Mathieu-Daudé wrote:
> On 26/11/24 04:54, Nicholas Piggin wrote:
> > On Tue Nov 26, 2024 at 5:28 AM AEST, Philippe Mathieu-Daudé wrote:
> > > Hi,
> > > 
> > > On 25/11/24 14:20, Nicholas Piggin wrote:
> > > > By convention, xscom regions get a xscom- prefix.
> > > > 
> > > > Fixes: 1adf24708bf7 ("hw/ppc: Add pnv nest pervasive common chiplet model")
> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > ---
> > > >    hw/ppc/pnv_nest_pervasive.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/ppc/pnv_nest_pervasive.c b/hw/ppc/pnv_nest_pervasive.c
> > > > index 77476753a4..780fa69dde 100644
> > > > --- a/hw/ppc/pnv_nest_pervasive.c
> > > > +++ b/hw/ppc/pnv_nest_pervasive.c
> > > > @@ -177,7 +177,7 @@ static void pnv_nest_pervasive_realize(DeviceState *dev, Error **errp)
> > > >        pnv_xscom_region_init(&nest_pervasive->xscom_ctrl_regs_mr,
> > > >                              OBJECT(nest_pervasive),
> > > >                              &pnv_nest_pervasive_control_xscom_ops,
> > > > -                          nest_pervasive, "pervasive-control",
> > > > +                          nest_pervasive, "xscom-pervasive-control",
> > > 
> > > Could this break migration stream? Or only RAM regions need to
> > > have a stable name? I don't remember, but try be be cautions with
> > > such cosmetic change just before the release ;)
> > 
> > Hey Phil,
> > 
> > Thanks for always somehow being across everything :)
> 
> ;)
> 
> Answering myself, I/O regions are migrated within the device state,
> per field, via DeviceClass::vmsd structure.

IIUC by default we don't migrate IO regions at all, unless we register
something into the VMSD explicitly.

> 
> IIRC RAM regions are the ones tied to their name, which is built
> using the object owner, which is why we can't change migrated RAM
> MR owners.

True.

Thanks,

-- 
Peter Xu