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

[Cédric Le Goater]

On 6/20/23 16:31, Frederic Barrat wrote:
> On 20/06/2023 13:20, Cédric Le Goater wrote:
> > On 6/20/23 12:45, Peter Maydell wrote:
> > > On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
> > > <danielhb413@gmail.com> wrote:
> > > > From: Frederic Barrat <fbarrat@linux.ibm.com>
> > > >
> > > > The Thread Interrupt Management Area (TIMA) can be accessed through 4
> > > > ports, targeted by the address. The base address of a TIMA
> > > > is using port 0 and the other ports are 0x80 apart. Using one port or
> > > > another can be useful to balance the load on the snoop buses. With
> > > > skiboot and linux, we currently use port 0, but as it tends to be
> > > > busy, another hypervisor is using port 1 for TIMA access.
> > > >
> > > > The port address bits fall in between the special op indication
> > > > bits (the 2 MSBs) and the register offset bits (the 6 LSBs). They are
> > > > "don't care" for the hardware when processing a TIMA operation. This
> > > > patch filters out those port address bits so that a TIMA operation can
> > > > be triggered using any port.
> > > >
> > > > It is also true for indirect access (through the IC BAR) and it's
> > > > actually nothing new, it was already the case on P9. Which helps here,
> > > > as the TIMA handling code is common between P9 (xive) and P10 (xive2).
> > > >
> > > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > > Message-Id: <20230601121331.487207-6-fbarrat@linux.ibm.com>
> > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > > ---
> > >
> > > Hi -- Coverity points out that there's a problem with this
> > > change (CID 1512997, 1512998):
> > >
> > > > --- a/hw/intc/pnv_xive2.c
> > > > +++ b/hw/intc/pnv_xive2.c
> > > > @@ -1662,6 +1662,8 @@ static void pnv_xive2_tm_write(void *opaque, hwaddr 
> > > > offset,
> > > >       bool gen1_tima_os =
> > > >           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
> > > >
> > > > +    offset &= TM_ADDRESS_MASK;
> > >
> > > Here we now mask off most of the bytes of 'offset',
> > > because TM_ADDRESS_MASK is 0xC3F...
> > >
> > > > +
> > > >       /* TODO: should we switch the TM ops table instead ? */
> > > >       if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
> > >
> > > ...but here we compare offset against HV_PUSH_OS_CTX_OFFSET,
> > > which is defined as
> > > #define HV_PUSH_OS_CTX_OFFSET  (HV_PAGE_OFFSET | (TM_QW1_OS + TM_WORD2))
> > > and since
> > > #define HV_PAGE_OFFSET         (XIVE_TM_HV_PAGE << TM_SHIFT)
> > > #define XIVE_TM_HV_PAGE         0x1
> > > #define TM_SHIFT                16
> > >
> > > that means HV_PUSH_OS_CTX_OFFSET has bits defined in the
> > > upper 16 bits, and the comparison can now never be true,
> > > making the if() dead code.
> > >
> > > >           xive2_tm_push_os_ctx(xptr, tctx, offset, value, size);
> > > > @@ -1681,6 +1683,8 @@ static uint64_t pnv_xive2_tm_read(void *opaque, hwaddr 
> > > > offset, unsigned size)
> > > >       bool gen1_tima_os =
> > > >           xive->cq_regs[CQ_XIVE_CFG >> 3] & CQ_XIVE_CFG_GEN1_TIMA_OS;
> > > >
> > > > +    offset &= TM_ADDRESS_MASK;
> > > > +
> > > >       /* TODO: should we switch the TM ops table instead ? */
> > > >       if (!gen1_tima_os && offset == HV_PULL_OS_CTX_OFFSET) {
> > > >           return xive2_tm_pull_os_ctx(xptr, tctx, offset, size);
> > >
> > > Similarly here.
> >
> >
> > yes. I think this went unnoticed because the push/pull os context
> > commands are only used by the HV when a vCPU is dipatched on a HW
> > thread. We would need a test for a KVM guest running under the QEMU
> > PowerNV POWER10 machine. This requires an image with some tuning
> > because emulation is a bit slow. I use to have a buildroot image
> > including a qemu and smaller buildroot image for it.
>
>
> Working on a fix. It's true that I hadn't run a guest within the powernv 
> machine for quite a while. I'm dusting off my buildroot repo to test it this 
> time...


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.

The weird part would be to include "xive2.h" in "xive.c" to get the
definitions of xive2_tm_push/pull_os_ctx. I guess that's ok.

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

Thanks,


C.