[PULL 00/29] ppc queue

[PULL 00/29] ppc queue

[Daniel Henrique Barboza]

The following changes since commit 3673ad389622d9ef4d2743101253c642def7935a:

  tcg/tci: Fix MemOpIdx operand index for 3-operand memops (2023-06-09 08:30:56 -0700)

are available in the Git repository at:

  https://gitlab.com/danielhb/qemu.git tags/pull-ppc-20230610

for you to fetch changes up to 9ec08f3569be3bc8bfd4d9b8b0445b9136910661:

  hw/ppc/Kconfig: MAC_NEWWORLD should always select USB_OHCI_PCI (2023-06-10 10:19:24 -0300)

----------------------------------------------------------------
ppc patch queue for 2023-06-10:

This queue includes several assorted fixes for target/ppc emulation and
XIVE2. It also includes an openpic fix, an avocado fix for ppc64
binaries without slipr and a Kconfig change for MAC_NEWWORLD.

----------------------------------------------------------------
BALATON Zoltan (8):
      target/ppc: Remove single use function
      target/ppc: Remove "ext" parameter of ppcemb_tlb_check()
      target/ppc: Move ppcemb_tlb_search() to mmu_common.c
      target/ppc: Remove some unneded line breaks
      target/ppc: Simplify ppcemb_tlb_search()
      target/ppc: Change ppcemb_tlb_check() to return bool
      target/ppc: Eliminate goto in mmubooke_check_tlb()
      target/ppc: Implement gathering irq statistics

Frederic Barrat (6):
      pnv/xive2: Add definition for TCTXT Config register
      pnv/xive2: Add definition for the ESB cache configuration register
      pnv/xive2: Allow writes to the Physical Thread Enable registers
      pnv/xive2: Introduce macros to manipulate TIMA addresses
      pnv/xive2: Handle TIMA access through all ports
      pnv/xive2: Quiet down some error messages

Nicholas Piggin (12):
      target/ppc: Fix nested-hv HEAI delivery
      target/ppc: Fix PMU hflags calculation
      target/ppc: PMU do not clear MMCR0[FCECE] on performance monitor alert
      target/ppc: Fix msgclrp interrupt type
      target/ppc: Support directed privileged doorbell interrupt (SDOOR)
      target/ppc: PMU implement PERFM interrupts
      target/ppc: Fix lqarx to set cpu_reserve
      target/ppc: Ensure stcx size matches larx
      target/ppc: Remove larx/stcx. memory barrier semantics
      target/ppc: Rework store conditional to avoid branch
      target/ppc: Fix decrementer time underflow and infinite timer loop
      target/ppc: Decrementer fix BookE semantics

Philippe Mathieu-Daudé (1):
      hw/ppc/openpic: Do not open-code ROUND_UP() macro

Thomas Huth (2):
      tests/avocado/tuxrun_baselines: Fix ppc64 tests for binaries without slirp
      hw/ppc/Kconfig: MAC_NEWWORLD should always select USB_OHCI_PCI

 hw/intc/pnv_xive2.c               | 24 ++++++++++-
 hw/intc/pnv_xive2_regs.h          |  8 ++++
 hw/intc/xive.c                    | 16 +++----
 hw/ppc/Kconfig                    |  1 +
 hw/ppc/ppc.c                      | 11 ++---
 include/hw/ppc/openpic.h          |  2 +-
 include/hw/ppc/xive_regs.h        | 16 +++++++
 target/ppc/cpu.h                  | 19 ++++----
 target/ppc/cpu_init.c             | 24 +++++++++--
 target/ppc/excp_helper.c          | 14 +++---
 target/ppc/helper_regs.c          | 73 +++++++++++++++++++++++--------
 target/ppc/helper_regs.h          |  1 +
 target/ppc/machine.c              |  8 ++--
 target/ppc/mmu_common.c           | 91 ++++++++++++++++++++++-----------------
 target/ppc/mmu_helper.c           | 32 +-------------
 target/ppc/power8-pmu.c           | 60 +++++++++++++++-----------
 target/ppc/power8-pmu.h           |  4 +-
 target/ppc/translate.c            | 80 ++++++++++++++++------------------
 tests/avocado/tuxrun_baselines.py |  1 +
 19 files changed, 289 insertions(+), 196 deletions(-)

[PULL 01/29] pnv/xive2: Add definition for TCTXT Config register

[Daniel Henrique Barboza]

From: Frederic Barrat <fbarrat@linux.ibm.com>

Add basic read/write support for the TCTXT Config register on P10. qemu
doesn't do anything with it yet, but it avoids logging a guest error
when skiboot configures the fused-core state:

qemu-system-ppc64 -machine powernv10 ... -d guest_errors
  ...
[    0.131670000,5] XIVE: [ IC 00  ] Initializing XIVE block ID 0...
XIVE[0] - TCTXT: invalid read @140
XIVE[0] - TCTXT: invalid write @140

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230601121331.487207-2-fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/intc/pnv_xive2.c      | 8 +++++++-
 hw/intc/pnv_xive2_regs.h | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index 7176d70234..889e409929 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -1265,6 +1265,9 @@ static uint64_t pnv_xive2_ic_tctxt_read(void *opaque, hwaddr offset,
     case TCTXT_EN1_RESET:
         val = xive->tctxt_regs[TCTXT_EN1 >> 3];
         break;
+    case TCTXT_CFG:
+        val = xive->tctxt_regs[reg];
+        break;
     default:
         xive2_error(xive, "TCTXT: invalid read @%"HWADDR_PRIx, offset);
     }
