On 21/11/2019 08:58, Greg Kurz wrote:
> On Wed, 20 Nov 2019 22:40:31 +0100
> Cédric Le Goater <c...@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 <c...@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 <c...@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 <gr...@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);
>>>>
>>>> /*
>>>
>>
>
