Direct TIMA operations can be done through 4 pages, each with a
different privilege level dictating what fields can be accessed. On
the other hand, indirect TIMA accesses on P10 are done through a
single page, which is the equivalent of the most privileged page of
direct TIMA accesses.

The offset in the IC bar of an indirect access specifies what hw
thread is targeted (page shift bits) and the offset in the
TIMA being accessed (the page offset bits). When the indirect
access is calling the underlying direct access functions, it is
therefore important to clearly separate the 2, as the direct functions
assume any page shift bits define the privilege ring level. For
indirect accesses, those bits must be 0. This patch fixes the offset
passed to direct TIMA functions.

It didn't matter for SMT1, as the 2 least significant bits of the page
shift are part of the hw thread ID and always 0, so the direct TIMA
functions were accessing the privilege ring 0 page. With SMT4/8, it is
no longer true.

The fix is specific to P10, as indirect TIMA access on P9 was handled
differently.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/intc/pnv_xive2.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index ed438a20ed..0618e6b0da 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -1590,6 +1590,17 @@ static uint32_t pnv_xive2_ic_tm_get_pir(PnvXive2 *xive, hwaddr offset)
     return xive->chip->chip_id << 8 | offset >> xive->ic_shift;
 }
 
+static uint32_t pnv_xive2_ic_tm_get_offset(PnvXive2 *xive, hwaddr offset)
+{
+    /*
+     * Indirect TIMA accesses are similar to direct accesses for
+     * privilege ring 0. So remove any traces of the hw thread ID from
+     * the offset in the IC BAR as it could be interpreted as the ring
+     * privilege when calling the underlying direct access functions.
+     */
+    return offset & ((1 << xive->ic_shift) - 1);
+}
+
 static XiveTCTX *pnv_xive2_get_indirect_tctx(PnvXive2 *xive, uint32_t pir)
 {
     PnvChip *chip = xive->chip;
@@ -1612,14 +1623,16 @@ static uint64_t pnv_xive2_ic_tm_indirect_read(void *opaque, hwaddr offset,
                                               unsigned size)
 {
     PnvXive2 *xive = PNV_XIVE2(opaque);
+    hwaddr tima_offset;
     uint32_t pir;
     XiveTCTX *tctx;
     uint64_t val = -1;
 
     pir = pnv_xive2_ic_tm_get_pir(xive, offset);
+    tima_offset = pnv_xive2_ic_tm_get_offset(xive, offset);
     tctx = pnv_xive2_get_indirect_tctx(xive, pir);
     if (tctx) {
-        val = xive_tctx_tm_read(NULL, tctx, offset, size);
+        val = xive_tctx_tm_read(NULL, tctx, tima_offset, size);
     }
 
     return val;
@@ -1629,13 +1642,15 @@ static void pnv_xive2_ic_tm_indirect_write(void *opaque, hwaddr offset,
                                            uint64_t val, unsigned size)
 {
     PnvXive2 *xive = PNV_XIVE2(opaque);
+    hwaddr tima_offset;
     uint32_t pir;
     XiveTCTX *tctx;
 
     pir = pnv_xive2_ic_tm_get_pir(xive, offset);
+    tima_offset = pnv_xive2_ic_tm_get_offset(xive, offset);
     tctx = pnv_xive2_get_indirect_tctx(xive, pir);
     if (tctx) {
-        xive_tctx_tm_write(NULL, tctx, offset, val, size);
+        xive_tctx_tm_write(NULL, tctx, tima_offset, val, size);
     }
 }
 
-- 
2.41.0

On 6/30/23 15:20, Frederic Barrat wrote:
> Direct TIMA operations can be done through 4 pages, each with a
> different privilege level dictating what fields can be accessed. On
> the other hand, indirect TIMA accesses on P10 are done through a
> single page, which is the equivalent of the most privileged page of
> direct TIMA accesses.
> 
> The offset in the IC bar of an indirect access specifies what hw
> thread is targeted (page shift bits) 

yes. Max is 128 threads.

> and the offset in the TIMA being accessed (the page offset bits). 

a part of the sentence seems to be missing ?


> When the indirect
> access is calling the underlying direct access functions, it is
> therefore important to clearly separate the 2, as the direct functions
> assume any page shift bits define the privilege ring level. For
> indirect accesses, those bits must be 0. This patch fixes the offset
> passed to direct TIMA functions.
> 
> It didn't matter for SMT1, as the 2 least significant bits of the page
> shift are part of the hw thread ID and always 0, so the direct TIMA
> functions were accessing the privilege ring 0 page. With SMT4/8, it is
> no longer true.

ah yes  !

> The fix is specific to P10, as indirect TIMA access on P9 was handled
> differently.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>   hw/intc/pnv_xive2.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index ed438a20ed..0618e6b0da 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -1590,6 +1590,17 @@ static uint32_t pnv_xive2_ic_tm_get_pir(PnvXive2 *xive, hwaddr offset)
>       return xive->chip->chip_id << 8 | offset >> xive->ic_shift;
>   }
>   
> +static uint32_t pnv_xive2_ic_tm_get_offset(PnvXive2 *xive, hwaddr offset)
> +{
> +    /*
> +     * Indirect TIMA accesses are similar to direct accesses for
> +     * privilege ring 0. So remove any traces of the hw thread ID from
> +     * the offset in the IC BAR as it could be interpreted as the ring
> +     * privilege when calling the underlying direct access functions.
> +     */
> +    return offset & ((1 << xive->ic_shift) - 1);

1ull << ...

This is correct but we should be more explicit in the naming. See below.

> +}
> +
>   static XiveTCTX *pnv_xive2_get_indirect_tctx(PnvXive2 *xive, uint32_t pir)
>   {
>       PnvChip *chip = xive->chip;
> @@ -1612,14 +1623,16 @@ static uint64_t pnv_xive2_ic_tm_indirect_read(void *opaque, hwaddr offset,
>                                                 unsigned size)
>   {
>       PnvXive2 *xive = PNV_XIVE2(opaque);
> +    hwaddr tima_offset;

a 'tima_' prefix is not precise enough. 'hw_' or 'hw_page_' maybe.

I would prefer 'hw_page_' since it relates to the XIVE_TM_HW_PAGE index
in the XIVE TM definitions.


>       uint32_t pir;
>       XiveTCTX *tctx;
>       uint64_t val = -1;
>   
>       pir = pnv_xive2_ic_tm_get_pir(xive, offset);
> +    tima_offset = pnv_xive2_ic_tm_get_offset(xive, offset);

This is extracting the register offset from XIVE_TM_HW_PAGE. Here are
suggestions to be more explicit :

   hw_page_offset = offset & pnv_xive2_ic_page_offset_mask(xive);

or

   hw_page_offset = pnv_xive2_ic_tm_hw_page_offset(xive, offset);


Thanks,

C.


>       tctx = pnv_xive2_get_indirect_tctx(xive, pir);
>       if (tctx) {
> -        val = xive_tctx_tm_read(NULL, tctx, offset, size);
> +        val = xive_tctx_tm_read(NULL, tctx, tima_offset, size);
>       }
>   
>       return val;
> @@ -1629,13 +1642,15 @@ static void pnv_xive2_ic_tm_indirect_write(void *opaque, hwaddr offset,
>                                              uint64_t val, unsigned size)
>   {
>       PnvXive2 *xive = PNV_XIVE2(opaque);
> +    hwaddr tima_offset;
>       uint32_t pir;
>       XiveTCTX *tctx;
>   
>       pir = pnv_xive2_ic_tm_get_pir(xive, offset);
> +    tima_offset = pnv_xive2_ic_tm_get_offset(xive, offset);
>       tctx = pnv_xive2_get_indirect_tctx(xive, pir);
>       if (tctx) {
> -        xive_tctx_tm_write(NULL, tctx, offset, val, size);
> +        xive_tctx_tm_write(NULL, tctx, tima_offset, val, size);
>       }
>   }
>

On 6/30/23 16:10, Cédric Le Goater wrote:
> On 6/30/23 15:20, Frederic Barrat wrote:
>> Direct TIMA operations can be done through 4 pages, each with a
>> different privilege level dictating what fields can be accessed. On
>> the other hand, indirect TIMA accesses on P10 are done through a
>> single page, which is the equivalent of the most privileged page of
>> direct TIMA accesses.
>>
>> The offset in the IC bar of an indirect access specifies what hw
>> thread is targeted (page shift bits) 
> 
> yes. Max is 128 threads.
> 
>> and the offset in the TIMA being accessed (the page offset bits). 
> 
> a part of the sentence seems to be missing ?

Forget that. Got it.

C.