@@ -1276,6 +1279,7 @@ static void pnv_xive2_ic_tctxt_write(void *opaque, hwaddr offset,
                                      uint64_t val, unsigned size)
 {
     PnvXive2 *xive = PNV_XIVE2(opaque);
+    uint32_t reg = offset >> 3;
 
     switch (offset) {
     /*
@@ -1297,7 +1301,9 @@ static void pnv_xive2_ic_tctxt_write(void *opaque, hwaddr offset,
     case TCTXT_EN1_RESET:
         xive->tctxt_regs[TCTXT_EN1 >> 3] &= ~val;
         break;
-
+    case TCTXT_CFG:
+        xive->tctxt_regs[reg] = val;
+        break;
     default:
         xive2_error(xive, "TCTXT: invalid write @%"HWADDR_PRIx, offset);
         return;
diff --git a/hw/intc/pnv_xive2_regs.h b/hw/intc/pnv_xive2_regs.h
index 0c096e4adb..8f1e0a1fde 100644
--- a/hw/intc/pnv_xive2_regs.h
+++ b/hw/intc/pnv_xive2_regs.h
@@ -405,6 +405,10 @@
 #define X_TCTXT_EN1_RESET                       0x307
 #define TCTXT_EN1_RESET                         0x038
 
+/* TCTXT Config register */
+#define X_TCTXT_CFG                             0x328
+#define TCTXT_CFG                               0x140
+
 /*
  * VSD Tables
  */
-- 
2.40.1

[PULL 02/29] pnv/xive2: Add definition for the ESB cache configuration register

[Daniel Henrique Barboza]

From: Frederic Barrat <fbarrat@linux.ibm.com>

Add basic read/write support for the ESB cache configuration register
on P10. We don't model the ESB cache in qemu so reading/writing the
register won't do anything, but it avoids logging a guest error when
skiboot configures it:

qemu-system-ppc64 -machine powernv10 ... -d guest_errors
      ...
XIVE[0] - VC: invalid read @240
XIVE[0] - VC: invalid write @240

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230601121331.487207-3-fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/intc/pnv_xive2.c      | 7 +++++++
 hw/intc/pnv_xive2_regs.h | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index 889e409929..a75ff270ac 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -955,6 +955,10 @@ static uint64_t pnv_xive2_ic_vc_read(void *opaque, hwaddr offset,
         val = xive->vc_regs[reg];
         break;
 
+    case VC_ESBC_CFG:
+        val = xive->vc_regs[reg];
+        break;
+
     /*
      * EAS cache updates (not modeled)
      */
@@ -1046,6 +1050,9 @@ static void pnv_xive2_ic_vc_write(void *opaque, hwaddr offset,
         /* ESB update */
         break;
 
+    case VC_ESBC_CFG:
+        break;
+
     /*
      * EAS cache updates (not modeled)
      */
diff --git a/hw/intc/pnv_xive2_regs.h b/hw/intc/pnv_xive2_regs.h
index 8f1e0a1fde..7165dc8704 100644
--- a/hw/intc/pnv_xive2_regs.h
+++ b/hw/intc/pnv_xive2_regs.h
@@ -232,6 +232,10 @@
 #define  VC_ESBC_FLUSH_POLL_BLOCK_ID_MASK       PPC_BITMASK(32, 35)
 #define  VC_ESBC_FLUSH_POLL_OFFSET_MASK         PPC_BITMASK(36, 63) /* 28-bit */
 
+/* ESBC configuration */
+#define X_VC_ESBC_CFG                           0x148
+#define VC_ESBC_CFG                             0x240
+
 /* EASC flush control register */
 #define X_VC_EASC_FLUSH_CTRL                    0x160
 #define VC_EASC_FLUSH_CTRL                      0x300
-- 
2.40.1

[PULL 03/29] pnv/xive2: Allow writes to the Physical Thread Enable registers

[Daniel Henrique Barboza]

From: Frederic Barrat <fbarrat@linux.ibm.com>

Fix what was probably a silly mistake and allow to write the Physical
Thread enable registers 0 and 1. Skiboot prefers to use the ENx_SET
variant so it went unnoticed, but there's no reason to discard a write
to the full register, it is Read-Write.

Fixes: da71b7e3ed45 ("ppc/pnv: Add a XIVE2 controller to the POWER10 chip")
Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230601121331.487207-4-fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/intc/pnv_xive2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index a75ff270ac..132f82a035 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -1294,6 +1294,7 @@ static void pnv_xive2_ic_tctxt_write(void *opaque, hwaddr offset,
      */
     case TCTXT_EN0: /* Physical Thread Enable */
     case TCTXT_EN1: /* Physical Thread Enable (fused core) */
+        xive->tctxt_regs[reg] = val;
         break;
 
     case TCTXT_EN0_SET:
-- 
2.40.1

[PULL 04/29] pnv/xive2: Introduce macros to manipulate TIMA addresses

[Daniel Henrique Barboza]

From: Frederic Barrat <fbarrat@linux.ibm.com>

TIMA addresses are somewhat special and are split in several bit
fields with different meanings. This patch describes it and introduce
macros to more easily access the various fields.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230601121331.487207-5-fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/intc/xive.c             | 14 +++++++-------
 include/hw/ppc/xive_regs.h | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index a986b96843..ebe399bc09 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -249,7 +249,7 @@ static const uint8_t *xive_tm_views[] = {
 static uint64_t xive_tm_mask(hwaddr offset, unsigned size, bool write)
 {
     uint8_t page_offset = (offset >> TM_SHIFT) & 0x3;
-    uint8_t reg_offset = offset & 0x3F;
+    uint8_t reg_offset = offset & TM_REG_OFFSET;
     uint8_t reg_mask = write ? 0x1 : 0x2;
     uint64_t mask = 0x0;
     int i;
@@ -266,8 +266,8 @@ static uint64_t xive_tm_mask(hwaddr offset, unsigned size, bool write)
 static void xive_tm_raw_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
                               unsigned size)
 {
-    uint8_t ring_offset = offset & 0x30;
-    uint8_t reg_offset = offset & 0x3F;
+    uint8_t ring_offset = offset & TM_RING_OFFSET;
+    uint8_t reg_offset = offset & TM_REG_OFFSET;
     uint64_t mask = xive_tm_mask(offset, size, true);
     int i;
 
@@ -296,8 +296,8 @@ static void xive_tm_raw_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
 
 static uint64_t xive_tm_raw_read(XiveTCTX *tctx, hwaddr offset, unsigned size)
 {
-    uint8_t ring_offset = offset & 0x30;
-    uint8_t reg_offset = offset & 0x3F;
+    uint8_t ring_offset = offset & TM_RING_OFFSET;
+    uint8_t reg_offset = offset & TM_REG_OFFSET;
     uint64_t mask = xive_tm_mask(offset, size, false);
     uint64_t ret;
     int i;
@@ -534,7 +534,7 @@ void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
     /*
      * First, check for special operations in the 2K region
      */
-    if (offset & 0x800) {
+    if (offset & TM_SPECIAL_OP) {
         xto = xive_tm_find_op(offset, size, true);
         if (!xto) {
             qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid write access at TIMA "
@@ -573,7 +573,7 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
     /*
      * First, check for special operations in the 2K region
      */
-    if (offset & 0x800) {
+    if (offset & TM_SPECIAL_OP) {
         xto = xive_tm_find_op(offset, size, false);
         if (!xto) {
             qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid read access to TIMA"
diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
index b7fde2354e..4a3c9badd3 100644
--- a/include/hw/ppc/xive_regs.h
+++ b/include/hw/ppc/xive_regs.h
@@ -48,6 +48,22 @@
 
 #define TM_SHIFT                16
 
+/*
+ * TIMA addresses are 12-bits (4k page).
+ * The MSB indicates a special op with side effect, which can be
+ * refined with bit 10 (see below).
+ * The registers, logically grouped in 4 rings (a quad-word each), are
+ * defined on the 6 LSBs (offset below 0x40)
+ * In between, we can add a cache line index from 0...3 (ie, 0, 0x80,
+ * 0x100, 0x180) to select a specific snooper. Those 'snoop port
+ * address' bits should be dropped when processing the operations as
+ * they are all equivalent.
+ */
+#define TM_ADDRESS_MASK         0xC3F
+#define TM_SPECIAL_OP           0x800
+#define TM_RING_OFFSET          0x30
+#define TM_REG_OFFSET           0x3F
+
 /* TM register offsets */
 #define TM_QW0_USER             0x000 /* All rings */
 #define TM_QW1_OS               0x010 /* Ring 0..2 */
-- 
2.40.1

[PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Daniel Henrique Barboza]

From: Frederic Barrat <fbarrat@linux.ibm.com>

The Thread Interrupt Management Area (TIMA) can be accessed through 4
ports, targeted by the address. The base address of a TIMA
is using port 0 and the other ports are 0x80 apart. Using one port or
another can be useful to balance the load on the snoop buses. With
skiboot and linux, we currently use port 0, but as it tends to be
busy, another hypervisor is using port 1 for TIMA access.

The port address bits fall in between the special op indication
bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
"don't care" for the hardware when processing a TIMA operation. This
patch filters out those port address bits so that a TIMA operation can
be triggered using any port.

It is also true for indirect access (through the IC BAR) and it's
actually nothing new, it was already the case on P9. Which helps here,
as the TIMA handling code is common between P9 (xive) and P10 (xive2).

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/intc/pnv_xive2.c | 4 ++++
 hw/intc/xive.c      | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index 132f82a035..e5a028c1e6 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, hwaddr offset,
     bool gen1_tima_os =
         xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
 
+    offset &= TM_ADDRESS_MASK;
+
     /* TODO: should we switch the TM ops table instead ? */
     if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
         xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
@@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, hwaddr offset, unsigned size)
     bool gen1_tima_os =
         xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
 
+    offset &= TM_ADDRESS_MASK;
+
     /* TODO: should we switch the TM ops table instead ? */
     if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
         return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index ebe399bc09..5204c14b87 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -500,7 +500,7 @@ static const XiveTmOp xive_tm_operations[] = {
 static const XiveTmOp *xive_tm_find_op(hwaddr offset, unsigned size, bool write)
 {
     uint8_t page_offset = (offset >> TM_SHIFT) & 0x3;
-    uint32_t op_offset = offset & 0xFFF;
+    uint32_t op_offset = offset & TM_ADDRESS_MASK;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(xive_tm_operations); i++) {
-- 
2.40.1

[PULL 06/29] target/ppc: Fix nested-hv HEAI delivery

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

ppc hypervisors turn HEAI interrupts into program interrupts injected
into the guest that executed the illegal instruction, if the hypervisor
doesn't handle it some other way.

The nested-hv implementation failed to account for this HEAI->program
conversion. The virtual hypervisor wants to see the HEAI when running
a nested guest, so that interrupt type can be returned to its KVM
caller.

Fixes: 7cebc5db2eba6 ("target/ppc: Introduce a vhyp framework for nested HV support")
Cc: balaton@eik.bme.hu
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-Id: <20230530132127.385001-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/excp_helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index fea9221501..9ffcfe788a 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1358,9 +1358,12 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 
     /*
      * We don't want to generate a Hypervisor Emulation Assistance
-     * Interrupt if we don't have HVB in msr_mask (PAPR mode).
+     * Interrupt if we don't have HVB in msr_mask (PAPR mode),
+     * unless running a nested-hv guest, in which case the L1
+     * kernel wants the interrupt.
      */
-    if (excp == POWERPC_EXCP_HV_EMU && !(env->msr_mask & MSR_HVB)) {
+    if (excp == POWERPC_EXCP_HV_EMU && !(env->msr_mask & MSR_HVB) &&
+            !books_vhyp_handles_hv_excp(cpu)) {
         excp = POWERPC_EXCP_PROGRAM;
     }
 
-- 
2.40.1

[PULL 07/29] pnv/xive2: Quiet down some error messages

[Daniel Henrique Barboza]

From: Frederic Barrat <fbarrat@linux.ibm.com>

When dumping the END and NVP tables ("info pic" from the HMP) on the
P10 model, we're likely to be flooded with error messages such as:

  XIVE[0] - VST: invalid NVPT entry f33800 !?

The error is printed when finding an empty VSD in an indirect
table (thus END and NVP tables with skiboot), which is going to happen
when dumping the xive state. So let's tune down those messages. They
can be re-enabled easily with a macro if needed.

Those errors were already hidden on xive/P9, for the same reason.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230531150537.369350-1-fbarrat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/intc/pnv_xive2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index e5a028c1e6..ec1edeb385 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -163,7 +163,9 @@ static uint64_t pnv_xive2_vst_addr_indirect(PnvXive2 *xive, uint32_t type,
     ldq_be_dma(&address_space_memory, vsd_addr, &vsd, MEMTXATTRS_UNSPECIFIED);
 
     if (!(vsd & VSD_ADDRESS_MASK)) {
+#ifdef XIVE2_DEBUG
         xive2_error(xive, "VST: invalid %s entry %x !?", info->name, idx);
+#endif
         return 0;
     }
 
@@ -185,7 +187,9 @@ static uint64_t pnv_xive2_vst_addr_indirect(PnvXive2 *xive, uint32_t type,
                    MEMTXATTRS_UNSPECIFIED);
 
         if (!(vsd & VSD_ADDRESS_MASK)) {
+#ifdef XIVE2_DEBUG
             xive2_error(xive, "VST: invalid %s entry %x !?", info->name, idx);
+#endif
             return 0;
         }
 
-- 
2.40.1

[PULL 08/29] target/ppc: Fix PMU hflags calculation

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

Some of the PMU hflags bits can go out of synch, for example a store to
MMCR0 with PMCjCE=1 fails to update hflags correctly and results in
hflags mismatch:

  qemu: fatal: TCG hflags mismatch (current:0x2408003d rebuilt:0x240a003d)

This can be reproduced by running perf on a recent machine.

Some of the fragility here is the duplication of PMU hflags calculations.
This change consolidates that in a single place to update pmu-related
hflags, to be called after a well defined state changes.

The post-load PMU update is pulled out of the MSR update because it does
not depend on the MSR value.

Fixes: 8b3d1c49a9f0 ("target/ppc: Add new PMC HFLAGS")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20230530130447.372617-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu_init.c    |  2 +-
 target/ppc/helper_regs.c | 73 ++++++++++++++++++++++++++++++----------
 target/ppc/helper_regs.h |  1 +
 target/ppc/machine.c     |  8 ++---
 target/ppc/power8-pmu.c  | 38 ++++++++++++---------
 target/ppc/power8-pmu.h  |  4 +--
 6 files changed, 85 insertions(+), 41 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 05bf73296b..398f2d9966 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7083,7 +7083,7 @@ static void ppc_cpu_reset_hold(Object *obj)
         if (env->mmu_model != POWERPC_MMU_REAL) {
             ppc_tlb_invalidate_all(env);
         }
-        pmu_update_summaries(env);
+        pmu_mmcr01_updated(env);
     }
 
     /* clean any pending stop state */
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index fb351c303f..bc7e9d7eda 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -47,6 +47,48 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
     env->tgpr[3] = tmp;
 }
 
+static uint32_t hreg_compute_pmu_hflags_value(CPUPPCState *env)
+{
+    uint32_t hflags = 0;
+
+#if defined(TARGET_PPC64)
+    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC0) {
+        hflags |= 1 << HFLAGS_PMCC0;
+    }
+    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
+        hflags |= 1 << HFLAGS_PMCC1;
+    }
+    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE) {
+        hflags |= 1 << HFLAGS_PMCJCE;
+    }
+
+#ifndef CONFIG_USER_ONLY
+    if (env->pmc_ins_cnt) {
+        hflags |= 1 << HFLAGS_INSN_CNT;
+    }
+    if (env->pmc_ins_cnt & 0x1e) {
+        hflags |= 1 << HFLAGS_PMC_OTHER;
+    }
+#endif
+#endif
+
+    return hflags;
+}
+
+/* Mask of all PMU hflags */
+static uint32_t hreg_compute_pmu_hflags_mask(CPUPPCState *env)
+{
+    uint32_t hflags_mask = 0;
+#if defined(TARGET_PPC64)
+    hflags_mask |= 1 << HFLAGS_PMCC0;
+    hflags_mask |= 1 << HFLAGS_PMCC1;
+    hflags_mask |= 1 << HFLAGS_PMCJCE;
+    hflags_mask |= 1 << HFLAGS_INSN_CNT;
+    hflags_mask |= 1 << HFLAGS_PMC_OTHER;
+#endif
+    return hflags_mask;
+}
+
 static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
 {
     target_ulong msr = env->msr;
@@ -104,30 +146,12 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
     if (env->spr[SPR_LPCR] & LPCR_HR) {
         hflags |= 1 << HFLAGS_HR;
     }
-    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC0) {
-        hflags |= 1 << HFLAGS_PMCC0;
-    }
-    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
-        hflags |= 1 << HFLAGS_PMCC1;
-    }
-    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE) {
-        hflags |= 1 << HFLAGS_PMCJCE;
-    }
 
 #ifndef CONFIG_USER_ONLY
     if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
         hflags |= 1 << HFLAGS_HV;
     }
 
-#if defined(TARGET_PPC64)
-    if (env->pmc_ins_cnt) {
-        hflags |= 1 << HFLAGS_INSN_CNT;
-    }
-    if (env->pmc_ins_cnt & 0x1e) {
-        hflags |= 1 << HFLAGS_PMC_OTHER;
-    }
-#endif
-
     /*
      * This is our encoding for server processors. The architecture
      * specifies that there is no such thing as userspace with
@@ -172,6 +196,8 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
     hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
 #endif
 
+    hflags |= hreg_compute_pmu_hflags_value(env);
+
     return hflags | (msr & msr_mask);
 }
 
@@ -180,6 +206,17 @@ void hreg_compute_hflags(CPUPPCState *env)
     env->hflags = hreg_compute_hflags_value(env);
 }
 
+/*
+ * This can be used as a lighter-weight alternative to hreg_compute_hflags
+ * when PMU MMCR0 or pmc_ins_cnt changes. pmc_ins_cnt is changed by
+ * pmu_update_summaries.
+ */
+void hreg_update_pmu_hflags(CPUPPCState *env)
+{
+    env->hflags &= ~hreg_compute_pmu_hflags_mask(env);
+    env->hflags |= hreg_compute_pmu_hflags_value(env);
+}
+
 #ifdef CONFIG_DEBUG_TCG
 void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
                           target_ulong *cs_base, uint32_t *flags)
diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
index 42f26870b9..8196c1346d 100644
--- a/target/ppc/helper_regs.h
+++ b/target/ppc/helper_regs.h
@@ -22,6 +22,7 @@
 
 void hreg_swap_gpr_tgpr(CPUPPCState *env);
 void hreg_compute_hflags(CPUPPCState *env);
+void hreg_update_pmu_hflags(CPUPPCState *env);
 void cpu_interrupt_exittb(CPUState *cs);
 int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
 
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index be6eb3d968..134b16c625 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -21,10 +21,6 @@ static void post_load_update_msr(CPUPPCState *env)
      */
     env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
     ppc_store_msr(env, msr);
-
-    if (tcg_enabled()) {
-        pmu_update_summaries(env);
-    }
 }
 
 static int get_avr(QEMUFile *f, void *pv, size_t size,
@@ -317,6 +313,10 @@ static int cpu_post_load(void *opaque, int version_id)
 
     post_load_update_msr(env);
 
+    if (tcg_enabled()) {
+        pmu_mmcr01_updated(env);
+    }
+
     return 0;
 }
 
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 64a64865d7..c4c331c6b5 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -31,7 +31,11 @@ static bool pmc_has_overflow_enabled(CPUPPCState *env, int sprn)
     return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
 }
 
-void pmu_update_summaries(CPUPPCState *env)
+/*
+ * Called after MMCR0 or MMCR1 changes to update pmc_ins_cnt and pmc_cyc_cnt.
+ * hflags must subsequently be updated.
+ */
+static void pmu_update_summaries(CPUPPCState *env)
 {
     target_ulong mmcr0 = env->spr[SPR_POWER_MMCR0];
     target_ulong mmcr1 = env->spr[SPR_POWER_MMCR1];
@@ -39,7 +43,7 @@ void pmu_update_summaries(CPUPPCState *env)
     int cyc_cnt = 0;
 
     if (mmcr0 & MMCR0_FC) {
-        goto hflags_calc;
+        goto out;
     }
 
     if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
@@ -73,10 +77,19 @@ void pmu_update_summaries(CPUPPCState *env)
     ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
     cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
 
- hflags_calc:
+ out:
     env->pmc_ins_cnt = ins_cnt;
     env->pmc_cyc_cnt = cyc_cnt;
-    env->hflags = deposit32(env->hflags, HFLAGS_INSN_CNT, 1, ins_cnt != 0);
+}
+
+void pmu_mmcr01_updated(CPUPPCState *env)
+{
+    pmu_update_summaries(env);
+    hreg_update_pmu_hflags(env);
+    /*
+     * Should this update overflow timers (if mmcr0 is updated) so they
+     * get set in cpu_post_load?
+     */
 }
 
 static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
@@ -234,18 +247,11 @@ static void pmu_delete_timers(CPUPPCState *env)
 
 void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
 {
-    bool hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
-    bool hflags_pmcc1 = (value & MMCR0_PMCC1) != 0;
-
     pmu_update_cycles(env);
 
     env->spr[SPR_POWER_MMCR0] = value;
 
-    /* MMCR0 writes can change HFLAGS_PMCC[01] and HFLAGS_INSN_CNT */
-    env->hflags = deposit32(env->hflags, HFLAGS_PMCC0, 1, hflags_pmcc0);
-    env->hflags = deposit32(env->hflags, HFLAGS_PMCC1, 1, hflags_pmcc1);
-
-    pmu_update_summaries(env);
+    pmu_mmcr01_updated(env);
 
     /* Update cycle overflow timers with the current MMCR0 state */
     pmu_update_overflow_timers(env);
@@ -257,8 +263,7 @@ void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
 
     env->spr[SPR_POWER_MMCR1] = value;
 
-    /* MMCR1 writes can change HFLAGS_INSN_CNT */
-    pmu_update_summaries(env);
+    pmu_mmcr01_updated(env);
 }
 
 target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
@@ -287,8 +292,8 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
         env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
         env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
 
-        /* Changing MMCR0_FC requires a new HFLAGS_INSN_CNT calc */
-        pmu_update_summaries(env);
+        /* Changing MMCR0_FC requires summaries and hflags update */
+        pmu_mmcr01_updated(env);
 
         /*
          * Delete all pending timers if we need to freeze
@@ -299,6 +304,7 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
     }
 
     if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAE) {
+        /* These MMCR0 bits do not require summaries or hflags update. */
         env->spr[SPR_POWER_MMCR0] &= ~MMCR0_PMAE;
         env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
     }
diff --git a/target/ppc/power8-pmu.h b/target/ppc/power8-pmu.h
index c0093e2219..775e640053 100644
--- a/target/ppc/power8-pmu.h
+++ b/target/ppc/power8-pmu.h
@@ -18,10 +18,10 @@
 #define PMC_COUNTER_NEGATIVE_VAL 0x80000000UL
 
 void cpu_ppc_pmu_init(CPUPPCState *env);
-void pmu_update_summaries(CPUPPCState *env);
+void pmu_mmcr01_updated(CPUPPCState *env);
 #else
 static inline void cpu_ppc_pmu_init(CPUPPCState *env) { }
-static inline void pmu_update_summaries(CPUPPCState *env) { }
+static inline void pmu_mmcr01_updated(CPUPPCState *env) { }
 #endif
 
 #endif
-- 
2.40.1

[PULL 09/29] target/ppc: PMU do not clear MMCR0[FCECE] on performance monitor alert

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

FCECE does not get cleared according to the ISA v3.1B.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20230530134313.387252-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/power8-pmu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index c4c331c6b5..af065115f2 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -289,7 +289,6 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
     pmu_update_cycles(env);
 
     if (env->spr[SPR_POWER_MMCR0] & MMCR0_FCECE) {
-        env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
         env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
 
         /* Changing MMCR0_FC requires summaries and hflags update */
-- 
2.40.1

[PULL 10/29] target/ppc: Fix msgclrp interrupt type

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

msgclrp matches msgsndp and should clear PPC_INTERRUPT_DOORBELL.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230530130714.373215-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/excp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 9ffcfe788a..de6ad121d2 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -3071,7 +3071,7 @@ void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
         return;
     }
 
-    ppc_set_irq(env_archcpu(env), PPC_INTERRUPT_HDOORBELL, 0);
+    ppc_set_irq(env_archcpu(env), PPC_INTERRUPT_DOORBELL, 0);
 }
 
 /*
-- 
2.40.1

[PULL 11/29] target/ppc: Support directed privileged doorbell interrupt (SDOOR)

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

BookS msgsndp instruction to self or DPDES register can cause SDOOR
interrupts which crash QEMU with exception not implemented.

Linux does not use msgsndp in SMT1, and KVM only uses DPDES to cause
doorbells when emulating a SMT guest (which is not the default), so
this has gone unnoticed.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20230530130526.372701-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/excp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index de6ad121d2..befa9aab7f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1542,6 +1542,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
     case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
     case POWERPC_EXCP_TRACE:     /* Trace exception                          */
+    case POWERPC_EXCP_SDOOR:     /* Doorbell interrupt                       */
         break;
     case POWERPC_EXCP_HISI:      /* Hypervisor instruction storage exception */
         msr |= env->error_code;
@@ -1587,7 +1588,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     case POWERPC_EXCP_PERFM:     /* Embedded performance monitor interrupt   */
     case POWERPC_EXCP_VPUA:      /* Vector assist exception                  */
     case POWERPC_EXCP_MAINT:     /* Maintenance exception                    */
-    case POWERPC_EXCP_SDOOR:     /* Doorbell interrupt                       */
     case POWERPC_EXCP_HV_MAINT:  /* Hypervisor Maintenance exception         */
         cpu_abort(cs, "%s exception not implemented\n",
                   powerpc_excp_name(excp));
-- 
2.40.1

[PULL 12/29] target/ppc: PMU implement PERFM interrupts

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

The PMU raises a performance monitor exception (causing an interrupt
when MSR[EE]=1) when MMCR0[PMAO] is set, and lowers it when clear.

Wire this up and implement the interrupt delivery for books. Linux perf
record can now collect PMI-driven samples.

fire_PMC_interrupt is renamed to perfm_alert, which matches a bit closer
to the new terminology used in the ISA and distinguishes the alert
condition (e.g., counter overflow) from the PERFM (or EBB) interrupts.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20230530134313.387252-2-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/excp_helper.c |  2 +-
 target/ppc/power8-pmu.c  | 21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index befa9aab7f..8b95410c36 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1543,6 +1543,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
     case POWERPC_EXCP_TRACE:     /* Trace exception                          */
     case POWERPC_EXCP_SDOOR:     /* Doorbell interrupt                       */
+    case POWERPC_EXCP_PERFM:     /* Performance monitor interrupt            */
         break;
     case POWERPC_EXCP_HISI:      /* Hypervisor instruction storage exception */
         msr |= env->error_code;
@@ -1585,7 +1586,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
          */
         return;
     case POWERPC_EXCP_THERM:     /* Thermal interrupt                        */
-    case POWERPC_EXCP_PERFM:     /* Embedded performance monitor interrupt   */
     case POWERPC_EXCP_VPUA:      /* Vector assist exception                  */
     case POWERPC_EXCP_MAINT:     /* Maintenance exception                    */
     case POWERPC_EXCP_HV_MAINT:  /* Hypervisor Maintenance exception         */
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index af065115f2..7bb4bf81f7 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -84,8 +84,17 @@ static void pmu_update_summaries(CPUPPCState *env)
 
 void pmu_mmcr01_updated(CPUPPCState *env)
 {
+    PowerPCCPU *cpu = env_archcpu(env);
+
     pmu_update_summaries(env);
     hreg_update_pmu_hflags(env);
+
+    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAO) {
+        ppc_set_irq(cpu, PPC_INTERRUPT_PERFM, 1);
+    } else {
+        ppc_set_irq(cpu, PPC_INTERRUPT_PERFM, 0);
+    }
+
     /*
      * Should this update overflow timers (if mmcr0 is updated) so they
      * get set in cpu_post_load?
@@ -282,7 +291,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
     pmc_update_overflow_timer(env, sprn);
 }
 
-static void fire_PMC_interrupt(PowerPCCPU *cpu)
+static void perfm_alert(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
 
@@ -306,6 +315,7 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
         /* These MMCR0 bits do not require summaries or hflags update. */
         env->spr[SPR_POWER_MMCR0] &= ~MMCR0_PMAE;
         env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
+        ppc_set_irq(cpu, PPC_INTERRUPT_PERFM, 1);
     }
 
     raise_ebb_perfm_exception(env);
@@ -314,20 +324,17 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
 void helper_handle_pmc5_overflow(CPUPPCState *env)
 {
     env->spr[SPR_POWER_PMC5] = PMC_COUNTER_NEGATIVE_VAL;
-    fire_PMC_interrupt(env_archcpu(env));
+    perfm_alert(env_archcpu(env));
 }
 
 /* This helper assumes that the PMC is running. */
 void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
 {
     bool overflow_triggered;
-    PowerPCCPU *cpu;
 
     overflow_triggered = pmu_increment_insns(env, num_insns);
-
     if (overflow_triggered) {
-        cpu = env_archcpu(env);
-        fire_PMC_interrupt(cpu);
+        perfm_alert(env_archcpu(env));
     }
 }
 
@@ -335,7 +342,7 @@ static void cpu_ppc_pmu_timer_cb(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
 
-    fire_PMC_interrupt(cpu);
+    perfm_alert(cpu);
 }
 
 void cpu_ppc_pmu_init(CPUPPCState *env)
-- 
2.40.1

[PULL 13/29] target/ppc: Remove single use function

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

The get_physical_address() function is a trivial wrapper of
get_physical_address_wtlb() that is only used once. Remove it and call
get_physical_address_wtlb() directly instead.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <302697d63d26caebefaeee1e45352145ebd0318a.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/mmu_helper.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index 64e30435f5..c0c71a68ff 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -168,15 +168,6 @@ static void booke206_flush_tlb(CPUPPCState *env, int flags,
     tlb_flush(env_cpu(env));
 }
 
-static int get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
-                                target_ulong eaddr, MMUAccessType access_type,
-                                int type)
-{
-    return get_physical_address_wtlb(env, ctx, eaddr, access_type, type, 0);
-}
-
-
-
 /*****************************************************************************/
 /* BATs management */
 #if !defined(FLUSH_ALL_TLBS)
@@ -643,7 +634,7 @@ target_ulong helper_rac(CPUPPCState *env, target_ulong addr)
      */
     nb_BATs = env->nb_BATs;
     env->nb_BATs = 0;
-    if (get_physical_address(env, &ctx, addr, 0, ACCESS_INT) == 0) {
+    if (get_physical_address_wtlb(env, &ctx, addr, 0, ACCESS_INT, 0) == 0) {
         ret = ctx.raddr;
     }
     env->nb_BATs = nb_BATs;
-- 
2.40.1

[PULL 14/29] target/ppc: Remove "ext" parameter of ppcemb_tlb_check()

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

This is only used by one caller so simplify function by removing this
parameter and move the operation to the single place where it's used.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <b21f11ae20e8a8c2e8b5d943f2bff12b5356005a.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |  3 +--
 target/ppc/mmu_common.c | 21 +++++++++------------
 target/ppc/mmu_helper.c |  2 +-
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 10c4ffa148..557e02e697 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1429,8 +1429,7 @@ int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
                             uint32_t pid);
 int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
                             hwaddr *raddrp,
-                            target_ulong address, uint32_t pid, int ext,
-                            int i);
+                            target_ulong address, uint32_t pid, int i);
 hwaddr booke206_tlb_to_page_size(CPUPPCState *env,
                                         ppcmas_tlb_t *tlb);
 #endif
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index 7235a4befe..21a353c51a 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -491,8 +491,7 @@ static int get_segment_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
 /* Generic TLB check function for embedded PowerPC implementations */
 int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
                             hwaddr *raddrp,
