[PATCH 0/7] ppc: pnv ChipTOD and various timebase fixes

[PATCH 0/7] ppc: pnv ChipTOD and various timebase fixes

[Nicholas Piggin]

The chiptod/TFMR/state machine is not really tied to the other
time register fixes, but they touch some of the same code, and
logically same facility.

Changes since v1 of chiptod patches:
- Split hackish ChipTOD<->TFMR/TBST interface into its own patch
- Fix multi-socket addressing on P9 / chip ID mode (P10 works)
- Change chiptod primary/secondary setting to use class properties
- Add more comments to explain TOD overview and timebase state
  machine.
- SMT support for TFMR, some functionality is limited to thread 0.
- FIRMWARE_CONTROL_ERROR bit implemented in TFMR.
- Misc cleanups and bug fixes.

The hacky part, addressing core from chiptod, is still hacky. Is
there strong objection to it?

This successfully runs skiboot chiptod initialisation code with
POWER9 and POWER10 multi-socket, multi-core, SMT. That requires
skiboot 7.1 (not in-tree), otherwise chiptod init is skipped on
QEMU machines.

Thanks,
Nick

Nicholas Piggin (7):
  target/ppc: Rename TBL to TB on 64-bit
  target/ppc: Improve timebase register defines naming
  target/ppc: Fix move-to timebase SPR access permissions
  pnv/chiptod: Add POWER9/10 chiptod model
  pnv/chiptod: Implement the ChipTOD to Core transfer
  target/ppc: Implement core timebase state machine and TFMR
  target/ppc: Add SMT support to time facilities

 include/hw/ppc/pnv_chip.h    |   3 +
 include/hw/ppc/pnv_chiptod.h |  55 ++++
 include/hw/ppc/pnv_core.h    |   4 +
 include/hw/ppc/pnv_xscom.h   |   9 +
 target/ppc/cpu.h             |  50 +++-
 hw/ppc/pnv.c                 |  63 +++++
 hw/ppc/pnv_chiptod.c         | 509 +++++++++++++++++++++++++++++++++++
 target/ppc/helper_regs.c     |  39 ++-
 target/ppc/ppc-qmp-cmds.c    |   4 +
 target/ppc/timebase_helper.c | 309 ++++++++++++++++++++-
 target/ppc/translate.c       |  42 ++-
 hw/ppc/meson.build           |   1 +
 hw/ppc/trace-events          |   4 +
 13 files changed, 1067 insertions(+), 25 deletions(-)
 create mode 100644 include/hw/ppc/pnv_chiptod.h
 create mode 100644 hw/ppc/pnv_chiptod.c

-- 
2.42.0

Re: [PATCH 0/7] ppc: pnv ChipTOD and various timebase fixes

[Cédric Le Goater]

On 11/23/23 11:30, Nicholas Piggin wrote:
> The chiptod/TFMR/state machine is not really tied to the other
> time register fixes, but they touch some of the same code, and
> logically same facility.
> 
> Changes since v1 of chiptod patches:
> - Split hackish ChipTOD<->TFMR/TBST interface into its own patch
> - Fix multi-socket addressing on P9 / chip ID mode (P10 works)
> - Change chiptod primary/secondary setting to use class properties
> - Add more comments to explain TOD overview and timebase state
>    machine.
> - SMT support for TFMR, some functionality is limited to thread 0.
> - FIRMWARE_CONTROL_ERROR bit implemented in TFMR.
> - Misc cleanups and bug fixes.
> 
> The hacky part, addressing core from chiptod, is still hacky. Is
> there strong objection to it?

Dunno yet :)

> This successfully runs skiboot chiptod initialisation code with
> POWER9 and POWER10 multi-socket, multi-core, SMT. That requires
> skiboot 7.1 (not in-tree), otherwise chiptod init is skipped on
> QEMU machines.

Let's update skiboot at the same time then.

Thanks,

C.

