[PATCH 0/4] ppc/xive: Rework Inter chip communication

[PATCH 0/4] ppc/xive: Rework Inter chip communication

[Cédric Le Goater]

Hello,

Today, the inter chip communication for interrupts uses the
pnv_xive_get_remote() routine to grab the remote XIVE interrupt
controller object. This is a modeling shortcut which can be improved
by implementing :

 * remote END triggers
 * memory operations on remote NVT structures.

Both are addressed by this series for P9. P10 should be similar.

Thanks,

C. 

Cédric Le Goater (4):
  ppc/xive: Use address_space routines to access the machine RAM
  ppc/xive: Introduce a new XiveRouter end_notify() handler
  ppc/xive: Handle END triggers between chips with MMIOs
  ppc/xive: Add support for the PC MMIOs

 hw/intc/pnv_xive_regs.h |   1 +
 include/hw/ppc/xive.h   |   2 +
 hw/intc/pnv_xive.c      | 170 +++++++++++++++++++++++++++++++---------
 hw/intc/pnv_xive2.c     |  27 ++++++-
 hw/intc/xive.c          |  28 ++++---
 5 files changed, 177 insertions(+), 51 deletions(-)

-- 
2.41.0

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

[Cédric Le Goater]

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

Signed-off-by: Cédric Le Goater <clg@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);
+    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;
 }
-- 
2.41.0

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

[Frederic Barrat]

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 <clg@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 and remove any potential ambiguity about the 
address space?

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

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

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 <clg@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 <fbarrat@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;
>>   }

[PATCH 2/4] ppc/xive: Introduce a new XiveRouter end_notify() handler

[Cédric Le Goater]

It will help us model the END triggers on the PowerNV machine, which
can be rerouted to another interrupt controller.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xive.h |  2 ++
 hw/intc/xive.c        | 28 ++++++++++++++++++----------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 9f580a2699e9..f120874e0ff1 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -401,6 +401,7 @@ struct XiveRouterClass {
     int (*write_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
                      XiveNVT *nvt, uint8_t word_number);
     uint8_t (*get_block_id)(XiveRouter *xrtr);
+    void (*end_notify)(XiveRouter *xrtr, XiveEAS *eas);
 };
 
 int xive_router_get_eas(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
@@ -414,6 +415,7 @@ int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
 int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
                           XiveNVT *nvt, uint8_t word_number);
 void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked);
+void xive_router_end_notify(XiveRouter *xrtr, XiveEAS *eas);
 
 /*
  * XIVE Presenter
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 56670b2cac6e..df3ee0496fe7 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1518,6 +1518,13 @@ static void xive_router_realize(DeviceState *dev, Error **errp)
     assert(xrtr->xfb);
 }
 
+static void xive_router_end_notify_handler(XiveRouter *xrtr, XiveEAS *eas)
+{
+    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
+
+    return xrc->end_notify(xrtr, eas);
+}
+
 /*
  * Encode the HW CAM line in the block group mode format :
  *
@@ -1664,8 +1671,7 @@ static bool xive_router_end_es_notify(XiveRouter *xrtr, uint8_t end_blk,
  * another chip. We don't model the PowerBus but the END trigger
  * message has the same parameters than in the function below.
  */
-static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
-                                   uint32_t end_idx, uint32_t end_data)
+void xive_router_end_notify(XiveRouter *xrtr, XiveEAS *eas)
 {
     XiveEND end;
     uint8_t priority;
@@ -1675,6 +1681,10 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
     XiveNVT nvt;
     bool found;
 
+    uint8_t end_blk = xive_get_field64(EAS_END_BLOCK, eas->w);
+    uint32_t end_idx = xive_get_field64(EAS_END_INDEX, eas->w);
+    uint32_t end_data = xive_get_field64(EAS_END_DATA,  eas->w);
+
     /* END cache lookup */
     if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
         qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
@@ -1817,10 +1827,7 @@ do_escalation:
     /*
      * The END trigger becomes an Escalation trigger
      */
-    xive_router_end_notify(xrtr,
-                           xive_get_field32(END_W4_ESC_END_BLOCK, end.w4),
-                           xive_get_field32(END_W4_ESC_END_INDEX, end.w4),
-                           xive_get_field32(END_W5_ESC_END_DATA,  end.w5));
+    xive_router_end_notify_handler(xrtr, (XiveEAS *) &end.w4);
 }
 
 void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
@@ -1871,10 +1878,7 @@ void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
     /*
      * The event trigger becomes an END trigger
      */