-                            target_ulong address, uint32_t pid, int ext,
-                            int i)
+                            target_ulong address, uint32_t pid, int i)
 {
     target_ulong mask;
 
@@ -514,11 +513,6 @@ int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
         return -1;
     }
     *raddrp = (tlb->RPN & mask) | (address & ~mask);
-    if (ext) {
-        /* Extend the physical address to 36 bits */
-        *raddrp |= (uint64_t)(tlb->RPN & 0xF) << 32;
-    }
-
     return 0;
 }
 
@@ -536,7 +530,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
     for (i = 0; i < env->nb_tlb; i++) {
         tlb = &env->tlb.tlbe[i];
         if (ppcemb_tlb_check(env, tlb, &raddr, address,
-                             env->spr[SPR_40x_PID], 0, i) < 0) {
+                             env->spr[SPR_40x_PID], i) < 0) {
             continue;
         }
         zsel = (tlb->attr >> 4) & 0xF;
@@ -598,20 +592,23 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
     int prot2;
 
     if (ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID],
-                         !env->nb_pids, i) >= 0) {
+                         env->spr[SPR_BOOKE_PID], i) >= 0) {
+        if (!env->nb_pids) {
+            /* Extend the physical address to 36 bits */
+            *raddr |= (uint64_t)(tlb->RPN & 0xF) << 32;
+        }
         goto found_tlb;
     }
 
     if (env->spr[SPR_BOOKE_PID1] &&
         ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID1], 0, i) >= 0) {
+                         env->spr[SPR_BOOKE_PID1], i) >= 0) {
         goto found_tlb;
     }
 
     if (env->spr[SPR_BOOKE_PID2] &&
         ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID2], 0, i) >= 0) {
+                         env->spr[SPR_BOOKE_PID2], i) >= 0) {
         goto found_tlb;
     }
 
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index c0c71a68ff..e7275eaec1 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -124,7 +124,7 @@ static int ppcemb_tlb_search(CPUPPCState *env, target_ulong address,
     ret = -1;
     for (i = 0; i < env->nb_tlb; i++) {
         tlb = &env->tlb.tlbe[i];
-        if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, 0, i) == 0) {
+        if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, i) == 0) {
             ret = i;
             break;
         }
