Re: [PULL 05/29] pnv/xive2: Handle TIMA access through all ports

[Frederic Barrat]

On 21/06/2023 09:18, Cédric Le Goater wrote:
> The XIVE2 TM ops are implemented with a shortcut (See the TODO in
> pnv_xive2_tm_*()). We could
>
> 1. extend xive_tctx_tm_write/read with a 'bool gen1_tima_os' parameter:
>
>      xive_tctx_tm_write(xptr, tctx, offset, value, size, gen1_tima_os);
>
>     and use the bool in xive_tm_find_op() to select a XiveTmOp array.
>     The new xive2_tm_operations[] would be defined as xive_tm_operations[]
>     but with an override of HV_PUSH_OS_CTX_OFFSET and 
> HV_PULL_OS_CTX_OFFSET.
>
> 2. or extend the XivePresenterClass with a get_config() handler like it
>     was done for Xive2RouterClass().
>
> 3. or introduce an array of XiveTmOp under XivePresenterClass defined by
>     the controller variant.
>
> In any case, we need a new xive2_tm_operations[] for the XIVE2 TM register
> layout. Option 1 is simpler I think.


I was also leaning on introducing a xive2_tm_operations[] array of 
operations to fix it correctly.

While I agree it's the simplest, I'm not fond of (1), since we'd need to 
carry that extra parameter to xive_tm_find_op(). Admittedly it's just 
one extra level, but I went with something which is hopefully what you 
had in mind for (2). I like that we can easily extend it in the future.


> This would  also "fix" the indirect ops in XIVE2, not that we care much
> but it will be cleaner.

I'm not sure I see what you mean here. It cleans up nicely 
pnv_xive2_tm_read/write(), but is that really what you had in mind?


Something related I notice is that when doing an indirect access to the 
TIMA through the IC BAR, we call the TIMA access functions with a NULL 
reference to the presenter:

static uint64_t pnv_xive2_ic_tm_indirect_read(void *opaque, hwaddr offset,
                                              unsigned size)
{
    PnvXive2 *xive = PNV_XIVE2(opaque);
...
    tctx = pnv_xive2_get_indirect_tctx(xive, pir);
    if (tctx) {
        val = xive_tctx_tm_read(NULL, tctx, offset, size);
    }

We seem to mostly ignore that first parameter in 
xive_tctx_tm_read/write(). IIUC, the special ops will be checked with a 
page offset matching ring 0 and won't match anything. Still, it seems a 
bit dangerous and I was wondering:
1. can't we just create it from the PnvXive2 we have at hand?
2. in any case, isn't it always redundant with tctxt->presenter?

  Fred