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

Commit Message

Daniel Henrique Barboza June 10, 2023, 1:31 p.m. UTC

From: Frederic Barrat <[e-mail]>

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 <[e-mail]>
Reviewed-by: Cédric Le Goater <[e-mail]>
Message-Id: <[e-mail]>
Signed-off-by: Daniel Henrique Barboza <[e-mail]>
---
 hw/intc/pnv_xive2.c | 4 ++++
 hw/intc/xive.c      | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Peter Maydell June 20, 2023, 10:45 a.m. UTC | #1

On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
<[e-mail]> wrote:
>
> From: Frederic Barrat <[e-mail]>
>
> 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 <[e-mail]>
> Reviewed-by: Cédric Le Goater <[e-mail]>
> Message-Id: <[e-mail]>
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> ---

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

Cédric Le Goater June 20, 2023, 11:20 a.m. UTC | #2

On 6/20/23 12:45, Peter Maydell wrote:
> On Sat, 10 Jun 2023 at 14:31, Daniel Henrique Barboza
> <[e-mail]> wrote:
>>
>> From: Frederic Barrat <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Cédric Le Goater <[e-mail]>
>> Message-Id: <[e-mail]>
>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>> ---
> 
> 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.

So, offset is within the full TIMA region (4 pages) and
TM_ADDRESS_MASK is a mask within a page. This needs a fix.

Thanks,

C.

Frederic Barrat June 20, 2023, 2:31 p.m. UTC | #3

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
>> <[e-mail]> wrote:
>>>
>>> From: Frederic Barrat <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Reviewed-by: Cédric Le Goater <[e-mail]>
>>> Message-Id: <[e-mail]>
>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>>> ---
>>
>> 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...

   Fred


> 
> So, offset is within the full TIMA region (4 pages) and
> TM_ADDRESS_MASK is a mask within a page. This needs a fix.
> 
> Thanks,
> 
> C.
>

Cédric Le Goater June 20, 2023, 2:57 p.m. UTC | #4

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
>>> <[e-mail]> wrote:
>>>>
>>>> From: Frederic Barrat <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Reviewed-by: Cédric Le Goater <[e-mail]>
>>>> Message-Id: <[e-mail]>
>>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>>>> ---
>>>
>>> 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...

I wonder if we could host the image in some place, since there is a need
for "nested"  guest testing. QEMU PPC supports at least 2 implementations :

  * PowerNV / KVM-on-pseries v1
  * KVM-on-pseries v1 / pseries

and yet to come :

  * KVM-on-pseries v2 (pHyp) / pseries

C.


> 
>    Fred
> 
> 
>>
>> So, offset is within the full TIMA region (4 pages) and
>> TM_ADDRESS_MASK is a mask within a page. This needs a fix.
>>
>> Thanks,
>>
>> C.
>>

Cédric Le Goater June 21, 2023, 7:18 a.m. UTC | #5

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
>>> <[e-mail]> wrote:
>>>>
>>>> From: Frederic Barrat <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Reviewed-by: Cédric Le Goater <[e-mail]>
>>>> Message-Id: <[e-mail]>
>>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>>>> ---
>>>
>>> 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.

Frederic Barrat June 21, 2023, 3:18 p.m. UTC | #6

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

Cédric Le Goater June 21, 2023, 4:59 p.m. UTC | #7

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

Patch

diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index 132f82a035..e5a028c1e6 100644
--- 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;
+
     /* TODO: should we switch the TM ops table instead ? */
     if (!gen1_tima_os && offset == HV_PUSH_OS_CTX_OFFSET) {
         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);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index ebe399bc09..5204c14b87 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -500,7 +500,7 @@  static const XiveTmOp xive_tm_operations[] = {
 static const XiveTmOp *xive_tm_find_op(hwaddr offset, unsigned size, bool write)
 {
     uint8_t page_offset = (offset >> TM_SHIFT) & 0x3;
-    uint32_t op_offset = offset & 0xFFF;
+    uint32_t op_offset = offset & TM_ADDRESS_MASK;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(xive_tm_operations); i++) {