-- 
2.40.1

[PULL 15/29] target/ppc: Move ppcemb_tlb_search() to mmu_common.c

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

This function is the only reason why ppcemb_tlb_check() is not static
to mmu_common.c but it also better fits in mmu_common.c so move it
there.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <b64fd712a773558dea9b84945c57785546c0ae2e.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |  4 +---
 target/ppc/mmu_common.c | 22 +++++++++++++++++++++-
 target/ppc/mmu_helper.c | 21 ---------------------
 3 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 557e02e697..8001582d52 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1427,9 +1427,7 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
 int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
                             hwaddr *raddrp, target_ulong address,
                             uint32_t pid);
-int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
-                            hwaddr *raddrp,
-                            target_ulong address, uint32_t pid, int i);
+int ppcemb_tlb_search(CPUPPCState *env, target_ulong address, uint32_t pid);
 hwaddr booke206_tlb_to_page_size(CPUPPCState *env,
                                         ppcmas_tlb_t *tlb);
 #endif
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index 21a353c51a..845eee4c6f 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -489,7 +489,7 @@ static int get_segment_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
 }
 
 /* Generic TLB check function for embedded PowerPC implementations */
-int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
+static int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
                             hwaddr *raddrp,
                             target_ulong address, uint32_t pid, int i)
 {
@@ -516,6 +516,26 @@ int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
     return 0;
 }
 
+/* Generic TLB search function for PowerPC embedded implementations */
+int ppcemb_tlb_search(CPUPPCState *env, target_ulong address, uint32_t pid)
+{
+    ppcemb_tlb_t *tlb;
+    hwaddr raddr;
+    int i, ret;
+
+    /* Default return value is no match */
+    ret = -1;
+    for (i = 0; i < env->nb_tlb; i++) {
+        tlb = &env->tlb.tlbe[i];
+        if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, i) == 0) {
+            ret = i;
+            break;
+        }
+    }
+
+    return ret;
+}
+
 static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
                                        target_ulong address,
                                        MMUAccessType access_type)
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index e7275eaec1..d3ea7588f9 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -112,27 +112,6 @@ static void ppc6xx_tlb_store(CPUPPCState *env, target_ulong EPN, int way,
     env->last_way = way;
 }
 
-/* Generic TLB search function for PowerPC embedded implementations */
-static int ppcemb_tlb_search(CPUPPCState *env, target_ulong address,
-                             uint32_t pid)
-{
-    ppcemb_tlb_t *tlb;
-    hwaddr raddr;
-    int i, ret;
-
-    /* Default return value is no match */
-    ret = -1;
-    for (i = 0; i < env->nb_tlb; i++) {
-        tlb = &env->tlb.tlbe[i];
-        if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, i) == 0) {
-            ret = i;
-            break;
-        }
-    }
-
-    return ret;
-}
-
 /* Helpers specific to PowerPC 40x implementations */
 static inline void ppc4xx_tlb_invalidate_all(CPUPPCState *env)
 {
-- 
2.40.1

[PULL 16/29] target/ppc: Remove some unneded line breaks

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

Make lines shorter and fix indentation in some functions prototypes.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <70952ba2d82141db1cf5cfcf4b227402be575874.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        | 8 +++-----
 target/ppc/mmu_common.c | 8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 8001582d52..c7c2a5534c 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1424,12 +1424,10 @@ void store_booke_tsr(CPUPPCState *env, target_ulong val);
 void ppc_tlb_invalidate_all(CPUPPCState *env);
 void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr);
 void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
-int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
-                            hwaddr *raddrp, target_ulong address,
-                            uint32_t pid);
+int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb, hwaddr *raddrp,
+                     target_ulong address, uint32_t pid);
 int ppcemb_tlb_search(CPUPPCState *env, target_ulong address, uint32_t pid);
-hwaddr booke206_tlb_to_page_size(CPUPPCState *env,
-                                        ppcmas_tlb_t *tlb);
+hwaddr booke206_tlb_to_page_size(CPUPPCState *env, ppcmas_tlb_t *tlb);
 #endif
 
 void ppc_store_fpscr(CPUPPCState *env, target_ulong val);
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index 845eee4c6f..a84bc7de88 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -694,8 +694,7 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
     return ret;
 }
 
-hwaddr booke206_tlb_to_page_size(CPUPPCState *env,
-                                        ppcmas_tlb_t *tlb)
+hwaddr booke206_tlb_to_page_size(CPUPPCState *env, ppcmas_tlb_t *tlb)
 {
     int tlbm_size;
 
@@ -705,9 +704,8 @@ hwaddr booke206_tlb_to_page_size(CPUPPCState *env,
 }
 
 /* TLB check function for MAS based SoftTLBs */
-int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
-                            hwaddr *raddrp, target_ulong address,
-                            uint32_t pid)
+int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb, hwaddr *raddrp,
+                     target_ulong address, uint32_t pid)
 {
     hwaddr mask;
     uint32_t tlb_pid;
-- 
2.40.1

[PULL 17/29] target/ppc: Simplify ppcemb_tlb_search()

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

No nead to store return value and break from loop when we can return
directly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <d470118c3adcbd41b1a91779f6bb7cbdb2b0d346.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/mmu_common.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index a84bc7de88..ff7f987546 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -521,19 +521,15 @@ int ppcemb_tlb_search(CPUPPCState *env, target_ulong address, uint32_t pid)
 {
     ppcemb_tlb_t *tlb;
     hwaddr raddr;
-    int i, ret;
+    int i;
 
-    /* Default return value is no match */
-    ret = -1;
     for (i = 0; i < env->nb_tlb; i++) {
         tlb = &env->tlb.tlbe[i];
         if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, i) == 0) {
-            ret = i;
-            break;
+            return i;
         }
     }
-
-    return ret;
+    return -1;
 }
 
 static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
-- 
2.40.1

[PULL 18/29] target/ppc: Change ppcemb_tlb_check() to return bool

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <bacd1bcbe99c07930c29a9815915da9ac75f6920.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/mmu_common.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index ff7f987546..bd7d7d5257 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -489,15 +489,15 @@ static int get_segment_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
 }
 
 /* Generic TLB check function for embedded PowerPC implementations */
