diff mbox series

[for-5.0,v5,11/23] ppc/pnv: Introduce a pnv_xive_is_cpu_enabled() helper

Message ID 20191115162436.30548-12-clg@kaod.org
State New
Headers show
Series ppc/pnv: add XIVE support for KVM guests | expand

Commit Message

Cédric Le Goater Nov. 15, 2019, 4:24 p.m. UTC 
and use this helper to exclude CPUs which are not enabled in the XIVE
controller.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/pnv_xive.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Greg Kurz Nov. 20, 2019, 5:26 p.m. UTC | #1 
On Fri, 15 Nov 2019 17:24:24 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> and use this helper to exclude CPUs which are not enabled in the XIVE
> controller.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/pnv_xive.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 71ca4961b6b1..4c8c6e51c20f 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -372,6 +372,20 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>      return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
>  }
>  
> +static int cpu_pir(PowerPCCPU *cpu)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    return env->spr_cb[SPR_PIR].default_value;
> +}
> +
> +static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
> +{
> +    int pir = cpu_pir(cpu);
> +    int thrd_id = pir & 0x7f;
> +
> +    return xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(thrd_id);

A similar check is open-coded in pnv_xive_get_indirect_tctx() :

    /* Check that HW thread is XIVE enabled */
    if (!(xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(pir & 0x3f))) {
        xive_error(xive, "IC: CPU %x is not enabled", pir);
    }

The thread id is only the 6 lower bits of the PIR there, and so seems to
indicate the skiboot sources:

        /* Get bit in register */
        bit = c->pir & 0x3f;

Why make it pir & 0x7f here ? If it should actually be 0x3f, maybe also use
the helper in pnv_xive_get_indirect_tctx().

> +}
> +
>  static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>                                uint8_t nvt_blk, uint32_t nvt_idx,
>                                bool cam_ignore, uint8_t priority,
> @@ -393,6 +407,10 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>              XiveTCTX *tctx;
>              int ring;
>  
> +            if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
> +                continue;
> +            }
> +
>              tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
>  
>              /*
Cédric Le Goater Nov. 20, 2019, 9:40 p.m. UTC | #2 
On 20/11/2019 18:26, Greg Kurz wrote:
> On Fri, 15 Nov 2019 17:24:24 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> and use this helper to exclude CPUs which are not enabled in the XIVE
>> controller.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/intc/pnv_xive.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
>> index 71ca4961b6b1..4c8c6e51c20f 100644
>> --- a/hw/intc/pnv_xive.c
>> +++ b/hw/intc/pnv_xive.c
>> @@ -372,6 +372,20 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>>      return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
>>  }
>>  
>> +static int cpu_pir(PowerPCCPU *cpu)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    return env->spr_cb[SPR_PIR].default_value;
>> +}
>> +
>> +static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
>> +{
>> +    int pir = cpu_pir(cpu);
>> +    int thrd_id = pir & 0x7f;
>> +
>> +    return xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(thrd_id);
> 
> A similar check is open-coded in pnv_xive_get_indirect_tctx() :
> 
>     /* Check that HW thread is XIVE enabled */
>     if (!(xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(pir & 0x3f))) {
>         xive_error(xive, "IC: CPU %x is not enabled", pir);
>     }
> 
> The thread id is only the 6 lower bits of the PIR there, and so seems to
> indicate the skiboot sources:
> 
>         /* Get bit in register */
>         bit = c->pir & 0x3f;

skiboot uses 0x3f when enabling the TCTXT of a CPU because register
INT_TCTXT_EN0 covers cores 0-15 (normal) and 0-7 (fused) and 
register INT_TCTXT_EN1 covers cores 16-23 (normal) and 8-11 (fused). 
The encoding in the registers is a bit different.

> Why make it pir & 0x7f here ? 

See pnv_chip_core_pir_p9 comments for some details on the CPU ID 
layout.

> If it should actually be 0x3f, 
but yes, we should fix the mask in the register setting. 

> maybe also use the helper in pnv_xive_get_indirect_tctx().

This is getting changed later on. So I rather not.

C.

> 
>> +}
>> +
>>  static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>>                                uint8_t nvt_blk, uint32_t nvt_idx,
>>                                bool cam_ignore, uint8_t priority,
>> @@ -393,6 +407,10 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>>              XiveTCTX *tctx;
>>              int ring;
>>  
>> +            if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
>> +                continue;
>> +            }
>> +
>>              tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
>>  
>>              /*
>
Greg Kurz Nov. 21, 2019, 7:58 a.m. UTC | #3 
On Wed, 20 Nov 2019 22:40:31 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 20/11/2019 18:26, Greg Kurz wrote:
> > On Fri, 15 Nov 2019 17:24:24 +0100
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> and use this helper to exclude CPUs which are not enabled in the XIVE
> >> controller.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/intc/pnv_xive.c | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> >> index 71ca4961b6b1..4c8c6e51c20f 100644
> >> --- a/hw/intc/pnv_xive.c
> >> +++ b/hw/intc/pnv_xive.c
> >> @@ -372,6 +372,20 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
> >>      return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
> >>  }
> >>  
> >> +static int cpu_pir(PowerPCCPU *cpu)
> >> +{
> >> +    CPUPPCState *env = &cpu->env;
> >> +    return env->spr_cb[SPR_PIR].default_value;
> >> +}
> >> +
> >> +static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
> >> +{
> >> +    int pir = cpu_pir(cpu);
> >> +    int thrd_id = pir & 0x7f;
> >> +
> >> +    return xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(thrd_id);
> > 
> > A similar check is open-coded in pnv_xive_get_indirect_tctx() :
> > 
> >     /* Check that HW thread is XIVE enabled */
> >     if (!(xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(pir & 0x3f))) {
> >         xive_error(xive, "IC: CPU %x is not enabled", pir);
> >     }
> > 
> > The thread id is only the 6 lower bits of the PIR there, and so seems to
> > indicate the skiboot sources:
> > 
> >         /* Get bit in register */
> >         bit = c->pir & 0x3f;
> 
> skiboot uses 0x3f when enabling the TCTXT of a CPU because register
> INT_TCTXT_EN0 covers cores 0-15 (normal) and 0-7 (fused) and 
> register INT_TCTXT_EN1 covers cores 16-23 (normal) and 8-11 (fused). 
> The encoding in the registers is a bit different.
> 
> > Why make it pir & 0x7f here ? 
> 
> See pnv_chip_core_pir_p9 comments for some details on the CPU ID 
> layout.
> 

*   57:61  Core number
*   62:63  Thread ID

Ok, so the CPU ID within the socket is 7 bits, ie. pir & 0x7f

> > If it should actually be 0x3f, 
> but yes, we should fix the mask in the register setting. 
> 
> > maybe also use the helper in pnv_xive_get_indirect_tctx().
> 
> This is getting changed later on. So I rather not.
> 

I don't see any later change there, neither in this series, nor
in your powernv-4.2 on github, but nevermind, this patch is
good enough for the purpose of CAM line matching.

Reviewed-by: Greg Kurz <groug@kaod.org>

> C.
> 
> > 
> >> +}
> >> +
> >>  static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
> >>                                uint8_t nvt_blk, uint32_t nvt_idx,
> >>                                bool cam_ignore, uint8_t priority,
> >> @@ -393,6 +407,10 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
> >>              XiveTCTX *tctx;
> >>              int ring;
> >>  
> >> +            if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
> >> +                continue;
> >> +            }
> >> +
> >>              tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
> >>  
> >>              /*
> > 
>
Cédric Le Goater Nov. 21, 2019, 9:16 a.m. UTC | #4 
On 21/11/2019 08:58, Greg Kurz wrote:
> On Wed, 20 Nov 2019 22:40:31 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 20/11/2019 18:26, Greg Kurz wrote:
>>> On Fri, 15 Nov 2019 17:24:24 +0100
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> and use this helper to exclude CPUs which are not enabled in the XIVE
>>>> controller.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  hw/intc/pnv_xive.c | 18 ++++++++++++++++++
>>>>  1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
>>>> index 71ca4961b6b1..4c8c6e51c20f 100644
>>>> --- a/hw/intc/pnv_xive.c
>>>> +++ b/hw/intc/pnv_xive.c
>>>> @@ -372,6 +372,20 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>>>>      return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
>>>>  }
>>>>  
>>>> +static int cpu_pir(PowerPCCPU *cpu)
>>>> +{
>>>> +    CPUPPCState *env = &cpu->env;
>>>> +    return env->spr_cb[SPR_PIR].default_value;
>>>> +}
>>>> +
>>>> +static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
>>>> +{
>>>> +    int pir = cpu_pir(cpu);
>>>> +    int thrd_id = pir & 0x7f;
>>>> +
>>>> +    return xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(thrd_id);
>>>
>>> A similar check is open-coded in pnv_xive_get_indirect_tctx() :
>>>
>>>     /* Check that HW thread is XIVE enabled */
>>>     if (!(xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(pir & 0x3f))) {
>>>         xive_error(xive, "IC: CPU %x is not enabled", pir);
>>>     }
>>>
>>> The thread id is only the 6 lower bits of the PIR there, and so seems to
>>> indicate the skiboot sources:
>>>
>>>         /* Get bit in register */
>>>         bit = c->pir & 0x3f;
>>
>> skiboot uses 0x3f when enabling the TCTXT of a CPU because register
>> INT_TCTXT_EN0 covers cores 0-15 (normal) and 0-7 (fused) and 
>> register INT_TCTXT_EN1 covers cores 16-23 (normal) and 8-11 (fused). 
>> The encoding in the registers is a bit different.
>>
>>> Why make it pir & 0x7f here ? 
>>
>> See pnv_chip_core_pir_p9 comments for some details on the CPU ID 
>> layout.
>>
> 
> *   57:61  Core number
> *   62:63  Thread ID
> 
> Ok, so the CPU ID within the socket is 7 bits, ie. pir & 0x7f
> 
>>> If it should actually be 0x3f, 
>> but yes, we should fix the mask in the register setting. 
>>
>>> maybe also use the helper in pnv_xive_get_indirect_tctx().
>>
>> This is getting changed later on. So I rather not.
>>
> 
> I don't see any later change there, neither in this series, 

Patch "ppc/pnv: Clarify how the TIMA is accessed on a multichip system"

changes a few things in that area.

> nor in your powernv-4.2 on github, but nevermind, this patch is
> good enough for the purpose of CAM line matching.

Yes. It could be better though and it's a localized change. 

the INT_TCTXT_EN0 (PC_THREAD_EN_REG0) needs a small fix on the mask. 
We don't use the EN1 register also for cores > 15. 

It works today because we don't start the machine with all the 
possible cores. Although it should be possible to start a QEMU 
PowerNV machine with 24 cores on each socket. 


I would like to have stricter checks on CAM line accesses because
it is an OS interface. The INT_TCTXT_EN0 (PC_THREAD_EN_REG0) is 
the first level (HW) but we need to check also the 'V' bit of 
each ring. That's more complex. For later.


C. 


> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
>> C.
>>
>>>
>>>> +}
>>>> +
>>>>  static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>>>>                                uint8_t nvt_blk, uint32_t nvt_idx,
>>>>                                bool cam_ignore, uint8_t priority,
>>>> @@ -393,6 +407,10 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>>>>              XiveTCTX *tctx;
>>>>              int ring;
>>>>  
>>>> +            if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
>>>> +                continue;
>>>> +            }
>>>> +
>>>>              tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
>>>>  
>>>>              /*
>>>
>>
>
Greg Kurz Nov. 21, 2019, 9:49 a.m. UTC | #5 
On Thu, 21 Nov 2019 10:16:14 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 21/11/2019 08:58, Greg Kurz wrote:
> > On Wed, 20 Nov 2019 22:40:31 +0100
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> On 20/11/2019 18:26, Greg Kurz wrote:
> >>> On Fri, 15 Nov 2019 17:24:24 +0100
> >>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>
> >>>> and use this helper to exclude CPUs which are not enabled in the XIVE
> >>>> controller.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>> ---
> >>>>  hw/intc/pnv_xive.c | 18 ++++++++++++++++++
> >>>>  1 file changed, 18 insertions(+)
> >>>>
> >>>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> >>>> index 71ca4961b6b1..4c8c6e51c20f 100644
> >>>> --- a/hw/intc/pnv_xive.c
> >>>> +++ b/hw/intc/pnv_xive.c
> >>>> @@ -372,6 +372,20 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
> >>>>      return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
> >>>>  }
> >>>>  
> >>>> +static int cpu_pir(PowerPCCPU *cpu)
> >>>> +{
> >>>> +    CPUPPCState *env = &cpu->env;
> >>>> +    return env->spr_cb[SPR_PIR].default_value;
> >>>> +}
> >>>> +
> >>>> +static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
> >>>> +{
> >>>> +    int pir = cpu_pir(cpu);
> >>>> +    int thrd_id = pir & 0x7f;
> >>>> +
> >>>> +    return xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(thrd_id);
> >>>
> >>> A similar check is open-coded in pnv_xive_get_indirect_tctx() :
> >>>
> >>>     /* Check that HW thread is XIVE enabled */
> >>>     if (!(xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(pir & 0x3f))) {
> >>>         xive_error(xive, "IC: CPU %x is not enabled", pir);
> >>>     }
> >>>
> >>> The thread id is only the 6 lower bits of the PIR there, and so seems to
> >>> indicate the skiboot sources:
> >>>
> >>>         /* Get bit in register */
> >>>         bit = c->pir & 0x3f;
> >>
> >> skiboot uses 0x3f when enabling the TCTXT of a CPU because register
> >> INT_TCTXT_EN0 covers cores 0-15 (normal) and 0-7 (fused) and 
> >> register INT_TCTXT_EN1 covers cores 16-23 (normal) and 8-11 (fused). 
> >> The encoding in the registers is a bit different.
> >>
> >>> Why make it pir & 0x7f here ? 
> >>
> >> See pnv_chip_core_pir_p9 comments for some details on the CPU ID 
> >> layout.
> >>
> > 
> > *   57:61  Core number
> > *   62:63  Thread ID
> > 
> > Ok, so the CPU ID within the socket is 7 bits, ie. pir & 0x7f
> > 
> >>> If it should actually be 0x3f, 
> >> but yes, we should fix the mask in the register setting. 
> >>
> >>> maybe also use the helper in pnv_xive_get_indirect_tctx().
> >>
> >> This is getting changed later on. So I rather not.
> >>
> > 
> > I don't see any later change there, neither in this series, 
> 
> Patch "ppc/pnv: Clarify how the TIMA is accessed on a multichip system"
> 
> changes a few things in that area.
> 

No, it changes pnv_xive_get_tctx() which gets removed later by

"[PATCH for-5.0 v5 19/23] ppc/xive: Remove the get_tctx() XiveRouter handler"

My remark was about pnv_xive_get_indirect_tctx(), but nevermind :)

> > nor in your powernv-4.2 on github, but nevermind, this patch is
> > good enough for the purpose of CAM line matching.
> 
> Yes. It could be better though and it's a localized change. 
> 
> the INT_TCTXT_EN0 (PC_THREAD_EN_REG0) needs a small fix on the mask. 
> We don't use the EN1 register also for cores > 15. 
> 
> It works today because we don't start the machine with all the 
> possible cores. Although it should be possible to start a QEMU 
> PowerNV machine with 24 cores on each socket. 
> 
> 
> I would like to have stricter checks on CAM line accesses because
> it is an OS interface. The INT_TCTXT_EN0 (PC_THREAD_EN_REG0) is 
> the first level (HW) but we need to check also the 'V' bit of 
> each ring. That's more complex. For later.
> 
> 
> C. 
> 
> 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> >> C.
> >>
> >>>
> >>>> +}
> >>>> +
> >>>>  static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
> >>>>                                uint8_t nvt_blk, uint32_t nvt_idx,
> >>>>                                bool cam_ignore, uint8_t priority,
> >>>> @@ -393,6 +407,10 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
> >>>>              XiveTCTX *tctx;
> >>>>              int ring;
> >>>>  
> >>>> +            if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
> >>>> +                continue;
> >>>> +            }
> >>>> +
> >>>>              tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
> >>>>  
> >>>>              /*
> >>>
> >>
> > 
>
diff mbox series

Patch

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 71ca4961b6b1..4c8c6e51c20f 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -372,6 +372,20 @@  static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
     return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
 }
 
+static int cpu_pir(PowerPCCPU *cpu)
+{
+    CPUPPCState *env = &cpu->env;
+    return env->spr_cb[SPR_PIR].default_value;
+}
+
+static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
+{
+    int pir = cpu_pir(cpu);
+    int thrd_id = pir & 0x7f;
+
+    return xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(thrd_id);
+}
+
 static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
                               uint8_t nvt_blk, uint32_t nvt_idx,
                               bool cam_ignore, uint8_t priority,
@@ -393,6 +407,10 @@  static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
             XiveTCTX *tctx;
             int ring;
 
+            if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
+                continue;
+            }
+
             tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
 
             /*