-    xive_router_end_notify(xrtr,
-                           xive_get_field64(EAS_END_BLOCK, eas.w),
-                           xive_get_field64(EAS_END_INDEX, eas.w),
-                           xive_get_field64(EAS_END_DATA,  eas.w));
+    xive_router_end_notify_handler(xrtr, &eas);
 }
 
 static Property xive_router_properties[] = {
@@ -1887,12 +1891,16 @@ static void xive_router_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     XiveNotifierClass *xnc = XIVE_NOTIFIER_CLASS(klass);
+    XiveRouterClass *xrc = XIVE_ROUTER_CLASS(klass);
 
     dc->desc    = "XIVE Router Engine";
     device_class_set_props(dc, xive_router_properties);
     /* Parent is SysBusDeviceClass. No need to call its realize hook */
     dc->realize = xive_router_realize;
     xnc->notify = xive_router_notify;
+
+    /* By default, the router handles END triggers locally */
+    xrc->end_notify = xive_router_end_notify;
 }
 
 static const TypeInfo xive_router_info = {
-- 
2.41.0

Re: [PATCH 2/4] ppc/xive: Introduce a new XiveRouter end_notify() handler

[Frederic Barrat]

On 29/08/2023 16:32, Cédric Le Goater wrote:
> It will help us model the END triggers on the PowerNV machine, which
> can be rerouted to another interrupt controller.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---



> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 56670b2cac6e..df3ee0496fe7 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1518,6 +1518,13 @@ static void xive_router_realize(DeviceState *dev, Error **errp)
>       assert(xrtr->xfb);
>   }
>   
> +static void xive_router_end_notify_handler(XiveRouter *xrtr, XiveEAS *eas)
> +{
> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
> +
> +    return xrc->end_notify(xrtr, eas);
> +}
> +
>   /*
>    * Encode the HW CAM line in the block group mode format :
>    *
> @@ -1664,8 +1671,7 @@ static bool xive_router_end_es_notify(XiveRouter *xrtr, uint8_t end_blk,
>    * another chip. We don't model the PowerBus but the END trigger
>    * message has the same parameters than in the function below.
>    */
> -static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
> -                                   uint32_t end_idx, uint32_t end_data)
> +void xive_router_end_notify(XiveRouter *xrtr, XiveEAS *eas)
>   {
>       XiveEND end;
>       uint8_t priority;
> @@ -1675,6 +1681,10 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>       XiveNVT nvt;
>       bool found;
>   
> +    uint8_t end_blk = xive_get_field64(EAS_END_BLOCK, eas->w);
> +    uint32_t end_idx = xive_get_field64(EAS_END_INDEX, eas->w);
> +    uint32_t end_data = xive_get_field64(EAS_END_DATA,  eas->w);
> +
>       /* END cache lookup */
>       if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
>           qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
> @@ -1817,10 +1827,7 @@ do_escalation:
>       /*
>        * The END trigger becomes an Escalation trigger
>        */
> -    xive_router_end_notify(xrtr,
> -                           xive_get_field32(END_W4_ESC_END_BLOCK, end.w4),
> -                           xive_get_field32(END_W4_ESC_END_INDEX, end.w4),
> -                           xive_get_field32(END_W5_ESC_END_DATA,  end.w5));
> +    xive_router_end_notify_handler(xrtr, (XiveEAS *) &end.w4);


I didn't like the cast, but I can see why you're doing it this way. We 
should be fine as long as the notify handler is not testing the validity 
bit of the EAS structure.

Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   }
>   
>   void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
> @@ -1871,10 +1878,7 @@ void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
>       /*
>        * The event trigger becomes an END trigger
>        */
> -    xive_router_end_notify(xrtr,
> -                           xive_get_field64(EAS_END_BLOCK, eas.w),
> -                           xive_get_field64(EAS_END_INDEX, eas.w),
> -                           xive_get_field64(EAS_END_DATA,  eas.w));
> +    xive_router_end_notify_handler(xrtr, &eas);
>   }
>   
>   static Property xive_router_properties[] = {
> @@ -1887,12 +1891,16 @@ static void xive_router_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       XiveNotifierClass *xnc = XIVE_NOTIFIER_CLASS(klass);
> +    XiveRouterClass *xrc = XIVE_ROUTER_CLASS(klass);
>   
>       dc->desc    = "XIVE Router Engine";
>       device_class_set_props(dc, xive_router_properties);
>       /* Parent is SysBusDeviceClass. No need to call its realize hook */
>       dc->realize = xive_router_realize;
>       xnc->notify = xive_router_notify;
> +
> +    /* By default, the router handles END triggers locally */
> +    xrc->end_notify = xive_router_end_notify;
>   }
>   
>   static const TypeInfo xive_router_info = {

Re: [PATCH 2/4] ppc/xive: Introduce a new XiveRouter end_notify() handler

[Cédric Le Goater]

On 8/31/23 09:50, Frederic Barrat wrote:
> 
> 
> On 29/08/2023 16:32, Cédric Le Goater wrote:
>> It will help us model the END triggers on the PowerNV machine, which
>> can be rerouted to another interrupt controller.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> 
> 
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 56670b2cac6e..df3ee0496fe7 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -1518,6 +1518,13 @@ static void xive_router_realize(DeviceState *dev, Error **errp)
>>       assert(xrtr->xfb);
>>   }
>> +static void xive_router_end_notify_handler(XiveRouter *xrtr, XiveEAS *eas)
>> +{
>> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
>> +
>> +    return xrc->end_notify(xrtr, eas);
>> +}
>> +
>>   /*
>>    * Encode the HW CAM line in the block group mode format :
>>    *
>> @@ -1664,8 +1671,7 @@ static bool xive_router_end_es_notify(XiveRouter *xrtr, uint8_t end_blk,
>>    * another chip. We don't model the PowerBus but the END trigger
>>    * message has the same parameters than in the function below.
>>    */
>> -static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>> -                                   uint32_t end_idx, uint32_t end_data)
>> +void xive_router_end_notify(XiveRouter *xrtr, XiveEAS *eas)
>>   {
>>       XiveEND end;
>>       uint8_t priority;
>> @@ -1675,6 +1681,10 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>>       XiveNVT nvt;
>>       bool found;
>> +    uint8_t end_blk = xive_get_field64(EAS_END_BLOCK, eas->w);
>> +    uint32_t end_idx = xive_get_field64(EAS_END_INDEX, eas->w);
>> +    uint32_t end_data = xive_get_field64(EAS_END_DATA,  eas->w);
>> +
>>       /* END cache lookup */
>>       if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
>>           qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
>> @@ -1817,10 +1827,7 @@ do_escalation:
>>       /*
>>        * The END trigger becomes an Escalation trigger
>>        */
>> -    xive_router_end_notify(xrtr,
>> -                           xive_get_field32(END_W4_ESC_END_BLOCK, end.w4),
>> -                           xive_get_field32(END_W4_ESC_END_INDEX, end.w4),
>> -                           xive_get_field32(END_W5_ESC_END_DATA,  end.w5));
>> +    xive_router_end_notify_handler(xrtr, (XiveEAS *) &end.w4);
> 
> 
> I didn't like the cast, but I can see why you're doing it this way. 

OPAL does a similar cast :

  https://github.com/open-power/skiboot/blob/master/hw/xive.c#L783

> We should be fine as long as the notify handler is not testing the validity bit of the EAS structure.

Indeed. It doesn't seem to be set :

XIVE[0] #0 END Escalation EAT
   0000008e -Q    end:00/008f data:00000000
   0000008f --    end:01/003f data:0000008f
   00000096 -Q    end:00/0097 data:00000000
   00000097 --    end:01/003f data:00000097

I don't remember to be honest. This is from a few years back. Ask the
HW designer may be ?
  
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

Thanks,

C.

> 
>    Fred
> 
> 
>>   }
>>   void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
>> @@ -1871,10 +1878,7 @@ void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
>>       /*
>>        * The event trigger becomes an END trigger
>>        */
>> -    xive_router_end_notify(xrtr,
>> -                           xive_get_field64(EAS_END_BLOCK, eas.w),
>> -                           xive_get_field64(EAS_END_INDEX, eas.w),
>> -                           xive_get_field64(EAS_END_DATA,  eas.w));
>> +    xive_router_end_notify_handler(xrtr, &eas);
>>   }
>>   static Property xive_router_properties[] = {
>> @@ -1887,12 +1891,16 @@ static void xive_router_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       XiveNotifierClass *xnc = XIVE_NOTIFIER_CLASS(klass);
>> +    XiveRouterClass *xrc = XIVE_ROUTER_CLASS(klass);
>>       dc->desc    = "XIVE Router Engine";
>>       device_class_set_props(dc, xive_router_properties);
>>       /* Parent is SysBusDeviceClass. No need to call its realize hook */
>>       dc->realize = xive_router_realize;
>>       xnc->notify = xive_router_notify;
>> +
>> +    /* By default, the router handles END triggers locally */
>> +    xrc->end_notify = xive_router_end_notify;
>>   }
>>   static const TypeInfo xive_router_info = {

[PATCH 3/4] ppc/xive: Handle END triggers between chips with MMIOs

[Cédric Le Goater]

The notify page of the interrupt controller can either be used to
receive trigger events from the HW controllers (PHB, PSI) or to
reroute interrupts between Interrupt Controllers. In which case, the
VSD table is used to determine the address of the notify page of the
remote IC and the store data is forwarded.

Today, our model grabs the remote VSD (EAS, END, NVT) address using
pnv_xive_get_remote() helper. Be more precise and implement remote END
triggers using a store on the remote IC notify page.

We still have a shortcut in the model for the NVT accesses which we
will address later.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/pnv_xive_regs.h |  1 +
 hw/intc/pnv_xive.c      | 69 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/hw/intc/pnv_xive_regs.h b/hw/intc/pnv_xive_regs.h
index c78f030c0260..793847638bce 100644
--- a/hw/intc/pnv_xive_regs.h
+++ b/hw/intc/pnv_xive_regs.h
@@ -228,6 +228,7 @@
  *       VSD and is only meant to be used in indirect mode !
  */
 #define VSD_MODE                PPC_BITMASK(0, 1)
+#define  VSD_MODE_INVALID       0
 #define  VSD_MODE_SHARED        1
 #define  VSD_MODE_EXCLUSIVE     2
 #define  VSD_MODE_FORWARD       3
diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index b2bafd61b157..aae5cb6f607b 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -225,6 +225,11 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
 
     /* Remote VST access */
     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
+        if (type != VST_TSEL_VPDT) {
+            xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
+                       info->name, blk, idx);
+            return 0;
+        }
         xive = pnv_xive_get_remote(blk);
 
         return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