-static int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
-                            hwaddr *raddrp,
-                            target_ulong address, uint32_t pid, int i)
+static bool ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
+                             hwaddr *raddrp,
+                             target_ulong address, uint32_t pid, int i)
 {
     target_ulong mask;
 
     /* Check valid flag */
     if (!(tlb->prot & PAGE_VALID)) {
-        return -1;
+        return false;
     }
     mask = ~(tlb->size - 1);
     qemu_log_mask(CPU_LOG_MMU, "%s: TLB %d address " TARGET_FMT_lx
@@ -506,14 +506,14 @@ static int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
                   mask, (uint32_t)tlb->PID, tlb->prot);
     /* Check PID */
     if (tlb->PID != 0 && tlb->PID != pid) {
-        return -1;
+        return false;
     }
     /* Check effective address */
     if ((address & mask) != tlb->EPN) {
-        return -1;
+        return false;
     }
     *raddrp = (tlb->RPN & mask) | (address & ~mask);
-    return 0;
+    return true;
 }
 
 /* Generic TLB search function for PowerPC embedded implementations */
@@ -525,7 +525,7 @@ int ppcemb_tlb_search(CPUPPCState *env, target_ulong address, uint32_t pid)
 
     for (i = 0; i < env->nb_tlb; i++) {
         tlb = &env->tlb.tlbe[i];
-        if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, i) == 0) {
+        if (ppcemb_tlb_check(env, tlb, &raddr, address, pid, i)) {
             return i;
         }
     }
@@ -545,8 +545,8 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
     pr = FIELD_EX64(env->msr, MSR, PR);
     for (i = 0; i < env->nb_tlb; i++) {
         tlb = &env->tlb.tlbe[i];
-        if (ppcemb_tlb_check(env, tlb, &raddr, address,
-                             env->spr[SPR_40x_PID], i) < 0) {
+        if (!ppcemb_tlb_check(env, tlb, &raddr, address,
+                              env->spr[SPR_40x_PID], i)) {
             continue;
         }
         zsel = (tlb->attr >> 4) & 0xF;
@@ -608,7 +608,7 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
     int prot2;
 
     if (ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID], i) >= 0) {
+                         env->spr[SPR_BOOKE_PID], i)) {
         if (!env->nb_pids) {
             /* Extend the physical address to 36 bits */
             *raddr |= (uint64_t)(tlb->RPN & 0xF) << 32;
@@ -618,13 +618,13 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
 
     if (env->spr[SPR_BOOKE_PID1] &&
         ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID1], i) >= 0) {
+                         env->spr[SPR_BOOKE_PID1], i)) {
         goto found_tlb;
     }
 
     if (env->spr[SPR_BOOKE_PID2] &&
         ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID2], i) >= 0) {
+                         env->spr[SPR_BOOKE_PID2], i)) {
         goto found_tlb;
     }
 
-- 
2.40.1

[PULL 19/29] target/ppc: Eliminate goto in mmubooke_check_tlb()

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

Move out checking PID registers into a separate function which makes
mmubooke_check_tlb() simpler and avoids using goto.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <bd84d5f38af0ba2983ccd5c07635db49267c828f.1685448535.git.balaton@eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/mmu_common.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index bd7d7d5257..ae1db6e348 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -601,37 +601,39 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
     return ret;
 }
 
-static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
-                              hwaddr *raddr, int *prot, target_ulong address,
-                              MMUAccessType access_type, int i)
+static bool mmubooke_check_pid(CPUPPCState *env, ppcemb_tlb_t *tlb,
+                               hwaddr *raddr, target_ulong addr, int i)
 {
-    int prot2;
-
-    if (ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID], i)) {
+    if (ppcemb_tlb_check(env, tlb, raddr, addr, env->spr[SPR_BOOKE_PID], i)) {
         if (!env->nb_pids) {
             /* Extend the physical address to 36 bits */
             *raddr |= (uint64_t)(tlb->RPN & 0xF) << 32;
         }
-        goto found_tlb;
+        return true;
+    } else if (!env->nb_pids) {
+        return false;
     }
-
     if (env->spr[SPR_BOOKE_PID1] &&
-        ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID1], i)) {
-        goto found_tlb;
+        ppcemb_tlb_check(env, tlb, raddr, addr, env->spr[SPR_BOOKE_PID1], i)) {
+        return true;
     }
-
     if (env->spr[SPR_BOOKE_PID2] &&
-        ppcemb_tlb_check(env, tlb, raddr, address,
-                         env->spr[SPR_BOOKE_PID2], i)) {
-        goto found_tlb;
+        ppcemb_tlb_check(env, tlb, raddr, addr, env->spr[SPR_BOOKE_PID2], i)) {
+        return true;
     }
+    return false;
+}
 
-     qemu_log_mask(CPU_LOG_MMU, "%s: TLB entry not found\n", __func__);
-    return -1;
+static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
+                              hwaddr *raddr, int *prot, target_ulong address,
+                              MMUAccessType access_type, int i)
+{
+    int prot2;
 
-found_tlb:
+    if (!mmubooke_check_pid(env, tlb, raddr, address, i)) {
+        qemu_log_mask(CPU_LOG_MMU, "%s: TLB entry not found\n", __func__);
+        return -1;
+    }
 
     if (FIELD_EX64(env->msr, MSR, PR)) {
         prot2 = tlb->prot & 0xF;
-- 
2.40.1

[PULL 20/29] target/ppc: Fix lqarx to set cpu_reserve

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

lqarx does not set cpu_reserve, which causes stqcx. to never succeed.

Cc: qemu-stable@nongnu.org
Fixes: 94bf2658676 ("target/ppc: Use atomic load for LQ and LQARX")
Fixes: 57b38ffd0c6 ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20230605025445.161932-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 37fd431870..452439b729 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3765,6 +3765,7 @@ static void gen_lqarx(DisasContext *ctx)
     tcg_gen_qemu_ld_i128(t16, EA, ctx->mem_idx, DEF_MEMOP(MO_128 | MO_ALIGN));
     tcg_gen_extr_i128_i64(lo, hi, t16);
 
+    tcg_gen_mov_tl(cpu_reserve, EA);
     tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
     tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
 }
-- 
2.40.1

[PULL 21/29] target/ppc: Ensure stcx size matches larx

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

Differently-sized larx/stcx. pairs can succeed if the starting address
matches. Add a check to require the size of stcx. exactly match the larx
that established the reservation. Use the term "reserve_length" for this
state, which matches the terminology used in the ISA.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-Id: <20230605025445.161932-2-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h       | 5 +++--
 target/ppc/cpu_init.c  | 4 ++--
 target/ppc/translate.c | 9 +++++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index c7c2a5534c..20508bac5e 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1114,8 +1114,9 @@ struct CPUArchState {
     target_ulong ov32;
     target_ulong ca32;
 
-    target_ulong reserve_addr; /* Reservation address */
-    target_ulong reserve_val;  /* Reservation value */
+    target_ulong reserve_addr;   /* Reservation address */
+    target_ulong reserve_length; /* Reservation larx op size (bytes) */
+    target_ulong reserve_val;    /* Reservation value */
     target_ulong reserve_val2;
 
     /* These are used in supervisor mode only */
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 398f2d9966..d4ef074afb 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7392,8 +7392,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
         }
         qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
     }
-    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
-                 env->reserve_addr);
+    qemu_fprintf(f, " ]     RES %03x@" TARGET_FMT_lx "\n",
+                 (int)env->reserve_length, env->reserve_addr);
 
     if (flags & CPU_DUMP_FPU) {
         for (i = 0; i < 32; i++) {
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 452439b729..cf0bd79b8c 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -75,6 +75,7 @@ static TCGv cpu_cfar;
 #endif
 static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
 static TCGv cpu_reserve;
+static TCGv cpu_reserve_length;
 static TCGv cpu_reserve_val;
 static TCGv cpu_reserve_val2;
 static TCGv cpu_fpscr;
@@ -143,6 +144,10 @@ void ppc_translate_init(void)
     cpu_reserve = tcg_global_mem_new(cpu_env,
                                      offsetof(CPUPPCState, reserve_addr),
                                      "reserve_addr");
+    cpu_reserve_length = tcg_global_mem_new(cpu_env,
+                                            offsetof(CPUPPCState,
+                                                     reserve_length),
+                                            "reserve_length");
     cpu_reserve_val = tcg_global_mem_new(cpu_env,
                                          offsetof(CPUPPCState, reserve_val),
                                          "reserve_val");
@@ -3469,6 +3474,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
     gen_addr_reg_index(ctx, t0);
     tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
     tcg_gen_mov_tl(cpu_reserve, t0);
+    tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
     tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
 }
@@ -3700,6 +3706,7 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
     gen_set_access_type(ctx, ACCESS_RES);
     gen_addr_reg_index(ctx, t0);
     tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
 
     t0 = tcg_temp_new();
     tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
@@ -3766,6 +3773,7 @@ static void gen_lqarx(DisasContext *ctx)
     tcg_gen_extr_i128_i64(lo, hi, t16);
 
     tcg_gen_mov_tl(cpu_reserve, EA);
+    tcg_gen_movi_tl(cpu_reserve_length, 16);
     tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
     tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
 }
@@ -3791,6 +3799,7 @@ static void gen_stqcx_(DisasContext *ctx)
     gen_addr_reg_index(ctx, EA);
 
     tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
 
     cmp = tcg_temp_new_i128();
     val = tcg_temp_new_i128();
-- 
2.40.1

[PULL 22/29] target/ppc: Remove larx/stcx. memory barrier semantics

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

larx and stcx. are not defined to order any memory operations.
Remove the barriers.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-Id: <20230605025445.161932-3-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/translate.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index cf0bd79b8c..cb4764476d 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3476,7 +3476,6 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
     tcg_gen_mov_tl(cpu_reserve, t0);
     tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
 }
 
 #define LARX(name, memop)                  \
@@ -3720,11 +3719,6 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
 
     gen_set_label(l1);
 
-    /*
-     * Address mismatch implies failure.  But we still need to provide
-     * the memory barrier semantics of the instruction.
-     */
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
 
     gen_set_label(l2);
@@ -3828,11 +3822,6 @@ static void gen_stqcx_(DisasContext *ctx)
     tcg_gen_br(lab_over);
     gen_set_label(lab_fail);
 
-    /*
-     * Address mismatch implies failure.  But we still need to provide
-     * the memory barrier semantics of the instruction.
-     */
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
 
     gen_set_label(lab_over);
-- 
2.40.1

[PULL 23/29] target/ppc: Rework store conditional to avoid branch

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

Rework store conditional to avoid a branch in the success case.
Change some of the variable names and layout while here so
gen_conditional_store more closely matches gen_stqcx_.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-Id: <20230605025445.161932-4-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/translate.c | 63 ++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index cb4764476d..b591f2e496 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3697,31 +3697,32 @@ static void gen_stdat(DisasContext *ctx)
 
 static void gen_conditional_store(DisasContext *ctx, MemOp memop)
 {
-    TCGLabel *l1 = gen_new_label();
-    TCGLabel *l2 = gen_new_label();
-    TCGv t0 = tcg_temp_new();
-    int reg = rS(ctx->opcode);
+    TCGLabel *lfail;
+    TCGv EA;
+    TCGv cr0;
+    TCGv t0;
+    int rs = rS(ctx->opcode);
 
+    lfail = gen_new_label();
+    EA = tcg_temp_new();
+    cr0 = tcg_temp_new();
+    t0 = tcg_temp_new();
+
+    tcg_gen_mov_tl(cr0, cpu_so);
     gen_set_access_type(ctx, ACCESS_RES);
-    gen_addr_reg_index(ctx, t0);
-    tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
-    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
+    gen_addr_reg_index(ctx, EA);
+    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), lfail);
 
