[PATCH 0/4] Various xive fixes

[PATCH 0/4] Various xive fixes

[Frederic Barrat]

A set of small fixes for the interrupt controller (xive2) on P10.

Frederic Barrat (4):
  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: Handle TIMA access through all ports

 hw/intc/pnv_xive2.c      | 20 +++++++++++++++++++-
 hw/intc/pnv_xive2_regs.h |  8 ++++++++
 hw/intc/xive.c           | 18 ++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.40.1

[PATCH 1/4] pnv/xive2: Add definition for TCTXT Config register

[Frederic Barrat]

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

Re: [PATCH 1/4] pnv/xive2: Add definition for TCTXT Config register

[Cédric Le Goater]

On 5/30/23 18:11, Frederic Barrat wrote:
> 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

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

If you respin, please add the same kind of support to POWER9.

Thanks,

C.


> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.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
>    */

Re: [PATCH 1/4] pnv/xive2: Add definition for TCTXT Config register

[Frederic Barrat]

On 30/05/2023 18:31, Cédric Le Goater wrote:
> On 5/30/23 18:11, Frederic Barrat wrote:
>> 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
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> If you respin, please add the same kind of support to POWER9.


It's already in place for read and write:

static void pnv_xive_ic_reg_write()
...
       case PC_TCTXT_CFG:


Skiboot is using it and we don't get an error message, even with 
guest_errors. So it looks good.

  Fred


> 
> Thanks,
> 
> C.
> 
> 
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.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
>>    */
> 

[PATCH 2/4] pnv/xive2: Add definition for the ESB cache configuration register

[Frederic Barrat]

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

Re: [PATCH 2/4] pnv/xive2: Add definition for the ESB cache configuration register

[Cédric Le Goater]

On 5/30/23 18:11, Frederic Barrat wrote:
> 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>

Thanks,

C.


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

[PATCH 3/4] pnv/xive2: Allow writes to the Physical Thread Enable registers

[Frederic Barrat]

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.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.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

Re: [PATCH 3/4] pnv/xive2: Allow writes to the Physical Thread Enable registers

[Cédric Le Goater]

On 5/30/23 18:11, Frederic Barrat wrote:
> 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.

Indeed.


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

Fixes: da71b7e3ed45 ("ppc/pnv: Add a XIVE2 controller to the POWER10 chip")

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

Thanks,

C.



> ---
>   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:

[PATCH 4/4] pnv/xive2: Handle TIMA access through all ports

[Frederic Barrat]

The Thread Interrupt Management Area (TIMA) can be accessed through 4
ports/snoop buses, 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.

The TIMA registers are in the 0x0 -> 0x3F range and there are 2
indication bits for special operations (bits 10 and 11; everything
fits on a 4k page). So the port address bits fall in between and are
"don't care" for the hardware when processing the TIMA operation. So
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>
---
 hw/intc/pnv_xive2.c |  4 ++++
 hw/intc/xive.c      | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index 132f82a035..c80316657a 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 &= 0xC3F; /* See comment in xive_tctx_tm_write() */
+
     /* 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 &= 0xC3F; /* See comment in xive_tctx_tm_read() */
+
     /* 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 a986b96843..c1abfae31d 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -527,6 +527,15 @@ void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
 
     trace_xive_tctx_tm_write(offset, size, value);
 
+    /*
+     * The TIMA can be accessed through 4 ports/snoop buses, with
+     * addresses 0x80 apart.
+     * However, the offset bits between the "special op" bits and the
+     * MSB of the range used for the TIMA registers are "don't care"
+     * for the hardware, so we filter them out.
+     */
+    offset &= 0xC3F;
+
     /*
      * TODO: check V bit in Q[0-3]W2
      */
@@ -566,6 +575,15 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
     const XiveTmOp *xto;
     uint64_t ret;
 
+    /*
+     * The TIMA can be accessed through 4 ports/snoop buses, with
+     * addresses 0x80 apart.
+     * However, the offset bits between the "special op" bits and the
+     * MSB of the range used for the TIMA registers are "don't care"
+     * for the hardware, so we filter them out.
+     */
+    offset &= 0xC3F;
+
     /*
      * TODO: check V bit in Q[0-3]W2
      */
-- 
2.40.1

Re: [PATCH 4/4] pnv/xive2: Handle TIMA access through all ports

[Cédric Le Goater]

On 5/30/23 18:11, Frederic Barrat wrote:
> The Thread Interrupt Management Area (TIMA) can be accessed through 4
> ports/snoop buses, 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.

and can we have some nice examples of how these ports are used ? only for
snooping or also for balancing operations ? which ones ?

> The TIMA registers are in the 0x0 -> 0x3F range and there are 2
> indication bits for special operations (bits 10 and 11; everything
> fits on a 4k page). So the port address bits fall in between and are
> "don't care" for the hardware when processing the TIMA operation. So
> 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>

Thanks,

C.

> ---
>   hw/intc/pnv_xive2.c |  4 ++++
>   hw/intc/xive.c      | 18 ++++++++++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index 132f82a035..c80316657a 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 &= 0xC3F; /* See comment in xive_tctx_tm_write() */
> +
>       /* 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 &= 0xC3F; /* See comment in xive_tctx_tm_read() */
> +
>       /* 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 a986b96843..c1abfae31d 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -527,6 +527,15 @@ void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
>   
>       trace_xive_tctx_tm_write(offset, size, value);
>   
> +    /*
> +     * The TIMA can be accessed through 4 ports/snoop buses, with
> +     * addresses 0x80 apart.
> +     * However, the offset bits between the "special op" bits and the
> +     * MSB of the range used for the TIMA registers are "don't care"
> +     * for the hardware, so we filter them out.
> +     */
> +    offset &= 0xC3F;
> +
>       /*
>        * TODO: check V bit in Q[0-3]W2
>        */
> @@ -566,6 +575,15 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
>       const XiveTmOp *xto;
>       uint64_t ret;
>   
> +    /*
> +     * The TIMA can be accessed through 4 ports/snoop buses, with
> +     * addresses 0x80 apart.
> +     * However, the offset bits between the "special op" bits and the
> +     * MSB of the range used for the TIMA registers are "don't care"
> +     * for the hardware, so we filter them out.
> +     */
> +    offset &= 0xC3F;
> +
>       /*
>        * TODO: check V bit in Q[0-3]W2
>        */

