Re: [PATCH 1/4] ppc/xive: Use address_space routines to access the machine RAM

[Cédric Le Goater]

On 8/31/23 09:44, Frederic Barrat wrote:

On 29/08/2023 16:32, Cédric Le Goater wrote:

to log an error in case of bad configuration of the XIVE tables by the FW.

Signed-off-by: Cédric Le Goater <c...@kaod.org>
---
  hw/intc/pnv_xive.c  | 27 +++++++++++++++++++++++----
  hw/intc/pnv_xive2.c | 27 +++++++++++++++++++++++----
  2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index e536b3ec26e5..b2bafd61b157 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -242,12 +242,20 @@ static int pnv_xive_vst_read(PnvXive *xive, uint32_t 
type, uint8_t blk,
  {
      const XiveVstInfo *info = &vst_infos[type];
      uint64_t addr = pnv_xive_vst_addr(xive, type, blk, idx);
+    MemTxResult result;
      if (!addr) {
          return -1;
      }
-    cpu_physical_memory_read(addr, data, info->size);
+    result = address_space_read(&address_space_memory, addr,
+                                MEMTXATTRS_UNSPECIFIED, data,
+                                info->size);

I had been wondering which is the "right" API to update the guest memory. Since the cpu_physical_memory_* family ends up calling its address_space_* equivalent, I'm guessing the point of the change is really to catch any error

yes.

and remove any potential ambiguity about the address space?

yes. See LPC and XSCOM for other address spaces in the PowerNV domain. Also,
the XIVE EDT but that's more a modeling trick.

In this case, the IC is reading RAM using a physical address, so checking
that the transaction status is important. The FW could configure bogus
addresses.

In any case,
Reviewed-by: Frederic Barrat <fbar...@linux.ibm.com>

Thanks,

C.

   Fred

+    if (result != MEMTX_OK) {
+        xive_error(xive, "VST: read failed at @0x%" HWADDR_PRIx
+                   " for VST %s %x/%x\n", addr, info->name, blk, idx);
+        return -1;
+    }
      return 0;
  }
@@ -258,16 +266,27 @@ static int pnv_xive_vst_write(PnvXive *xive, uint32_t 
type, uint8_t blk,
  {
      const XiveVstInfo *info = &vst_infos[type];
      uint64_t addr = pnv_xive_vst_addr(xive, type, blk, idx);
+    MemTxResult result;
      if (!addr) {
          return -1;
      }
      if (word_number == XIVE_VST_WORD_ALL) {
-        cpu_physical_memory_write(addr, data, info->size);
+        result = address_space_write(&address_space_memory, addr,
+                                     MEMTXATTRS_UNSPECIFIED, data,
+                                     info->size);
      } else {
-        cpu_physical_memory_write(addr + word_number * 4,
-                                  data + word_number * 4, 4);
+        result = address_space_write(&address_space_memory,
+                                     addr + word_number * 4,
+                                     MEMTXATTRS_UNSPECIFIED,
+                                     data + word_number * 4, 4);
+    }
+
+    if (result != MEMTX_OK) {
+        xive_error(xive, "VST: write failed at @0x%" HWADDR_PRIx
+                    "for VST %s %x/%x\n", addr, info->name, blk, idx);
+        return -1;
      }
      return 0;
  }
diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index bbb44a533cff..4b8d0a5d8120 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -240,12 +240,20 @@ static int pnv_xive2_vst_read(PnvXive2 *xive, uint32_t 
type, uint8_t blk,
  {
      const XiveVstInfo *info = &vst_infos[type];
      uint64_t addr = pnv_xive2_vst_addr(xive, type, blk, idx);
+    MemTxResult result;
      if (!addr) {
          return -1;
      }
-    cpu_physical_memory_read(addr, data, info->size);
+    result = address_space_read(&address_space_memory, addr,
+                                MEMTXATTRS_UNSPECIFIED, data,
+                                info->size);
+    if (result != MEMTX_OK) {
+        xive2_error(xive, "VST: read failed at @0x%" HWADDR_PRIx
+                   " for VST %s %x/%x\n", addr, info->name, blk, idx);
+        return -1;
+    }
      return 0;
  }
@@ -256,16 +264,27 @@ static int pnv_xive2_vst_write(PnvXive2 *xive, uint32_t 
type, uint8_t blk,
  {
      const XiveVstInfo *info = &vst_infos[type];
      uint64_t addr = pnv_xive2_vst_addr(xive, type, blk, idx);
+    MemTxResult result;
      if (!addr) {
          return -1;
      }
      if (word_number == XIVE_VST_WORD_ALL) {
-        cpu_physical_memory_write(addr, data, info->size);
+        result = address_space_write(&address_space_memory, addr,
+                                     MEMTXATTRS_UNSPECIFIED, data,
+                                     info->size);
      } else {
-        cpu_physical_memory_write(addr + word_number * 4,
-                                  data + word_number * 4, 4);
+        result = address_space_write(&address_space_memory,
+                                     addr + word_number * 4,
+                                     MEMTXATTRS_UNSPECIFIED,
+                                     data + word_number * 4, 4);
+    }
+
+    if (result != MEMTX_OK) {
+        xive2_error(xive, "VST: write failed at @0x%" HWADDR_PRIx
+                   "for VST %s %x/%x\n", addr, info->name, blk, idx);
+        return -1;
      }
      return 0;
  }