-    t0 = tcg_temp_new();
     tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
-                              cpu_gpr[reg], ctx->mem_idx,
+                              cpu_gpr[rs], ctx->mem_idx,
                               DEF_MEMOP(memop) | MO_ALIGN);
     tcg_gen_setcond_tl(TCG_COND_EQ, t0, t0, cpu_reserve_val);
     tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
-    tcg_gen_or_tl(t0, t0, cpu_so);
-    tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
-    tcg_gen_br(l2);
+    tcg_gen_or_tl(cr0, cr0, t0);
 
-    gen_set_label(l1);
-
-    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
-
-    gen_set_label(l2);
+    gen_set_label(lfail);
+    tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
     tcg_gen_movi_tl(cpu_reserve, -1);
 }
 
@@ -3775,25 +3776,26 @@ static void gen_lqarx(DisasContext *ctx)
 /* stqcx. */
 static void gen_stqcx_(DisasContext *ctx)
 {
-    TCGLabel *lab_fail, *lab_over;
-    int rs = rS(ctx->opcode);
+    TCGLabel *lfail;
     TCGv EA, t0, t1;
+    TCGv cr0;
     TCGv_i128 cmp, val;
+    int rs = rS(ctx->opcode);
 
     if (unlikely(rs & 1)) {
         gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         return;
     }
 
-    lab_fail = gen_new_label();
-    lab_over = gen_new_label();
+    lfail = gen_new_label();
+    EA = tcg_temp_new();
+    cr0 = tcg_temp_new();
 
+    tcg_gen_mov_tl(cr0, cpu_so);
     gen_set_access_type(ctx, ACCESS_RES);
-    EA = tcg_temp_new();
     gen_addr_reg_index(ctx, EA);
-
-    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
-    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
+    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lfail);
 
     cmp = tcg_temp_new_i128();
     val = tcg_temp_new_i128();
@@ -3816,15 +3818,10 @@ static void gen_stqcx_(DisasContext *ctx)
 
     tcg_gen_setcondi_tl(TCG_COND_EQ, t0, t0, 0);
     tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
-    tcg_gen_or_tl(t0, t0, cpu_so);
-    tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
-
-    tcg_gen_br(lab_over);
-    gen_set_label(lab_fail);
-
-    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
+    tcg_gen_or_tl(cr0, cr0, t0);
 
-    gen_set_label(lab_over);
+    gen_set_label(lfail);
+    tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
     tcg_gen_movi_tl(cpu_reserve, -1);
 }
 #endif /* defined(TARGET_PPC64) */
-- 
2.40.1

[PULL 24/29] target/ppc: Fix decrementer time underflow and infinite timer loop

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

It is possible to store a very large value to the decrementer that it
does not raise the decrementer exception so the timer is scheduled, but
the next time value wraps and is treated as in the past.

This can occur if (u64)-1 is stored on a zero-triggered exception, or
(u64)-1 is stored twice on an underflow-triggered exception, for
example.

If such a value is set in DECAR, it gets stored to the decrementer by
the timer function, which then immediately causes another timer, which
hangs QEMU.

Clamp the decrementer to the implemented width, and use that as the
value for the timer calculation, effectively preventing this overflow.

Reported-by: sdicaro@DDCI.com
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20230530131214.373524-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/ppc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 4e816c68c7..d80b0adc6c 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -798,6 +798,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
     int64_t signed_decr;
 
     /* Truncate value to decr_width and sign extend for simplicity */
+    value = extract64(value, 0, nr_bits);
+    decr = extract64(decr, 0, nr_bits);
     signed_value = sextract64(value, 0, nr_bits);
     signed_decr = sextract64(decr, 0, nr_bits);
 
-- 
2.40.1

[PULL 25/29] target/ppc: Decrementer fix BookE semantics

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

The decrementer store function has logic that short-cuts the timer if a
very small value is stored (0, 1, or 2) and raises an interrupt
directly. There are two problem with this on BookE.

First is that BookE says a decrementer interrupt should not be raised
on a store of 0, only of a decrement from 1. Second is that raising
the irq directly will bypass the auto-reload logic in the booke decr
timer function, breaking autoreload when 1 or 2 is stored.

Fix this by removing that small-value special case. It makes this
tricky logic even more difficult to reason about, and it hardly matters
for performance.

Cc: sdicaro@DDCI.com
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20230530131214.373524-2-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/ppc.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index d80b0adc6c..1b1220c423 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -811,11 +811,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
     }
 
     /*
-     * Going from 2 -> 1, 1 -> 0 or 0 -> -1 is the event to generate a DEC
-     * interrupt.
-     *
-     * If we get a really small DEC value, we can assume that by the time we
-     * handled it we should inject an interrupt already.
+     * Going from 1 -> 0 or 0 -> -1 is the event to generate a DEC interrupt.
      *
      * On MSB level based DEC implementations the MSB always means the interrupt
      * is pending, so raise it on those.
@@ -823,8 +819,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
      * On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
      * an edge interrupt, so raise it here too.
      */