Re: [PATCH 4/4] pnv/xive2: Handle TIMA access through all ports

[Cédric Le Goater]

On 5/30/23 18:40, Cédric Le Goater wrote:
> On 5/30/23 18:11, Frederic Barrat wrote:
>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>> ports/snoop buses, 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.
> 
> and can we have some nice examples of how these ports are used ? only for
> snooping or also for balancing operations ? which ones ?
> 
>> The TIMA registers are in the 0x0 -> 0x3F range and there are 2
>> indication bits for special operations (bits 10 and 11; everything
>> fits on a 4k page). So the port address bits fall in between and are
>> "don't care" for the hardware when processing the TIMA operation. So
>> 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>

one extra comment, since we already have one mask for the tima offsets :

     /*
      * First, check for special operations in the 2K region
      */
     if (offset & 0x800) {

I think it would be cleaner to add some defines in the reg definition file.

Can come later.

Thanks,

C.

Re: [PATCH 4/4] pnv/xive2: Handle TIMA access through all ports

[Frederic Barrat]

On 30/05/2023 18:49, Cédric Le Goater wrote:
> On 5/30/23 18:40, Cédric Le Goater wrote:
>> On 5/30/23 18:11, Frederic Barrat wrote:
>>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>>> ports/snoop buses, 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.
>>
>> and can we have some nice examples of how these ports are used ? only for
>> snooping or also for balancing operations ? which ones ?
>>
>>> The TIMA registers are in the 0x0 -> 0x3F range and there are 2
>>> indication bits for special operations (bits 10 and 11; everything
>>> fits on a 4k page). So the port address bits fall in between and are
>>> "don't care" for the hardware when processing the TIMA operation. So
>>> 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>
> 
> one extra comment, since we already have one mask for the tima offsets :
> 
>      /*
>       * First, check for special operations in the 2K region
>       */
>      if (offset & 0x800) {
> 
> I think it would be cleaner to add some defines in the reg definition file.


Yeah, you're right, I should respin it with some proper defines. And 
I'll add one for the special op bit(s).

   Fred


> 
> Can come later.
> 
> Thanks,
> 
> C.
> 

Re: [PATCH 4/4] pnv/xive2: Handle TIMA access through all ports

[Frederic Barrat]

On 30/05/2023 18:40, Cédric Le Goater wrote:
> On 5/30/23 18:11, Frederic Barrat wrote:
>> The Thread Interrupt Management Area (TIMA) can be accessed through 4
>> ports/snoop buses, 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.
> 
> and can we have some nice examples of how these ports are used ? only for
> snooping or also for balancing operations ? which ones ?


This is really "static" balancing. The snoop bus 0 and 3 are known to be 
the busiest, for various kind of operations. We found out recently that 
another hypervisor is always using snoop bus 1 for TIMA accesses, to 
spread the load. So it is really empirical.

xive_tm_raw_write/read() were actually working (by chance?), since they 
only retain the least significant bits, so the snoop address bits were 
dropped. The problem was really for the special op detection, which 
matches on the full address.

   Fred


>> The TIMA registers are in the 0x0 -> 0x3F range and there are 2
>> indication bits for special operations (bits 10 and 11; everything
>> fits on a 4k page). So the port address bits fall in between and are
>> "don't care" for the hardware when processing the TIMA operation. So
>> 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>
> 
> Thanks,
> 
> C.
> 
>> ---
>>   hw/intc/pnv_xive2.c |  4 ++++
>>   hw/intc/xive.c      | 18 ++++++++++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
>> index 132f82a035..c80316657a 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 &= 0xC3F; /* See comment in xive_tctx_tm_write() */
>> +
>>       /* 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 &= 0xC3F; /* See comment in xive_tctx_tm_read() */
>> +
>>       /* 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 a986b96843..c1abfae31d 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -527,6 +527,15 @@ void xive_tctx_tm_write(XivePresenter *xptr, 
>> XiveTCTX *tctx, hwaddr offset,
>>       trace_xive_tctx_tm_write(offset, size, value);
>> +    /*
>> +     * The TIMA can be accessed through 4 ports/snoop buses, with
>> +     * addresses 0x80 apart.
>> +     * However, the offset bits between the "special op" bits and the
>> +     * MSB of the range used for the TIMA registers are "don't care"
>> +     * for the hardware, so we filter them out.
>> +     */
>> +    offset &= 0xC3F;
>> +
>>       /*
>>        * TODO: check V bit in Q[0-3]W2
>>        */
>> @@ -566,6 +575,15 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, 
>> XiveTCTX *tctx, hwaddr offset,
>>       const XiveTmOp *xto;
>>       uint64_t ret;
>> +    /*
>> +     * The TIMA can be accessed through 4 ports/snoop buses, with
>> +     * addresses 0x80 apart.
>> +     * However, the offset bits between the "special op" bits and the
>> +     * MSB of the range used for the TIMA registers are "don't care"
>> +     * for the hardware, so we filter them out.
>> +     */
>> +    offset &= 0xC3F;
>> +
>>       /*
>>        * TODO: check V bit in Q[0-3]W2
>>        */
>