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