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

[Cédric Le Goater]

On 6/21/23 17:18, Frederic Barrat wrote:
> 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). 

It is.

> I like that we can easily extend it in the future.

Yes. There are new  bits in the Gen2 TM layout that might need an
implementation for other workloads than OPAL/Linux. Having a second
array will help.
 

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

yes.

I meant that indirect ops in XIVE2 didn't bother testing Gen1/Gen2.

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

Yes. I don't remember why. May be because it was not important at
the time.

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

we could.

> 2. in any case, isn't it always redundant with tctxt->presenter?

it is. it should be the same. May add an assert in pnv_xive2_get_indirect_tctx()
if they are different.


Thanks,

C.


>    Fred