-    if ((value < 3) ||
-        ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
+    if (((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
         ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
           && signed_decr >= 0)) {
         (*raise_excp)(cpu);
-- 
2.40.1

[PULL 26/29] hw/ppc/openpic: Do not open-code ROUND_UP() macro

[Daniel Henrique Barboza]

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

While reviewing, the ROUND_UP() macro is easier to figure out.
Besides, the comment confirms we want to round up here.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Message-Id: <20230523061546.49031-1-philmd@linaro.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 include/hw/ppc/openpic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
index ebdaf8a493..bae8dafe16 100644
--- a/include/hw/ppc/openpic.h
+++ b/include/hw/ppc/openpic.h
@@ -55,7 +55,7 @@ typedef enum IRQType {
  * Round up to the nearest 64 IRQs so that the queue length
  * won't change when moving between 32 and 64 bit hosts.
  */
-#define IRQQUEUE_SIZE_BITS ((OPENPIC_MAX_IRQ + 63) & ~63)
+#define IRQQUEUE_SIZE_BITS ROUND_UP(OPENPIC_MAX_IRQ, 64)
 
 typedef struct IRQQueue {
     unsigned long *queue;
-- 
2.40.1

[PULL 27/29] tests/avocado/tuxrun_baselines: Fix ppc64 tests for binaries without slirp

[Daniel Henrique Barboza]

From: Thomas Huth <thuth@redhat.com>

The ppc64 tuxrun tests are currently failing if "slirp" has been
disabled in the binary since they are using "-netdev user" now.
We have to skip the test if this network backend is missing.

Fixes: 6ee3624236 ("improve code coverage for ppc64")
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Acked-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20230606192802.666000-1-thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/avocado/tuxrun_baselines.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/avocado/tuxrun_baselines.py b/tests/avocado/tuxrun_baselines.py
index 3a46e7a745..e12250eabb 100644
--- a/tests/avocado/tuxrun_baselines.py
+++ b/tests/avocado/tuxrun_baselines.py
@@ -184,6 +184,7 @@ def common_tuxrun(self,
 
     def ppc64_common_tuxrun(self, sums, prefix):
         # add device args to command line.
+        self.require_netdev('user')
         self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
                          '-device', 'virtio-net,netdev=vnet')
         self.vm.add_args('-netdev', '{"type":"user","id":"hostnet0"}',
-- 
2.40.1

[PULL 28/29] target/ppc: Implement gathering irq statistics

[Daniel Henrique Barboza]

From: BALATON Zoltan <balaton@eik.bme.hu>

Count exceptions which can be queried with info irq monitor command.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230606220200.7EBCC74635C@zero.eik.bme.hu>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h         |  1 +
 target/ppc/cpu_init.c    | 18 ++++++++++++++++++
 target/ppc/excp_helper.c |  1 +
 3 files changed, 20 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 20508bac5e..0ee2adc105 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1195,6 +1195,7 @@ struct CPUArchState {
     int error_code;
     uint32_t pending_interrupts;
 #if !defined(CONFIG_USER_ONLY)
+    uint64_t excp_stats[POWERPC_EXCP_NB];
     /*
      * This is the IRQ controller, which is implementation dependent and only
      * relevant when emulating a complete machine. Note that this isn't used
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index d4ef074afb..9f97222655 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -48,6 +48,7 @@
 
 #ifndef CONFIG_USER_ONLY
 #include "hw/boards.h"
+#include "hw/intc/intc.h"
 #endif
 
 /* #define PPC_DEBUG_SPR */
@@ -7123,6 +7124,16 @@ static bool ppc_cpu_is_big_endian(CPUState *cs)
     return !FIELD_EX64(env->msr, MSR, LE);
 }
 
+static bool ppc_get_irq_stats(InterruptStatsProvider *obj,
+                              uint64_t **irq_counts, unsigned int *nb_irqs)
+{
+    CPUPPCState *env = &POWERPC_CPU(obj)->env;
+
+    *irq_counts = env->excp_stats;
+    *nb_irqs = ARRAY_SIZE(env->excp_stats);
+    return true;
+}
+
 #ifdef CONFIG_TCG
 static void ppc_cpu_exec_enter(CPUState *cs)
 {
@@ -7286,6 +7297,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = ppc_cpu_gdb_write_register;
 #ifndef CONFIG_USER_ONLY
     cc->sysemu_ops = &ppc_sysemu_ops;
+    INTERRUPT_STATS_PROVIDER_CLASS(oc)->get_statistics = ppc_get_irq_stats;
 #endif
 
     cc->gdb_num_core_regs = 71;
@@ -7323,6 +7335,12 @@ static const TypeInfo ppc_cpu_type_info = {
     .abstract = true,
     .class_size = sizeof(PowerPCCPUClass),
     .class_init = ppc_cpu_class_init,
+#ifndef CONFIG_USER_ONLY
+    .interfaces = (InterfaceInfo[]) {
+          { TYPE_INTERRUPT_STATS_PROVIDER },
+          { }
+    },
+#endif
 };
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 8b95410c36..12d8a7257b 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1655,6 +1655,7 @@ static void powerpc_excp(PowerPCCPU *cpu, int excp)
     qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
                   " => %s (%d) error=%02x\n", env->nip, powerpc_excp_name(excp),
                   excp, env->error_code);
+    env->excp_stats[excp]++;
 
     switch (env->excp_model) {
     case POWERPC_EXCP_40x:
-- 
2.40.1

[PULL 29/29] hw/ppc/Kconfig: MAC_NEWWORLD should always select USB_OHCI_PCI

[Daniel Henrique Barboza]

From: Thomas Huth <thuth@redhat.com>

The PowerMacs have an OHCI controller soldered on the motherboard,
so this should always be enabled for the "mac99" machine.
This fixes the problem that QEMU aborts when the user tries to run
the "mac99" machine with a build that has been compiled with the
"--without-default-devices" configure switch.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Message-Id: <20230530102041.55527-1-thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index a689d9b219..5dfbf47ef5 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -115,6 +115,7 @@ config MAC_NEWWORLD
     select MAC_PMU
     select UNIN_PCI
     select FW_CFG_PPC
+    select USB_OHCI_PCI
 
 config E500
     bool
-- 
2.40.1

Re: [PULL 00/29] ppc queue

[Richard Henderson]

On 6/10/23 06:31, Daniel Henrique Barboza wrote:
> The following changes since commit 3673ad389622d9ef4d2743101253c642def7935a:
> 
>    tcg/tci: Fix MemOpIdx operand index for 3-operand memops (2023-06-09 08:30:56 -0700)
> 
> are available in the Git repository at:
> 
>    https://gitlab.com/danielhb/qemu.git  tags/pull-ppc-20230610
> 
> for you to fetch changes up to 9ec08f3569be3bc8bfd4d9b8b0445b9136910661:
> 
>    hw/ppc/Kconfig: MAC_NEWWORLD should always select USB_OHCI_PCI (2023-06-10 10:19:24 -0300)
> 
> ----------------------------------------------------------------
> ppc patch queue for 2023-06-10:
> 
> This queue includes several assorted fixes for target/ppc emulation and
> XIVE2. It also includes an openpic fix, an avocado fix for ppc64
> binaries without slipr and a Kconfig change for MAC_NEWWORLD.

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate.


r~

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Peter Maydell]

On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> The Thread Interrupt Management Area (TIMA) can be accessed through 4
> ports, targeted by the address. The base address of a TIMA
> is using port 0 and the other ports are 0x80 apart. Using one port or
> another can be useful to balance the load on the snoop buses. With
> skiboot and linux, we currently use port 0, but as it tends to be
> busy, another hypervisor is using port 1 for TIMA access.
>
> The port address bits fall in between the special op indication
> bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
> "don't care" for the hardware when processing a TIMA operation. This
> patch filters out those port address bits so that a TIMA operation can
> be triggered using any port.
>
> It is also true for indirect access (through the IC BAR) and it's
> actually nothing new, it was already the case on P9. Which helps here,
> as the TIMA handling code is common between P9 (xive) and P10 (xive2).
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Hi -- Coverity points out that there's a problem with this
change (CID 1512997, 1512998):

> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, hwaddr offset,
>      bool gen1_tima_os =
>          xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>
> +    offset &= TM_ADDRESS_MASK;

Here we now mask off most of the bytes of 'offset',
because TM_ADDRESS_MASK is 0xC3F...

> +
>      /* TODO: should we switch the TM ops table instead ? */
>      if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {

...but here we compare offset against HV_PUSH_OS_CTX_OFFSET,
which is defined as
#define HV_PUSH_OS_CTX_OFFSET  (HV_PAGE_OFFSET | (TM_QW1_OS + TM_WORD2))
and since
#define HV_PAGE_OFFSET         (XIVE_TM_HV_PAGE << TM_SHIFT)
#define XIVE_TM_HV_PAGE         0x1
#define TM_SHIFT                16

that means HV_PUSH_OS_CTX_OFFSET has bits defined in the
upper 16 bits, and the comparison can now never be true,
making the if() dead code.

>          xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
> @@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, hwaddr offset, unsigned size)
>      bool gen1_tima_os =
>          xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>
> +    offset &= TM_ADDRESS_MASK;
> +
>      /* TODO: should we switch the TM ops table instead ? */
>      if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
>          return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);

Similarly here.

thanks
-- PMM

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Cédric Le Goater]

On 6/20/23 12:45, Peter Maydell wrote:
> On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>> From: Frederic Barrat <fbarrat@linux.ibm.com>
>>
>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>> ports, targeted by the address. The base address of a TIMA
>> is using port 0 and the other ports are 0x80 apart. Using one port or
>> another can be useful to balance the load on the snoop buses. With
>> skiboot and linux, we currently use port 0, but as it tends to be
>> busy, another hypervisor is using port 1 for TIMA access.
>>
>> The port address bits fall in between the special op indication
>> bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
>> "don't care" for the hardware when processing a TIMA operation. This
>> patch filters out those port address bits so that a TIMA operation can
>> be triggered using any port.
>>
>> It is also true for indirect access (through the IC BAR) and it's
>> actually nothing new, it was already the case on P9. Which helps here,
>> as the TIMA handling code is common between P9 (xive) and P10 (xive2).
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
> 
> Hi -- Coverity points out that there's a problem with this
> change (CID 1512997, 1512998):
> 
>> --- a/hw/intc/pnv_xive2.c
>> +++ b/hw/intc/pnv_xive2.c
>> @@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, hwaddr offset,
>>       bool gen1_tima_os =
>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>
>> +    offset &= TM_ADDRESS_MASK;
> 
> Here we now mask off most of the bytes of 'offset',
> because TM_ADDRESS_MASK is 0xC3F...
> 
>> +
>>       /* TODO: should we switch the TM ops table instead ? */
>>       if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
> 
> ...but here we compare offset against HV_PUSH_OS_CTX_OFFSET,
> which is defined as
> #define HV_PUSH_OS_CTX_OFFSET  (HV_PAGE_OFFSET | (TM_QW1_OS + TM_WORD2))
> and since
> #define HV_PAGE_OFFSET         (XIVE_TM_HV_PAGE << TM_SHIFT)
> #define XIVE_TM_HV_PAGE         0x1
> #define TM_SHIFT                16
> 
> that means HV_PUSH_OS_CTX_OFFSET has bits defined in the
> upper 16 bits, and the comparison can now never be true,
> making the if() dead code.
> 
>>           xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
>> @@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, hwaddr offset, unsigned size)
>>       bool gen1_tima_os =
>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>
>> +    offset &= TM_ADDRESS_MASK;
>> +
>>       /* TODO: should we switch the TM ops table instead ? */
>>       if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
>>           return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);
> 
> Similarly here.


yes. I think this went unnoticed because the push/pull os context
commands are only used by the HV when a vCPU is dipatched on a HW
thread. We would need a test for a KVM guest running under the QEMU
PowerNV POWER10 machine. This requires an image with some tuning
because emulation is a bit slow. I use to have a buildroot image
including a qemu and smaller buildroot image for it.

So, offset is within the full TIMA region (4 pages) and
TM_ADDRESS_MASK is a mask within a page. This needs a fix.

Thanks,

C.

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Frederic Barrat]

On 20/06/2023 13:20, Cédric Le Goater wrote:
> On 6/20/23 12:45, Peter Maydell wrote:
>> On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
>> <danielhb413@gmail.com> wrote:
>>>
>>> From: Frederic Barrat <fbarrat@linux.ibm.com>
>>>
>>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>>> ports, targeted by the address. The base address of a TIMA
>>> is using port 0 and the other ports are 0x80 apart. Using one port or
>>> another can be useful to balance the load on the snoop buses. With
>>> skiboot and linux, we currently use port 0, but as it tends to be
>>> busy, another hypervisor is using port 1 for TIMA access.
>>>
>>> The port address bits fall in between the special op indication
>>> bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
>>> "don't care" for the hardware when processing a TIMA operation. This
>>> patch filters out those port address bits so that a TIMA operation can
>>> be triggered using any port.
>>>
>>> It is also true for indirect access (through the IC BAR) and it's
>>> actually nothing new, it was already the case on P9. Which helps here,
>>> as the TIMA handling code is common between P9 (xive) and P10 (xive2).
>>>
>>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>> Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>
>> Hi -- Coverity points out that there's a problem with this
>> change (CID 1512997, 1512998):
>>
>>> --- a/hw/intc/pnv_xive2.c
>>> +++ b/hw/intc/pnv_xive2.c
>>> @@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, 
>>> hwaddr offset,
>>>       bool gen1_tima_os =
>>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>>
>>> +    offset &= TM_ADDRESS_MASK;
>>
>> Here we now mask off most of the bytes of 'offset',
>> because TM_ADDRESS_MASK is 0xC3F...
>>
>>> +
>>>       /* TODO: should we switch the TM ops table instead ? */
>>>       if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
>>
>> ...but here we compare offset against HV_PUSH_OS_CTX_OFFSET,
>> which is defined as
>> #define HV_PUSH_OS_CTX_OFFSET  (HV_PAGE_OFFSET | (TM_QW1_OS + TM_WORD2))
>> and since
>> #define HV_PAGE_OFFSET         (XIVE_TM_HV_PAGE << TM_SHIFT)
>> #define XIVE_TM_HV_PAGE         0x1
>> #define TM_SHIFT                16
>>
>> that means HV_PUSH_OS_CTX_OFFSET has bits defined in the
>> upper 16 bits, and the comparison can now never be true,
>> making the if() dead code.
>>
>>>           xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
>>> @@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, 
>>> hwaddr offset, unsigned size)
>>>       bool gen1_tima_os =
>>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>>
>>> +    offset &= TM_ADDRESS_MASK;
>>> +
>>>       /* TODO: should we switch the TM ops table instead ? */
>>>       if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
>>>           return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);
>>
>> Similarly here.
> 
> 
> yes. I think this went unnoticed because the push/pull os context
> commands are only used by the HV when a vCPU is dipatched on a HW
> thread. We would need a test for a KVM guest running under the QEMU
> PowerNV POWER10 machine. This requires an image with some tuning
> because emulation is a bit slow. I use to have a buildroot image
> including a qemu and smaller buildroot image for it.


Working on a fix. It's true that I hadn't run a guest within the powernv 
machine for quite a while. I'm dusting off my buildroot repo to test it 
this time...

   Fred


> 
> So, offset is within the full TIMA region (4 pages) and
> TM_ADDRESS_MASK is a mask within a page. This needs a fix.
> 
> Thanks,
> 
> C.
> 

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Cédric Le Goater]

On 6/20/23 16:31, Frederic Barrat wrote:
> 
> 
> On 20/06/2023 13:20, Cédric Le Goater wrote:
>> On 6/20/23 12:45, Peter Maydell wrote:
>>> On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>>
>>>> From: Frederic Barrat <fbarrat@linux.ibm.com>
>>>>
>>>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>>>> ports, targeted by the address. The base address of a TIMA
>>>> is using port 0 and the other ports are 0x80 apart. Using one port or
>>>> another can be useful to balance the load on the snoop buses. With
>>>> skiboot and linux, we currently use port 0, but as it tends to be
>>>> busy, another hypervisor is using port 1 for TIMA access.
>>>>
>>>> The port address bits fall in between the special op indication
>>>> bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
>>>> "don't care" for the hardware when processing a TIMA operation. This
>>>> patch filters out those port address bits so that a TIMA operation can
>>>> be triggered using any port.
>>>>
>>>> It is also true for indirect access (through the IC BAR) and it's
>>>> actually nothing new, it was already the case on P9. Which helps here,
>>>> as the TIMA handling code is common between P9 (xive) and P10 (xive2).
>>>>
>>>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>> Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>
>>> Hi -- Coverity points out that there's a problem with this
>>> change (CID 1512997, 1512998):
>>>
>>>> --- a/hw/intc/pnv_xive2.c
>>>> +++ b/hw/intc/pnv_xive2.c
>>>> @@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, hwaddr offset,
>>>>       bool gen1_tima_os =
>>>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>>>
>>>> +    offset &= TM_ADDRESS_MASK;
>>>
>>> Here we now mask off most of the bytes of 'offset',
>>> because TM_ADDRESS_MASK is 0xC3F...
>>>
>>>> +
>>>>       /* TODO: should we switch the TM ops table instead ? */
>>>>       if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
>>>
>>> ...but here we compare offset against HV_PUSH_OS_CTX_OFFSET,
>>> which is defined as
>>> #define HV_PUSH_OS_CTX_OFFSET  (HV_PAGE_OFFSET | (TM_QW1_OS + TM_WORD2))
>>> and since
>>> #define HV_PAGE_OFFSET         (XIVE_TM_HV_PAGE << TM_SHIFT)
>>> #define XIVE_TM_HV_PAGE         0x1
>>> #define TM_SHIFT                16
>>>
>>> that means HV_PUSH_OS_CTX_OFFSET has bits defined in the
>>> upper 16 bits, and the comparison can now never be true,
>>> making the if() dead code.
>>>
>>>>           xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
>>>> @@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, hwaddr offset, unsigned size)
>>>>       bool gen1_tima_os =
>>>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>>>
>>>> +    offset &= TM_ADDRESS_MASK;
>>>> +
>>>>       /* TODO: should we switch the TM ops table instead ? */
>>>>       if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
>>>>           return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);
>>>
>>> Similarly here.
>>
>>
>> yes. I think this went unnoticed because the push/pull os context
>> commands are only used by the HV when a vCPU is dipatched on a HW
>> thread. We would need a test for a KVM guest running under the QEMU
>> PowerNV POWER10 machine. This requires an image with some tuning
>> because emulation is a bit slow. I use to have a buildroot image
>> including a qemu and smaller buildroot image for it.
> 
> 
> Working on a fix. It's true that I hadn't run a guest within the powernv 
> machine for quite a while. I'm dusting off my buildroot repo to test it this time...

