Re: [PATCH] pnv/xive2: Fix TIMA offset for indirect access

[Cédric Le Goater]

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