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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 7/3/23 05:08, 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) 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>
> ---

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

> Changelog:
> v2: rename function and variable
>      rebase to Danel's ppc-next
> 
> hw/intc/pnv_xive2.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index e8ab176de6..82fcd3ea22 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -1590,6 +1590,18 @@ 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_hw_page_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 & ((1ull << xive->ic_shift) - 1);
> +}
> +
>   static XiveTCTX *pnv_xive2_get_indirect_tctx(PnvXive2 *xive, uint32_t pir)
>   {
>       PnvChip *chip = xive->chip;
> @@ -1612,14 +1624,16 @@ static uint64_t pnv_xive2_ic_tm_indirect_read(void *opaque, hwaddr offset,
>                                                 unsigned size)
>   {
>       PnvXive2 *xive = PNV_XIVE2(opaque);
> +    hwaddr hw_page_offset;
>       uint32_t pir;
>       XiveTCTX *tctx;
>       uint64_t val = -1;
>   
>       pir = pnv_xive2_ic_tm_get_pir(xive, offset);
> +    hw_page_offset = pnv_xive2_ic_tm_get_hw_page_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, hw_page_offset, size);
>       }
>   
>       return val;
> @@ -1629,13 +1643,15 @@ static void pnv_xive2_ic_tm_indirect_write(void *opaque, hwaddr offset,
>                                              uint64_t val, unsigned size)
>   {
>       PnvXive2 *xive = PNV_XIVE2(opaque);
> +    hwaddr hw_page_offset;
>       uint32_t pir;
>       XiveTCTX *tctx;
>   
>       pir = pnv_xive2_ic_tm_get_pir(xive, offset);
> +    hw_page_offset = pnv_xive2_ic_tm_get_hw_page_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, hw_page_offset, val, size);
>       }
>   }
>