@@ -294,12 +299,26 @@ static int pnv_xive_vst_write(PnvXive *xive, uint32_t type, uint8_t blk,
 static int pnv_xive_get_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
                             XiveEND *end)
 {
+    PnvXive *xive = PNV_XIVE(xrtr);
+
+    if (pnv_xive_block_id(xive) != blk) {
+        xive_error(xive, "VST: END %x/%x is remote !?", blk, idx);
+        return -1;
+    }
+
     return pnv_xive_vst_read(PNV_XIVE(xrtr), VST_TSEL_EQDT, blk, idx, end);
 }
 
 static int pnv_xive_write_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
                               XiveEND *end, uint8_t word_number)
 {
+    PnvXive *xive = PNV_XIVE(xrtr);
+
+    if (pnv_xive_block_id(xive) != blk) {
+        xive_error(xive, "VST: END %x/%x is remote !?", blk, idx);
+        return -1;
+    }
+
     return pnv_xive_vst_write(PNV_XIVE(xrtr), VST_TSEL_EQDT, blk, idx, end,
                               word_number);
 }
@@ -1368,6 +1387,50 @@ static const MemoryRegionOps pnv_xive_ic_reg_ops = {
 #define PNV_XIVE_SYNC_PUSH          0xf00 /* Sync push context */
 #define PNV_XIVE_SYNC_VPC           0xf80 /* Sync remove VPC store */
 
+static void pnv_xive_end_notify(XiveRouter *xrtr, XiveEAS *eas)
+{
+    PnvXive *xive = PNV_XIVE(xrtr);
+    uint8_t end_blk = xive_get_field64(EAS_END_BLOCK, eas->w);
+    uint32_t end_idx = xive_get_field64(EAS_END_INDEX, eas->w);
+    uint32_t end_data = xive_get_field64(EAS_END_DATA, eas->w);
+    uint64_t end_vsd = xive->vsds[VST_TSEL_EQDT][end_blk];
+
+    switch (GETFIELD(VSD_MODE, end_vsd)) {
+    case VSD_MODE_EXCLUSIVE:
+        /* Perform the END notification on the local IC. */
+        xive_router_end_notify(xrtr, eas);
+        break;
+
+    case VSD_MODE_FORWARD: {
+        MemTxResult result;
+        uint64_t notif_port = end_vsd & VSD_ADDRESS_MASK;
+        uint64_t data = XIVE_TRIGGER_END | XIVE_TRIGGER_PQ |
+            be64_to_cpu(eas->w);
+
+        /* Forward the store on the remote IC notify page. */
+        address_space_stq_be(&address_space_memory, notif_port, data,
+                             MEMTXATTRS_UNSPECIFIED, &result);
+        if (result != MEMTX_OK) {
+            xive_error(xive, "IC: Forward notif END %x/%x [%x] failed @%"
+                       HWADDR_PRIx, end_blk, end_idx, end_data, notif_port);
+            return;
+        }
+        break;
+    }
+
+    case VSD_MODE_INVALID:
+    default:
+        /* Set FIR */
+        xive_error(xive, "IC: Invalid END VSD for block %x", end_blk);
+        return;
+    }
+}
+
+/*
+ * The notify page can either be used to receive trigger events from
+ * the HW controllers (PHB, PSI) or to reroute interrupts between
+ * Interrupt controllers.
+ */
 static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
 {
     uint8_t blk;
@@ -1376,8 +1439,8 @@ static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
     trace_pnv_xive_ic_hw_trigger(addr, val);
 
     if (val & XIVE_TRIGGER_END) {
-        xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64,
-                   addr, val);
+        val = cpu_to_be64(val);
+        pnv_xive_end_notify(XIVE_ROUTER(xive), (XiveEAS *) &val);
         return;
     }
 
@@ -1917,6 +1980,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&xive->ic_notify_mmio, OBJECT(dev),
                           &pnv_xive_ic_notify_ops,
                           xive, "xive-ic-notify", 1 << xive->ic_shift);