> 
> Thanks,
> Nick
> 
> Nicholas Piggin (7):
>    target/ppc: Rename TBL to TB on 64-bit
>    target/ppc: Improve timebase register defines naming
>    target/ppc: Fix move-to timebase SPR access permissions
>    pnv/chiptod: Add POWER9/10 chiptod model
>    pnv/chiptod: Implement the ChipTOD to Core transfer
>    target/ppc: Implement core timebase state machine and TFMR
>    target/ppc: Add SMT support to time facilities
> 
>   include/hw/ppc/pnv_chip.h    |   3 +
>   include/hw/ppc/pnv_chiptod.h |  55 ++++
>   include/hw/ppc/pnv_core.h    |   4 +
>   include/hw/ppc/pnv_xscom.h   |   9 +
>   target/ppc/cpu.h             |  50 +++-
>   hw/ppc/pnv.c                 |  63 +++++
>   hw/ppc/pnv_chiptod.c         | 509 +++++++++++++++++++++++++++++++++++
>   target/ppc/helper_regs.c     |  39 ++-
>   target/ppc/ppc-qmp-cmds.c    |   4 +
>   target/ppc/timebase_helper.c | 309 ++++++++++++++++++++-
>   target/ppc/translate.c       |  42 ++-
>   hw/ppc/meson.build           |   1 +
>   hw/ppc/trace-events          |   4 +
>   13 files changed, 1067 insertions(+), 25 deletions(-)
>   create mode 100644 include/hw/ppc/pnv_chiptod.h
>   create mode 100644 hw/ppc/pnv_chiptod.c
> 

Re: [PATCH 0/7] ppc: pnv ChipTOD and various timebase fixes

[Nicholas Piggin]

On Fri Nov 24, 2023 at 3:09 AM AEST, Cédric Le Goater wrote:
> On 11/23/23 11:30, Nicholas Piggin wrote:
> > The chiptod/TFMR/state machine is not really tied to the other
> > time register fixes, but they touch some of the same code, and
> > logically same facility.
> > 
> > Changes since v1 of chiptod patches:
> > - Split hackish ChipTOD<->TFMR/TBST interface into its own patch
> > - Fix multi-socket addressing on P9 / chip ID mode (P10 works)
> > - Change chiptod primary/secondary setting to use class properties
> > - Add more comments to explain TOD overview and timebase state
> >    machine.
> > - SMT support for TFMR, some functionality is limited to thread 0.
> > - FIRMWARE_CONTROL_ERROR bit implemented in TFMR.
> > - Misc cleanups and bug fixes.
> > 
> > The hacky part, addressing core from chiptod, is still hacky. Is
> > there strong objection to it?
>
> Dunno yet :)

Thanks for the nice review!

> > This successfully runs skiboot chiptod initialisation code with
> > POWER9 and POWER10 multi-socket, multi-core, SMT. That requires
> > skiboot 7.1 (not in-tree), otherwise chiptod init is skipped on
> > QEMU machines.
>
> Let's update skiboot at the same time then.

Yeah, I'll update skiboot ahead of adding merging this.

Thanks,
Nick

[PATCH] target/ppc: Re-name registers to match ISA

[Nicholas Piggin]

Several registers have names that don't match the ISA (or convention
with other QEMU PPC registers), making them unintuitive to use with
GDB.

Fortunately most of these registers are obscure and/or have not been
correctly implemented in the gdb server (e.g., DEC, TB, CFAR), so risk
of breaking users should be low.

QEMU should follow the ISA for register name convention (where there is
no established GDB name).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
There is never a great time to change user interface, but I'd like to
make this change for 9.0.

Thanks,
Nick

 target/ppc/cpu_init.c    | 20 ++++++++++----------
 target/ppc/helper_regs.c |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 40fe14a6c2..15c1f2fdc8 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -5062,7 +5062,7 @@ static void register_970_hid_sprs(CPUPPCState *env)
 
 static void register_970_hior_sprs(CPUPPCState *env)
 {
-    spr_register(env, SPR_HIOR, "SPR_HIOR",
+    spr_register(env, SPR_HIOR, "HIOR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_hior, &spr_write_hior,
                  0x00000000);
@@ -5070,11 +5070,11 @@ static void register_970_hior_sprs(CPUPPCState *env)
 
 static void register_book3s_ctrl_sprs(CPUPPCState *env)
 {
-    spr_register(env, SPR_CTRL, "SPR_CTRL",
+    spr_register(env, SPR_CTRL, "CTRL",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, &spr_write_CTRL,
                  0x00000000);
-    spr_register(env, SPR_UCTRL, "SPR_UCTRL",
+    spr_register(env, SPR_UCTRL, "UCTRL",
                  &spr_read_ureg, SPR_NOACCESS,
                  &spr_read_ureg, SPR_NOACCESS,
                  0x00000000);
@@ -5465,7 +5465,7 @@ static void register_book3s_purr_sprs(CPUPPCState *env)
 static void register_power6_dbg_sprs(CPUPPCState *env)
 {
 #if !defined(CONFIG_USER_ONLY)
-    spr_register(env, SPR_CFAR, "SPR_CFAR",
+    spr_register(env, SPR_CFAR, "CFAR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_cfar, &spr_write_cfar,
                  0x00000000);
@@ -5483,7 +5483,7 @@ static void register_power5p_common_sprs(CPUPPCState *env)
 static void register_power6_common_sprs(CPUPPCState *env)
 {
 #if !defined(CONFIG_USER_ONLY)
-    spr_register_kvm(env, SPR_DSCR, "SPR_DSCR",
+    spr_register_kvm(env, SPR_DSCR, "DSCR",
                      SPR_NOACCESS, SPR_NOACCESS,
                      &spr_read_generic, &spr_write_generic,
                      KVM_REG_PPC_DSCR, 0x00000000);
@@ -5695,7 +5695,7 @@ static void register_power8_book4_sprs(CPUPPCState *env)
                      &spr_read_generic, &spr_write_generic,
                      KVM_REG_PPC_ACOP, 0);
     /* PID is only in BookE in ISA v2.07 */
-    spr_register_kvm(env, SPR_BOOKS_PID, "PID",
+    spr_register_kvm(env, SPR_BOOKS_PID, "PIDR",
                      SPR_NOACCESS, SPR_NOACCESS,
                      &spr_read_generic, &spr_write_pidr,
                      KVM_REG_PPC_PID, 0);
@@ -5716,7 +5716,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
                      &spr_read_generic, &spr_write_generic,
                      KVM_REG_PPC_ACOP, 0);
     /* PID is only in BookE in ISA v2.06 */
-    spr_register_kvm(env, SPR_BOOKS_PID, "PID",
+    spr_register_kvm(env, SPR_BOOKS_PID, "PIDR",
                      SPR_NOACCESS, SPR_NOACCESS,
                      &spr_read_generic, &spr_write_generic32,
                      KVM_REG_PPC_PID, 0);
@@ -5750,7 +5750,7 @@ static void register_power9_mmu_sprs(CPUPPCState *env)
                     &spr_read_generic, &spr_write_generic,
                     0x0000000000000000);
     /* PID is part of the BookS ISA from v3.0 */
-    spr_register_kvm(env, SPR_BOOKS_PID, "PID",
+    spr_register_kvm(env, SPR_BOOKS_PID, "PIDR",
                      SPR_NOACCESS, SPR_NOACCESS,
                      &spr_read_generic, &spr_write_pidr,
                      KVM_REG_PPC_PID, 0);
@@ -5791,7 +5791,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
             &spr_read_generic, &spr_write_generic32,
             0);
 
-    spr_register(env, SPR_UDEXCR, "DEXCR",
+    spr_register(env, SPR_UDEXCR, "UDEXCR",
             &spr_read_dexcr_ureg, SPR_NOACCESS,
             &spr_read_dexcr_ureg, SPR_NOACCESS,
             0);
@@ -5802,7 +5802,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
             &spr_read_generic, &spr_write_generic32,
             0);
 
-    spr_register(env, SPR_UHDEXCR, "HDEXCR",
+    spr_register(env, SPR_UHDEXCR, "UHDEXCR",
             &spr_read_dexcr_ureg, SPR_NOACCESS,
             &spr_read_dexcr_ureg, SPR_NOACCESS,
             0);
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index f1493ddca0..59cf3dfaae 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -509,7 +509,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
                      &spr_read_generic, &spr_write_generic,
                      KVM_REG_PPC_DAR, 0x00000000);
     /* Timer */
-    spr_register(env, SPR_DECR, "DECR",
+    spr_register(env, SPR_DECR, "DEC",
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_decr, &spr_write_decr,
                  0x00000000);
-- 
2.42.0

[PATCH 0/7] ppc: pnv ChipTOD and various timebase fixes

[Nicholas Piggin]

The chiptod/TFMR/state machine is not really tied to the other
time register fixes, but they touch some of the same code, and
logically same facility.

Changes since v1 of chiptod patches:
- Split hackish ChipTOD<->TFMR/TBST interface into its own patch
- Fix multi-socket addressing on P9 / chip ID mode (P10 works)
- Change chiptod primary/secondary setting to use class properties
- Add more comments to explain TOD overview and timebase state
  machine.
- SMT support for TFMR, some functionality is limited to thread 0.
- FIRMWARE_CONTROL_ERROR bit implemented in TFMR.
- Misc cleanups and bug fixes.

The hacky part, addressing core from chiptod, is still hacky. Is
there strong objection to it?

This successfully runs skiboot chiptod initialisation code with
POWER9 and POWER10 multi-socket, multi-core, SMT. That requires
skiboot 7.1 (not in-tree), otherwise chiptod init is skipped on
QEMU machines.

Thanks,
Nick

Nicholas Piggin (7):
  target/ppc: Rename TBL to TB on 64-bit
  target/ppc: Improve timebase register defines naming
  target/ppc: Fix move-to timebase SPR access permissions
  pnv/chiptod: Add POWER9/10 chiptod model
  pnv/chiptod: Implement the ChipTOD to Core transfer
  target/ppc: Implement core timebase state machine and TFMR
  target/ppc: Add SMT support to time facilities

 include/hw/ppc/pnv_chip.h    |   3 +
 include/hw/ppc/pnv_chiptod.h |  55 ++++
 include/hw/ppc/pnv_core.h    |   4 +
 include/hw/ppc/pnv_xscom.h   |   9 +
 target/ppc/cpu.h             |  50 +++-
 hw/ppc/pnv.c                 |  63 +++++
 hw/ppc/pnv_chiptod.c         | 509 +++++++++++++++++++++++++++++++++++
 target/ppc/helper_regs.c     |  39 ++-
 target/ppc/ppc-qmp-cmds.c    |   4 +
 target/ppc/timebase_helper.c | 309 ++++++++++++++++++++-
 target/ppc/translate.c       |  42 ++-
 hw/ppc/meson.build           |   1 +
 hw/ppc/trace-events          |   4 +
 13 files changed, 1067 insertions(+), 25 deletions(-)
 create mode 100644 include/hw/ppc/pnv_chiptod.h
 create mode 100644 hw/ppc/pnv_chiptod.c

-- 
2.42.0

Re: [PATCH] target/ppc: Re-name registers to match ISA

[Cédric Le Goater]

On 12/1/23 13:24, Nicholas Piggin wrote:
> Several registers have names that don't match the ISA (or convention
> with other QEMU PPC registers), making them unintuitive to use with
> GDB.
> 
> Fortunately most of these registers are obscure and/or have not been
> correctly implemented in the gdb server (e.g., DEC, TB, CFAR), so risk
> of breaking users should be low.
> 
> QEMU should follow the ISA for register name convention (where there is
> no established GDB name).
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>


Acked-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
> There is never a great time to change user interface, but I'd like to
> make this change for 9.0.
> 
> Thanks,
> Nick
> 
>   target/ppc/cpu_init.c    | 20 ++++++++++----------
>   target/ppc/helper_regs.c |  2 +-
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 40fe14a6c2..15c1f2fdc8 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5062,7 +5062,7 @@ static void register_970_hid_sprs(CPUPPCState *env)
>   
>   static void register_970_hior_sprs(CPUPPCState *env)
>   {
> -    spr_register(env, SPR_HIOR, "SPR_HIOR",
> +    spr_register(env, SPR_HIOR, "HIOR",
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    &spr_read_hior, &spr_write_hior,
>                    0x00000000);
> @@ -5070,11 +5070,11 @@ static void register_970_hior_sprs(CPUPPCState *env)
>   
>   static void register_book3s_ctrl_sprs(CPUPPCState *env)
>   {
> -    spr_register(env, SPR_CTRL, "SPR_CTRL",
> +    spr_register(env, SPR_CTRL, "CTRL",
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    SPR_NOACCESS, &spr_write_CTRL,
>                    0x00000000);
> -    spr_register(env, SPR_UCTRL, "SPR_UCTRL",
> +    spr_register(env, SPR_UCTRL, "UCTRL",
>                    &spr_read_ureg, SPR_NOACCESS,
>                    &spr_read_ureg, SPR_NOACCESS,
>                    0x00000000);
> @@ -5465,7 +5465,7 @@ static void register_book3s_purr_sprs(CPUPPCState *env)
>   static void register_power6_dbg_sprs(CPUPPCState *env)
>   {
>   #if !defined(CONFIG_USER_ONLY)
> -    spr_register(env, SPR_CFAR, "SPR_CFAR",
> +    spr_register(env, SPR_CFAR, "CFAR",
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    &spr_read_cfar, &spr_write_cfar,
>                    0x00000000);
> @@ -5483,7 +5483,7 @@ static void register_power5p_common_sprs(CPUPPCState *env)
>   static void register_power6_common_sprs(CPUPPCState *env)
>   {
>   #if !defined(CONFIG_USER_ONLY)
> -    spr_register_kvm(env, SPR_DSCR, "SPR_DSCR",
> +    spr_register_kvm(env, SPR_DSCR, "DSCR",
>                        SPR_NOACCESS, SPR_NOACCESS,
>                        &spr_read_generic, &spr_write_generic,
>                        KVM_REG_PPC_DSCR, 0x00000000);
> @@ -5695,7 +5695,7 @@ static void register_power8_book4_sprs(CPUPPCState *env)
>                        &spr_read_generic, &spr_write_generic,
>                        KVM_REG_PPC_ACOP, 0);
>       /* PID is only in BookE in ISA v2.07 */
> -    spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> +    spr_register_kvm(env, SPR_BOOKS_PID, "PIDR",
>                        SPR_NOACCESS, SPR_NOACCESS,
>                        &spr_read_generic, &spr_write_pidr,
>                        KVM_REG_PPC_PID, 0);
> @@ -5716,7 +5716,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
>                        &spr_read_generic, &spr_write_generic,
>                        KVM_REG_PPC_ACOP, 0);
>       /* PID is only in BookE in ISA v2.06 */
> -    spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> +    spr_register_kvm(env, SPR_BOOKS_PID, "PIDR",
>                        SPR_NOACCESS, SPR_NOACCESS,
>                        &spr_read_generic, &spr_write_generic32,
>                        KVM_REG_PPC_PID, 0);
> @@ -5750,7 +5750,7 @@ static void register_power9_mmu_sprs(CPUPPCState *env)
>                       &spr_read_generic, &spr_write_generic,
>                       0x0000000000000000);
>       /* PID is part of the BookS ISA from v3.0 */
> -    spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> +    spr_register_kvm(env, SPR_BOOKS_PID, "PIDR",
>                        SPR_NOACCESS, SPR_NOACCESS,
>                        &spr_read_generic, &spr_write_pidr,
>                        KVM_REG_PPC_PID, 0);
> @@ -5791,7 +5791,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
>               &spr_read_generic, &spr_write_generic32,
>               0);
>   
> -    spr_register(env, SPR_UDEXCR, "DEXCR",
> +    spr_register(env, SPR_UDEXCR, "UDEXCR",
>               &spr_read_dexcr_ureg, SPR_NOACCESS,
>               &spr_read_dexcr_ureg, SPR_NOACCESS,
>               0);
> @@ -5802,7 +5802,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
>               &spr_read_generic, &spr_write_generic32,
>               0);
>   
> -    spr_register(env, SPR_UHDEXCR, "HDEXCR",
> +    spr_register(env, SPR_UHDEXCR, "UHDEXCR",
>               &spr_read_dexcr_ureg, SPR_NOACCESS,
>               &spr_read_dexcr_ureg, SPR_NOACCESS,
>               0);
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index f1493ddca0..59cf3dfaae 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -509,7 +509,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
>                        &spr_read_generic, &spr_write_generic,
>                        KVM_REG_PPC_DAR, 0x00000000);
>       /* Timer */
> -    spr_register(env, SPR_DECR, "DECR",
> +    spr_register(env, SPR_DECR, "DEC",
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    &spr_read_decr, &spr_write_decr,
>                    0x00000000);