I wonder if we could host the image in some place, since there is a need
for "nested"  guest testing. QEMU PPC supports at least 2 implementations :

  * PowerNV / KVM-on-pseries v1
  * KVM-on-pseries v1 / pseries

and yet to come :

  * KVM-on-pseries v2 (pHyp) / pseries

C.


> 
>    Fred
> 
> 
>>
>> So, offset is within the full TIMA region (4 pages) and
>> TM_ADDRESS_MASK is a mask within a page. This needs a fix.
>>
>> Thanks,
>>
>> C.
>>

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Cédric Le Goater]

On 6/20/23 16:31, Frederic Barrat wrote:
> 
> 
> On 20/06/2023 13:20, Cédric Le Goater wrote:
>> On 6/20/23 12:45, Peter Maydell wrote:
>>> On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>>
>>>> From: Frederic Barrat <fbarrat@linux.ibm.com>
>>>>
>>>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>>>> ports, targeted by the address. The base address of a TIMA
>>>> is using port 0 and the other ports are 0x80 apart. Using one port or
>>>> another can be useful to balance the load on the snoop buses. With
>>>> skiboot and linux, we currently use port 0, but as it tends to be
>>>> busy, another hypervisor is using port 1 for TIMA access.
>>>>
>>>> The port address bits fall in between the special op indication
>>>> bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
>>>> "don't care" for the hardware when processing a TIMA operation. This
>>>> patch filters out those port address bits so that a TIMA operation can
>>>> be triggered using any port.
>>>>
>>>> It is also true for indirect access (through the IC BAR) and it's
>>>> actually nothing new, it was already the case on P9. Which helps here,
>>>> as the TIMA handling code is common between P9 (xive) and P10 (xive2).
>>>>
>>>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>> Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>
>>> Hi -- Coverity points out that there's a problem with this
>>> change (CID 1512997, 1512998):
>>>
>>>> --- a/hw/intc/pnv_xive2.c
>>>> +++ b/hw/intc/pnv_xive2.c
>>>> @@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, hwaddr offset,
>>>>       bool gen1_tima_os =
>>>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>>>
>>>> +    offset &= TM_ADDRESS_MASK;
>>>
>>> Here we now mask off most of the bytes of 'offset',
>>> because TM_ADDRESS_MASK is 0xC3F...
>>>
>>>> +
>>>>       /* TODO: should we switch the TM ops table instead ? */
>>>>       if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
>>>
>>> ...but here we compare offset against HV_PUSH_OS_CTX_OFFSET,
>>> which is defined as
>>> #define HV_PUSH_OS_CTX_OFFSET  (HV_PAGE_OFFSET | (TM_QW1_OS + TM_WORD2))
>>> and since
>>> #define HV_PAGE_OFFSET         (XIVE_TM_HV_PAGE << TM_SHIFT)
>>> #define XIVE_TM_HV_PAGE         0x1
>>> #define TM_SHIFT                16
>>>
>>> that means HV_PUSH_OS_CTX_OFFSET has bits defined in the
>>> upper 16 bits, and the comparison can now never be true,
>>> making the if() dead code.
>>>
>>>>           xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
>>>> @@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, hwaddr offset, unsigned size)
>>>>       bool gen1_tima_os =
>>>>           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
>>>>
>>>> +    offset &= TM_ADDRESS_MASK;
>>>> +
>>>>       /* TODO: should we switch the TM ops table instead ? */
>>>>       if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
>>>>           return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);
>>>
>>> Similarly here.
>>
>>
>> yes. I think this went unnoticed because the push/pull os context
>> commands are only used by the HV when a vCPU is dipatched on a HW
>> thread. We would need a test for a KVM guest running under the QEMU
>> PowerNV POWER10 machine. This requires an image with some tuning
>> because emulation is a bit slow. I use to have a buildroot image
>> including a qemu and smaller buildroot image for it.
> 
> 
> Working on a fix. It's true that I hadn't run a guest within the powernv machine for quite a while. I'm dusting off my buildroot repo to test it this time...


The XIVE2 TM ops are implemented with a shortcut (See the TODO in
pnv_xive2_tm_*()). We could

1. extend xive_tctx_tm_write/read with a 'bool gen1_tima_os' parameter:

	xive_tctx_tm_write(xptr, tctx, offset, value, size, gen1_tima_os);

    and use the bool in xive_tm_find_op() to select a XiveTmOp array.
    The new xive2_tm_operations[] would be defined as xive_tm_operations[]
    but with an override of HV_PUSH_OS_CTX_OFFSET and HV_PULL_OS_CTX_OFFSET.

2. or extend the XivePresenterClass with a get_config() handler like it
    was done for Xive2RouterClass().

3. or introduce an array of XiveTmOp under XivePresenterClass defined by
    the controller variant.

In any case, we need a new xive2_tm_operations[] for the XIVE2 TM register
layout. Option 1 is simpler I think.

The weird part would be to include "xive2.h" in "xive.c" to get the
definitions of xive2_tm_push/pull_os_ctx. I guess that's ok.

This would  also "fix" the indirect ops in XIVE2, not that we care much
but it will be cleaner.

Thanks,


C.

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Frederic Barrat]

On 21/06/2023 09:18, Cédric Le Goater wrote:
> The XIVE2 TM ops are implemented with a shortcut (See the TODO in
> pnv_xive2_tm_*()). We could
> 
> 1. extend xive_tctx_tm_write/read with a 'bool gen1_tima_os' parameter:
> 
>      xive_tctx_tm_write(xptr, tctx, offset, value, size, gen1_tima_os);
> 
>     and use the bool in xive_tm_find_op() to select a XiveTmOp array.
>     The new xive2_tm_operations[] would be defined as xive_tm_operations[]
>     but with an override of HV_PUSH_OS_CTX_OFFSET and 
> HV_PULL_OS_CTX_OFFSET.
> 
> 2. or extend the XivePresenterClass with a get_config() handler like it
>     was done for Xive2RouterClass().
> 
> 3. or introduce an array of XiveTmOp under XivePresenterClass defined by
>     the controller variant.
> 
> In any case, we need a new xive2_tm_operations[] for the XIVE2 TM register
> layout. Option 1 is simpler I think.


I was also leaning on introducing a xive2_tm_operations[] array of 
operations to fix it correctly.

While I agree it's the simplest, I'm not fond of (1), since we'd need to 
carry that extra parameter to xive_tm_find_op(). Admittedly it's just 
one extra level, but I went with something which is hopefully what you 
had in mind for (2). I like that we can easily extend it in the future.


> This would  also "fix" the indirect ops in XIVE2, not that we care much
> but it will be cleaner.

I'm not sure I see what you mean here. It cleans up nicely 
pnv_xive2_tm_read/write(), but is that really what you had in mind?


Something related I notice is that when doing an indirect access to the 
TIMA through the IC BAR, we call the TIMA access functions with a NULL 
reference to the presenter:

static uint64_t pnv_xive2_ic_tm_indirect_read(void *opaque, hwaddr offset,
                                               unsigned size)
{
     PnvXive2 *xive = PNV_XIVE2(opaque);
...
     tctx = pnv_xive2_get_indirect_tctx(xive, pir);
     if (tctx) {
         val = xive_tctx_tm_read(NULL, tctx, offset, size);
     }

We seem to mostly ignore that first parameter in 
xive_tctx_tm_read/write(). IIUC, the special ops will be checked with a 
page offset matching ring 0 and won't match anything. Still, it seems a 
bit dangerous and I was wondering:
1. can't we just create it from the PnvXive2 we have at hand?
2. in any case, isn't it always redundant with tctxt->presenter?

   Fred

Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Cédric Le Goater]

On 6/21/23 17:18, Frederic Barrat wrote:
> 
> 
> On 21/06/2023 09:18, Cédric Le Goater wrote:
>> The XIVE2 TM ops are implemented with a shortcut (See the TODO in
>> pnv_xive2_tm_*()). We could
>>
>> 1. extend xive_tctx_tm_write/read with a 'bool gen1_tima_os' parameter:
>>
>>      xive_tctx_tm_write(xptr, tctx, offset, value, size, gen1_tima_os);
>>
>>     and use the bool in xive_tm_find_op() to select a XiveTmOp array.
>>     The new xive2_tm_operations[] would be defined as xive_tm_operations[]
>>     but with an override of HV_PUSH_OS_CTX_OFFSET and HV_PULL_OS_CTX_OFFSET.
>>
>> 2. or extend the XivePresenterClass with a get_config() handler like it
>>     was done for Xive2RouterClass().
>>
>> 3. or introduce an array of XiveTmOp under XivePresenterClass defined by
>>     the controller variant.
>>
>> In any case, we need a new xive2_tm_operations[] for the XIVE2 TM register
>> layout. Option 1 is simpler I think.
> 
> 
> I was also leaning on introducing a xive2_tm_operations[] array of operations to fix it correctly.
> 
> While I agree it's the simplest, I'm not fond of (1), since we'd need to carry that extra parameter to xive_tm_find_op(). Admittedly it's just one extra level, but I went with something which is hopefully what you had in mind for (2). 

It is.

> I like that we can easily extend it in the future.

Yes. There are new  bits in the Gen2 TM layout that might need an
implementation for other workloads than OPAL/Linux. Having a second
array will help.
  

>> This would  also "fix" the indirect ops in XIVE2, not that we care much
>> but it will be cleaner.
> 
> I'm not sure I see what you mean here. It cleans up nicely pnv_xive2_tm_read/write(), but is that really what you had in mind?

yes.

I meant that indirect ops in XIVE2 didn't bother testing Gen1/Gen2.

> 
> 
> Something related I notice is that when doing an indirect access to the TIMA through the IC BAR, we call the TIMA access functions with a NULL reference to the presenter:

Yes. I don't remember why. May be because it was not important at
the time.

> 
> static uint64_t pnv_xive2_ic_tm_indirect_read(void *opaque, hwaddr offset,
>                                                unsigned size)
> {
>      PnvXive2 *xive = PNV_XIVE2(opaque);
> ...
>      tctx = pnv_xive2_get_indirect_tctx(xive, pir);
>      if (tctx) {
>          val = xive_tctx_tm_read(NULL, tctx, offset, size);
>      }
> 
> We seem to mostly ignore that first parameter in xive_tctx_tm_read/write(). IIUC, the special ops will be checked with a page offset matching ring 0 and won't match anything. Still, it seems a bit dangerous and I was wondering:
> 1. can't we just create it from the PnvXive2 we have at hand?

we could.

> 2. in any case, isn't it always redundant with tctxt->presenter?

it is. it should be the same. May add an assert in pnv_xive2_get_indirect_tctx()
if they are different.


Thanks,

C.


> 
>    Fred