+    xive->ic_notify_mmio.disable_reentrancy_guard = true;
 
     /* The Pervasive LSI trigger and EOI pages (not modeled) */
     memory_region_init_io(&xive->ic_lsi_mmio, OBJECT(dev), &pnv_xive_ic_lsi_ops,
@@ -2017,6 +2081,7 @@ static void pnv_xive_class_init(ObjectClass *klass, void *data)
     xrc->get_nvt = pnv_xive_get_nvt;
     xrc->write_nvt = pnv_xive_write_nvt;
     xrc->get_block_id = pnv_xive_get_block_id;
+    xrc->end_notify = pnv_xive_end_notify;
 
     xnc->notify = pnv_xive_notify;
     xpc->match_nvt  = pnv_xive_match_nvt;
-- 
2.41.0

Re: [PATCH 3/4] ppc/xive: Handle END triggers between chips with MMIOs

[Frederic Barrat]

On 29/08/2023 16:32, Cédric Le Goater wrote:
> The notify page of the interrupt controller can either be used to
> receive trigger events from the HW controllers (PHB, PSI) or to
> reroute interrupts between Interrupt Controllers. In which case, the
> VSD table is used to determine the address of the notify page of the
> remote IC and the store data is forwarded.
> 
> Today, our model grabs the remote VSD (EAS, END, NVT) address using
> pnv_xive_get_remote() helper. Be more precise and implement remote END
> triggers using a store on the remote IC notify page.
> 
> We still have a shortcut in the model for the NVT accesses which we
> will address later.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   hw/intc/pnv_xive_regs.h |  1 +
>   hw/intc/pnv_xive.c      | 69 +++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/pnv_xive_regs.h b/hw/intc/pnv_xive_regs.h
> index c78f030c0260..793847638bce 100644
> --- a/hw/intc/pnv_xive_regs.h
> +++ b/hw/intc/pnv_xive_regs.h
> @@ -228,6 +228,7 @@
>    *       VSD and is only meant to be used in indirect mode !
>    */
>   #define VSD_MODE                PPC_BITMASK(0, 1)
> +#define  VSD_MODE_INVALID       0
>   #define  VSD_MODE_SHARED        1
>   #define  VSD_MODE_EXCLUSIVE     2
>   #define  VSD_MODE_FORWARD       3
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index b2bafd61b157..aae5cb6f607b 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -225,6 +225,11 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>   
>       /* Remote VST access */
>       if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
> +        if (type != VST_TSEL_VPDT) {
> +            xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
> +                       info->name, blk, idx);
> +            return 0;
> +        }
>           xive = pnv_xive_get_remote(blk);
>   
>           return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
> @@ -294,12 +299,26 @@ static int pnv_xive_vst_write(PnvXive *xive, uint32_t type, uint8_t blk,
>   static int pnv_xive_get_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>                               XiveEND *end)
>   {
> +    PnvXive *xive = PNV_XIVE(xrtr);
> +
> +    if (pnv_xive_block_id(xive) != blk) {
> +        xive_error(xive, "VST: END %x/%x is remote !?", blk, idx);
> +        return -1;
> +    }
> +
>       return pnv_xive_vst_read(PNV_XIVE(xrtr), VST_TSEL_EQDT, blk, idx, end);
>   }
>   
>   static int pnv_xive_write_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>                                 XiveEND *end, uint8_t word_number)
>   {
> +    PnvXive *xive = PNV_XIVE(xrtr);
> +
> +    if (pnv_xive_block_id(xive) != blk) {
> +        xive_error(xive, "VST: END %x/%x is remote !?", blk, idx);
> +        return -1;
> +    }
> +
>       return pnv_xive_vst_write(PNV_XIVE(xrtr), VST_TSEL_EQDT, blk, idx, end,
>                                 word_number);
>   }
> @@ -1368,6 +1387,50 @@ static const MemoryRegionOps pnv_xive_ic_reg_ops = {
>   #define PNV_XIVE_SYNC_PUSH          0xf00 /* Sync push context */
>   #define PNV_XIVE_SYNC_VPC           0xf80 /* Sync remove VPC store */
>   
> +static void pnv_xive_end_notify(XiveRouter *xrtr, XiveEAS *eas)
> +{
> +    PnvXive *xive = PNV_XIVE(xrtr);
> +    uint8_t end_blk = xive_get_field64(EAS_END_BLOCK, eas->w);
> +    uint32_t end_idx = xive_get_field64(EAS_END_INDEX, eas->w);
> +    uint32_t end_data = xive_get_field64(EAS_END_DATA, eas->w);
> +    uint64_t end_vsd = xive->vsds[VST_TSEL_EQDT][end_blk];
> +
> +    switch (GETFIELD(VSD_MODE, end_vsd)) {
> +    case VSD_MODE_EXCLUSIVE:
> +        /* Perform the END notification on the local IC. */
> +        xive_router_end_notify(xrtr, eas);
> +        break;
> +
> +    case VSD_MODE_FORWARD: {
> +        MemTxResult result;
> +        uint64_t notif_port = end_vsd & VSD_ADDRESS_MASK;
> +        uint64_t data = XIVE_TRIGGER_END | XIVE_TRIGGER_PQ |
> +            be64_to_cpu(eas->w);
> +
> +        /* Forward the store on the remote IC notify page. */
> +        address_space_stq_be(&address_space_memory, notif_port, data,
> +                             MEMTXATTRS_UNSPECIFIED, &result);
> +        if (result != MEMTX_OK) {
> +            xive_error(xive, "IC: Forward notif END %x/%x [%x] failed @%"
> +                       HWADDR_PRIx, end_blk, end_idx, end_data, notif_port);
> +            return;
> +        }
> +        break;
> +    }
> +
> +    case VSD_MODE_INVALID:
> +    default:
> +        /* Set FIR */
> +        xive_error(xive, "IC: Invalid END VSD for block %x", end_blk);
> +        return;
> +    }
> +}
> +
> +/*
> + * The notify page can either be used to receive trigger events from
> + * the HW controllers (PHB, PSI) or to reroute interrupts between
> + * Interrupt controllers.
> + */
>   static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
>   {
>       uint8_t blk;
> @@ -1376,8 +1439,8 @@ static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
>       trace_pnv_xive_ic_hw_trigger(addr, val);
>   
>       if (val & XIVE_TRIGGER_END) {
> -        xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64,
> -                   addr, val);
> +        val = cpu_to_be64(val);
> +        pnv_xive_end_notify(XIVE_ROUTER(xive), (XiveEAS *) &val);
>           return;
>       }
>   
> @@ -1917,6 +1980,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
>       memory_region_init_io(&xive->ic_notify_mmio, OBJECT(dev),
>                             &pnv_xive_ic_notify_ops,
>                             xive, "xive-ic-notify", 1 << xive->ic_shift);
> +    xive->ic_notify_mmio.disable_reentrancy_guard = true;
>   
>       /* The Pervasive LSI trigger and EOI pages (not modeled) */
>       memory_region_init_io(&xive->ic_lsi_mmio, OBJECT(dev), &pnv_xive_ic_lsi_ops,
> @@ -2017,6 +2081,7 @@ static void pnv_xive_class_init(ObjectClass *klass, void *data)
>       xrc->get_nvt = pnv_xive_get_nvt;
>       xrc->write_nvt = pnv_xive_write_nvt;
>       xrc->get_block_id = pnv_xive_get_block_id;
> +    xrc->end_notify = pnv_xive_end_notify;
>   
>       xnc->notify = pnv_xive_notify;
>       xpc->match_nvt  = pnv_xive_match_nvt;

[PATCH 4/4] ppc/xive: Add support for the PC MMIOs

[Cédric Le Goater]

The XIVE interrupt contoller maintains various fields on interrupt
targets in a structure called NVT. Each unit has a NVT cache, backed
by RAM.

When the NVT structure is not local (in RAM) to the chip, the XIVE
interrupt controller forwards the memory operation to the owning chip
using the PC MMIO region configured for this purpose. QEMU does not
need to be so precise since software shouldn't perform any of these
operations. The model implementation is simplified to return the RAM
address of the NVT structure which is then used by pnv_xive_vst_write
or read to perform the operation in RAM.

Remove the last use of pnv_xive_get_remote().

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/pnv_xive.c | 84 ++++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 36 deletions(-)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index aae5cb6f607b..9b10e905195a 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -84,28 +84,6 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
     return blk;
 }
 
-/*
- * Remote access to controllers. HW uses MMIOs. For now, a simple scan
- * of the chips is good enough.
- *
- * TODO: Block scope support
- */
-static PnvXive *pnv_xive_get_remote(uint8_t blk)
-{
-    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
-    int i;
-
-    for (i = 0; i < pnv->num_chips; i++) {
-        Pnv9Chip *chip9 = PNV9_CHIP(pnv->chips[i]);
-        PnvXive *xive = &chip9->xive;
-
-        if (pnv_xive_block_id(xive) == blk) {
-            return xive;
-        }
-    }
-    return NULL;
-}
-
 /*
  * VST accessors for SBE, EAT, ENDT, NVT
  *
@@ -209,6 +187,42 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
     return pnv_xive_vst_addr_direct(xive, type, vsd, (idx % vst_per_page));
 }
 
+/*
+ * This is a simplified model of operation forwarding on a remote IC.
+ *
+ * A PC MMIO address is built to identify the NVT structure. The load
+ * on the remote IC will return the address of the structure in RAM,
+ * which will then be used by pnv_xive_vst_write/read to perform the
+ * RAM operation.
+ */
+static uint64_t pnv_xive_vst_addr_remote(PnvXive *xive, uint32_t type,
+                                         uint64_t vsd, uint8_t blk,
+                                         uint32_t idx)
+{
+    const XiveVstInfo *info = &vst_infos[type];
+    uint64_t remote_addr = vsd & VSD_ADDRESS_MASK;
+    uint64_t vst_addr;
+    MemTxResult result;
+
+    if (type != VST_TSEL_VPDT) {
+        xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
+                   info->name, blk, idx);
+        return 0;
+    }
+
+    remote_addr |= idx << xive->pc_shift;
+
+    vst_addr = address_space_ldq_be(&address_space_memory, remote_addr,
+                                    MEMTXATTRS_UNSPECIFIED, &result);
+    if (result != MEMTX_OK) {
+        xive_error(xive, "VST: read failed at @0x%"  HWADDR_PRIx
+                   " for NVT %x/%x\n", remote_addr, blk, idx);
+        return 0;
+    }
+
+    return vst_addr;
+}
+
 static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
                                   uint32_t idx)
 {
@@ -225,14 +239,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
 
     /* Remote VST access */
     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
-        if (type != VST_TSEL_VPDT) {
-            xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
-                       info->name, blk, idx);
-            return 0;
-        }
-        xive = pnv_xive_get_remote(blk);
-
-        return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
+        return pnv_xive_vst_addr_remote(xive, type, vsd, blk, idx);
     }
 
     if (VSD_INDIRECT & vsd) {
@@ -1785,16 +1792,20 @@ static const MemoryRegionOps pnv_xive_vc_ops = {
 };
 
 /*
- * Presenter Controller MMIO region. The Virtualization Controller
- * updates the IPB in the NVT table when required. Not modeled.
+ * Presenter Controller MMIO region. Points to the NVT sets.
+ *
+ * HW implements all possible mem ops to the underlying NVT structure
+ * but QEMU does not need to be so precise. The model implementation
+ * simply returns the RAM address of the NVT structure which is then
+ * used by pnv_xive_vst_write/read to perform the RAM operation.
  */
-static uint64_t pnv_xive_pc_read(void *opaque, hwaddr addr,
-                                 unsigned size)
+static uint64_t pnv_xive_pc_read(void *opaque, hwaddr offset, unsigned size)
 {
     PnvXive *xive = PNV_XIVE(opaque);
+    uint32_t nvt_idx = offset >> xive->pc_shift;
+    uint8_t blk = pnv_xive_block_id(xive); /* TODO: VDT -> block xlate */
 
-    xive_error(xive, "PC: invalid read @%"HWADDR_PRIx, addr);
-    return -1;
+    return pnv_xive_vst_addr(xive, VST_TSEL_VPDT, blk, nvt_idx);
 }
 
 static void pnv_xive_pc_write(void *opaque, hwaddr addr,
@@ -2016,6 +2027,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
     /* Presenter Controller MMIO region (not modeled) */
     memory_region_init_io(&xive->pc_mmio, OBJECT(xive), &pnv_xive_pc_ops, xive,
                           "xive-pc", PNV9_XIVE_PC_SIZE);
+    xive->pc_mmio.disable_reentrancy_guard = true;
 
     /* Thread Interrupt Management Area (Direct) */
     memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops,
-- 
2.41.0

Re: [PATCH 4/4] ppc/xive: Add support for the PC MMIOs

[Frederic Barrat]

On 29/08/2023 16:32, Cédric Le Goater wrote:
> The XIVE interrupt contoller maintains various fields on interrupt
> targets in a structure called NVT. Each unit has a NVT cache, backed
> by RAM.
> 
> When the NVT structure is not local (in RAM) to the chip, the XIVE
> interrupt controller forwards the memory operation to the owning chip
> using the PC MMIO region configured for this purpose. QEMU does not
> need to be so precise since software shouldn't perform any of these
> operations. The model implementation is simplified to return the RAM
> address of the NVT structure which is then used by pnv_xive_vst_write
> or read to perform the operation in RAM.
> 
> Remove the last use of pnv_xive_get_remote().
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> -


Nice cleanup

Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   hw/intc/pnv_xive.c | 84 ++++++++++++++++++++++++++--------------------
>   1 file changed, 48 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index aae5cb6f607b..9b10e905195a 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -84,28 +84,6 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
>       return blk;
>   }
>   
> -/*
> - * Remote access to controllers. HW uses MMIOs. For now, a simple scan
> - * of the chips is good enough.
> - *
> - * TODO: Block scope support
> - */
> -static PnvXive *pnv_xive_get_remote(uint8_t blk)
> -{
> -    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> -    int i;
> -
> -    for (i = 0; i < pnv->num_chips; i++) {
> -        Pnv9Chip *chip9 = PNV9_CHIP(pnv->chips[i]);
> -        PnvXive *xive = &chip9->xive;
> -
> -        if (pnv_xive_block_id(xive) == blk) {
> -            return xive;
> -        }
> -    }
> -    return NULL;
> -}
> -
>   /*
>    * VST accessors for SBE, EAT, ENDT, NVT
>    *
> @@ -209,6 +187,42 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>       return pnv_xive_vst_addr_direct(xive, type, vsd, (idx % vst_per_page));
>   }
>   
> +/*
> + * This is a simplified model of operation forwarding on a remote IC.
> + *
> + * A PC MMIO address is built to identify the NVT structure. The load
> + * on the remote IC will return the address of the structure in RAM,
> + * which will then be used by pnv_xive_vst_write/read to perform the
> + * RAM operation.
> + */
> +static uint64_t pnv_xive_vst_addr_remote(PnvXive *xive, uint32_t type,
> +                                         uint64_t vsd, uint8_t blk,
> +                                         uint32_t idx)
> +{
> +    const XiveVstInfo *info = &vst_infos[type];
> +    uint64_t remote_addr = vsd & VSD_ADDRESS_MASK;
> +    uint64_t vst_addr;
> +    MemTxResult result;
> +
> +    if (type != VST_TSEL_VPDT) {
> +        xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
> +                   info->name, blk, idx);
> +        return 0;
> +    }
> +
> +    remote_addr |= idx << xive->pc_shift;
> +
> +    vst_addr = address_space_ldq_be(&address_space_memory, remote_addr,
> +                                    MEMTXATTRS_UNSPECIFIED, &result);
> +    if (result != MEMTX_OK) {
> +        xive_error(xive, "VST: read failed at @0x%"  HWADDR_PRIx
> +                   " for NVT %x/%x\n", remote_addr, blk, idx);
> +        return 0;
> +    }
> +
> +    return vst_addr;
> +}
> +
>   static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>                                     uint32_t idx)
>   {
> @@ -225,14 +239,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>   
>       /* Remote VST access */
>       if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
> -        if (type != VST_TSEL_VPDT) {
> -            xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
> -                       info->name, blk, idx);
> -            return 0;
> -        }
> -        xive = pnv_xive_get_remote(blk);
> -
> -        return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
> +        return pnv_xive_vst_addr_remote(xive, type, vsd, blk, idx);
>       }
>   
>       if (VSD_INDIRECT & vsd) {
> @@ -1785,16 +1792,20 @@ static const MemoryRegionOps pnv_xive_vc_ops = {
>   };
>   
>   /*
> - * Presenter Controller MMIO region. The Virtualization Controller
> - * updates the IPB in the NVT table when required. Not modeled.
> + * Presenter Controller MMIO region. Points to the NVT sets.
> + *
> + * HW implements all possible mem ops to the underlying NVT structure
> + * but QEMU does not need to be so precise. The model implementation
> + * simply returns the RAM address of the NVT structure which is then
> + * used by pnv_xive_vst_write/read to perform the RAM operation.
>    */
> -static uint64_t pnv_xive_pc_read(void *opaque, hwaddr addr,
> -                                 unsigned size)
> +static uint64_t pnv_xive_pc_read(void *opaque, hwaddr offset, unsigned size)
>   {
>       PnvXive *xive = PNV_XIVE(opaque);
> +    uint32_t nvt_idx = offset >> xive->pc_shift;
> +    uint8_t blk = pnv_xive_block_id(xive); /* TODO: VDT -> block xlate */
>   
> -    xive_error(xive, "PC: invalid read @%"HWADDR_PRIx, addr);
> -    return -1;
> +    return pnv_xive_vst_addr(xive, VST_TSEL_VPDT, blk, nvt_idx);
>   }
>   
>   static void pnv_xive_pc_write(void *opaque, hwaddr addr,
> @@ -2016,6 +2027,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
>       /* Presenter Controller MMIO region (not modeled) */
>       memory_region_init_io(&xive->pc_mmio, OBJECT(xive), &pnv_xive_pc_ops, xive,
>                             "xive-pc", PNV9_XIVE_PC_SIZE);
> +    xive->pc_mmio.disable_reentrancy_guard = true;
>   
>       /* Thread Interrupt Management Area (Direct) */
>       memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops,

Re: [PATCH 4/4] ppc/xive: Add support for the PC MMIOs

[Cédric Le Goater]

On 8/31/23 09:57, Frederic Barrat wrote:
> 
> 
> On 29/08/2023 16:32, Cédric Le Goater wrote:
>> The XIVE interrupt contoller maintains various fields on interrupt
>> targets in a structure called NVT. Each unit has a NVT cache, backed
>> by RAM.
>>
>> When the NVT structure is not local (in RAM) to the chip, the XIVE
>> interrupt controller forwards the memory operation to the owning chip
>> using the PC MMIO region configured for this purpose. QEMU does not
>> need to be so precise since software shouldn't perform any of these
>> operations. The model implementation is simplified to return the RAM
>> address of the NVT structure which is then used by pnv_xive_vst_write
>> or read to perform the operation in RAM.
>>
>> Remove the last use of pnv_xive_get_remote().
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> -
> 
> 
> Nice cleanup

Well, it is not perfect (I was lazy in implementing all the ops) but,
at least, the model is now using all addresses set by FW in the VSD
structures (VSD_MODE_FORWARD).

Thanks,

C.

> 
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
>    Fred
> 
> 
>>   hw/intc/pnv_xive.c | 84 ++++++++++++++++++++++++++--------------------
>>   1 file changed, 48 insertions(+), 36 deletions(-)
>>
>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
>> index aae5cb6f607b..9b10e905195a 100644
>> --- a/hw/intc/pnv_xive.c
>> +++ b/hw/intc/pnv_xive.c
>> @@ -84,28 +84,6 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
>>       return blk;
>>   }
>> -/*
>> - * Remote access to controllers. HW uses MMIOs. For now, a simple scan
>> - * of the chips is good enough.
>> - *
>> - * TODO: Block scope support
>> - */
>> -static PnvXive *pnv_xive_get_remote(uint8_t blk)
>> -{
>> -    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>> -    int i;
>> -
>> -    for (i = 0; i < pnv->num_chips; i++) {
>> -        Pnv9Chip *chip9 = PNV9_CHIP(pnv->chips[i]);
>> -        PnvXive *xive = &chip9->xive;
>> -
>> -        if (pnv_xive_block_id(xive) == blk) {
>> -            return xive;
>> -        }
>> -    }
>> -    return NULL;
>> -}
>> -
>>   /*
>>    * VST accessors for SBE, EAT, ENDT, NVT
>>    *
>> @@ -209,6 +187,42 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>>       return pnv_xive_vst_addr_direct(xive, type, vsd, (idx % vst_per_page));
>>   }
>> +/*
>> + * This is a simplified model of operation forwarding on a remote IC.
>> + *
>> + * A PC MMIO address is built to identify the NVT structure. The load
>> + * on the remote IC will return the address of the structure in RAM,
>> + * which will then be used by pnv_xive_vst_write/read to perform the
>> + * RAM operation.
>> + */
>> +static uint64_t pnv_xive_vst_addr_remote(PnvXive *xive, uint32_t type,
>> +                                         uint64_t vsd, uint8_t blk,
>> +                                         uint32_t idx)
>> +{
>> +    const XiveVstInfo *info = &vst_infos[type];
>> +    uint64_t remote_addr = vsd & VSD_ADDRESS_MASK;
>> +    uint64_t vst_addr;
>> +    MemTxResult result;
>> +
>> +    if (type != VST_TSEL_VPDT) {
>> +        xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
>> +                   info->name, blk, idx);
>> +        return 0;
>> +    }
>> +
>> +    remote_addr |= idx << xive->pc_shift;
>> +
>> +    vst_addr = address_space_ldq_be(&address_space_memory, remote_addr,
>> +                                    MEMTXATTRS_UNSPECIFIED, &result);
>> +    if (result != MEMTX_OK) {
>> +        xive_error(xive, "VST: read failed at @0x%"  HWADDR_PRIx
>> +                   " for NVT %x/%x\n", remote_addr, blk, idx);
>> +        return 0;
>> +    }
>> +
>> +    return vst_addr;
>> +}
>> +
>>   static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>>                                     uint32_t idx)
>>   {
>> @@ -225,14 +239,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>>       /* Remote VST access */
>>       if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>> -        if (type != VST_TSEL_VPDT) {
>> -            xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
>> -                       info->name, blk, idx);
>> -            return 0;
>> -        }
>> -        xive = pnv_xive_get_remote(blk);
>> -
>> -        return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
>> +        return pnv_xive_vst_addr_remote(xive, type, vsd, blk, idx);
>>       }
>>       if (VSD_INDIRECT & vsd) {
>> @@ -1785,16 +1792,20 @@ static const MemoryRegionOps pnv_xive_vc_ops = {
>>   };
>>   /*
>> - * Presenter Controller MMIO region. The Virtualization Controller
>> - * updates the IPB in the NVT table when required. Not modeled.
>> + * Presenter Controller MMIO region. Points to the NVT sets.
>> + *
>> + * HW implements all possible mem ops to the underlying NVT structure
>> + * but QEMU does not need to be so precise. The model implementation
>> + * simply returns the RAM address of the NVT structure which is then
>> + * used by pnv_xive_vst_write/read to perform the RAM operation.
>>    */
>> -static uint64_t pnv_xive_pc_read(void *opaque, hwaddr addr,
>> -                                 unsigned size)
>> +static uint64_t pnv_xive_pc_read(void *opaque, hwaddr offset, unsigned size)
>>   {
>>       PnvXive *xive = PNV_XIVE(opaque);
>> +    uint32_t nvt_idx = offset >> xive->pc_shift;
>> +    uint8_t blk = pnv_xive_block_id(xive); /* TODO: VDT -> block xlate */
>> -    xive_error(xive, "PC: invalid read @%"HWADDR_PRIx, addr);
>> -    return -1;
>> +    return pnv_xive_vst_addr(xive, VST_TSEL_VPDT, blk, nvt_idx);
>>   }
>>   static void pnv_xive_pc_write(void *opaque, hwaddr addr,
>> @@ -2016,6 +2027,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
>>       /* Presenter Controller MMIO region (not modeled) */
>>       memory_region_init_io(&xive->pc_mmio, OBJECT(xive), &pnv_xive_pc_ops, xive,
>>                             "xive-pc", PNV9_XIVE_PC_SIZE);
>> +    xive->pc_mmio.disable_reentrancy_guard = true;
>>       /* Thread Interrupt Management Area (Direct) */
>>       memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops,