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

[Peter Maydell]

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.

thanks
-- PMM