On Thu, 21 Nov 2019 10:16:14 +0100 Cédric Le Goater <c...@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 <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. > 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 <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); > >>>> > >>>> /* > >>> > >> > > >