[Skiboot] [PATCH v2] Add purging CPU L2 and L3 caches into NPU hreset.
Andrew Donnellan
andrew.donnellan at au1.ibm.com
Fri Nov 30 17:16:36 AEDT 2018
On 30/11/18 12:05 pm, Alexey Kardashevskiy wrote:>> +static int
start_l2_purge(uint32_t chip_id, uint32_t core_id)
>> +{
>> + int rc;
>> + uint64_t addr = XSCOM_ADDR_P9_EX(core_id, L2_PRD_PURGE_CMD_REG);
>> +
>> + rc = xscom_write_mask(chip_id, addr, L2CAC_FLUSH,
>> + L2_PRD_PURGE_CMD_TYPE_MASK);
>> + if (rc)
>> + goto out;
>> + rc = xscom_write_mask(chip_id, addr, L2_PRD_PURGE_CMD_TRIGGER,
>> + L2_PRD_PURGE_CMD_TRIGGER);
>> + if (rc)
>> +out: prlog(PR_ERR, "PURGE L2 on core 0x%x: XSCOM write_mask "
>> + "failed %i\n", core_id, rc);
>
>
> Nit: I find such use of goto with a label inside another scope rather
> confusing. Goto is usually used to avoid code duplication on a cleanup
> path and this is not the case.
>
> I'd rather ditch "goto" and do "if (!rc) xscom_write_mask" for the
> second xscom_write_mask() or move "out:" 1 line higher. Up to
> Stewart really :)
Just handle the second one exactly like the first one.
+ if (rc)
+ goto out;
+ rc = xscom_write_mask(chip_id, addr, L3_PRD_PURGE_REQ,
+ L3_PRD_PURGE_REQ);
+ if (rc)
+ goto out;
+
+ return rc;
+
+out:
+ prlog(PR_ERR, "PURGE L3 on core 0x%x: XSCOM write_mask "
+ "failed %i\n", core_id, rc);
+ return rc;
>
>
>> + return rc;
>> +
>
> Unnecessary empty line.
>
>> +}
>> +
>> +static int wait_l2_purge(uint32_t chip_id, uint32_t core_id)
>> +{
>> + int rc;
>> + unsigned long now = mftb();
>> + unsigned long end = now + msecs_to_tb(2);
>> + uint64_t val = L2_PRD_PURGE_CMD_REG_BUSY;
>> + uint64_t addr = XSCOM_ADDR_P9_EX(core_id, L2_PRD_PURGE_CMD_REG);
>> +
>> + while (val & L2_PRD_PURGE_CMD_REG_BUSY) {
>
> for (val = L2_PRD_PURGE_CMD_REG_BUSY;;)
If the concern is an unnecessary initialiser then use do/while? Better
than using a for.
>
> and....
>
>> + rc = xscom_read(chip_id, addr, &val);
>> + if (rc) {
>> + prlog(PR_ERR, "PURGE L2 on core 0x%x: XSCOM read "
>> + "failed %i\n", core_id, rc);
>> + break;
>> + }
>
>
> ... break out here:
> if (val & L2_PRD_PURGE_CMD_REG_BUSY)
> break;
>
> As if you read val with the bit set, then checking timeout is rather
> pointless, no? >
>
>> + now = mftb();
>> + if (tb_compare(now, end) == TB_AAFTERB) {
>> + prlog(PR_ERR, "PURGE L2 on core 0x%x timed out %i\n",
>> + core_id, rc);
>> + return OPAL_BUSY;
>> + }
>> + }
>> +
>> + /* We have to clear the trigger bit ourselves */
>> + val &= ~L2_PRD_PURGE_CMD_TRIGGER;
>> + rc = xscom_write(chip_id, addr, val);
>> + if (rc)
>> + prlog(PR_ERR, "PURGE L2 on core 0x%x: XSCOM write failed %i\n",
>> + core_id, rc);
>> + return rc;
>> +
>
> Unnecessary empty line.
>
>
>> +}
>> +
>> +static int start_l3_purge(uint32_t chip_id, uint32_t core_id)
>> +{
>> + int rc = 0;
>> + uint64_t addr = XSCOM_ADDR_P9_EX(core_id, L3_PRD_PURGE_REG);
>> +
>> + rc = xscom_write_mask(chip_id, addr, L3_FULL_PURGE,
>> + L3_PRD_PURGE_TTYPE_MASK);
>> + if (rc)
>> + goto out;
>> + rc = xscom_write_mask(chip_id, addr, L3_PRD_PURGE_REQ,
>> + L3_PRD_PURGE_REQ);
>> + if (rc)
>> +out: prlog(PR_ERR, "PURGE L3 on core 0x%x: XSCOM write_mask "
>> + "failed %i\n", core_id, rc);
>> + return rc;
>> +
>> +}
>> +
>> +static int wait_l3_purge(uint32_t chip_id, uint32_t core_id)
>> +{
>> + int rc;
>> + unsigned long now = mftb();
>> + unsigned long end = now + msecs_to_tb(2);
>> + uint64_t val = L3_PRD_PURGE_REQ;
>> + uint64_t addr = XSCOM_ADDR_P9_EX(core_id, L3_PRD_PURGE_REG);
>> +
>> + /* Trigger bit is automatically set to zero when flushing is done*/
>> + while (val & L3_PRD_PURGE_REQ) {
>> + rc = xscom_read(chip_id, addr, &val);
>> + if (rc) {
>> + prlog(PR_ERR, "PURGE L3 on core 0x%x: XSCOM read "
>> + "failed %i\n", core_id, rc);
>> + break;
>> + }
>> + now = mftb();
>> + if (tb_compare(now, end) == TB_AAFTERB) {
>> + prlog(PR_ERR, "PURGE L3 on core 0x%x timed out %i\n",
>> + core_id, rc);
>> + return OPAL_BUSY;
>> + }
>> + }
>> + return rc;
>> +}
>> +
>> +static int64_t purge_l2_l3_caches(void)
>> +{
>> + int rc = 0;
>> + struct cpu_thread *t;
>> + uint64_t core_id, prev_core_id = 0xdeadbeef;
>
> I'd make it (uint64_t)-1 :) deafbeaf is more for poisoning memory.
>
>
>> +
>> + if (proc_gen != proc_gen_p9)
>> + return OPAL_UNSUPPORTED;
>
>
> I assume this is a leftover from when it was an OPAL call. There is no
> even small chance for this to be called for anything but p9, right?
>
>
>> +
>> + for_each_ungarded_cpu(t) {
>> + /* Only need to do it once per core chiplet */
>> + core_id = pir_to_core_id(t->pir);
>> + if (prev_core_id == core_id)
>> + continue;
>> + prev_core_id = core_id;
>> + if (start_l2_purge(t->chip_id, core_id))
>> + goto out;
>> + if (start_l3_purge(t->chip_id, core_id))
>> + goto out;
>> + }
>> +
>> + for_each_ungarded_cpu(t) {
>> + /* Only need to do it once per core chiplet */
>> + core_id = pir_to_core_id(t->pir);
>> + if (prev_core_id == core_id)
>> + continue;
>
> Looks like if there is a single core at all (quite unlikely but still a
> possible situation), we won't wait below at all.
>
>> + prev_core_id = core_id;
>> +
>> + if (wait_l2_purge(t->chip_id, core_id))
>> + goto out;
>> + if (wait_l3_purge(t->chip_id, core_id))
>> + goto out;
>> + }
>> + return rc;
>
>
> return OPAL_SUCCESS;
>
>
>> +out:
>> + prlog(PR_ERR, "Failed on core: 0x%llx\n", core_id);
>> + return rc;
>
> return OPAL_BUSY_EVENT. Now it always returns 0.
>
>
>> +
>> +}
>> +
>> static int64_t npu2_hreset(struct pci_slot *slot __unused)
>> {
>> struct npu2 *p;
>> @@ -1125,6 +1260,7 @@ static int64_t npu2_hreset(struct pci_slot *slot __unused)
>> reset_ntl(ndev);
>> }
>> }
>> + purge_l2_l3_caches();
>
>
> This takes care of the guest termination case (which is good).
>
> Now in order to cover the period between the guest started rebooting and
> the guest started onlining the memory, we also need purge_l2_l3_caches()
> near npu2_dev_procedure_reset() in npu2_dev_cfg_exp_devcap() as this is
> the reset method used for NPU when the guest reboots.
>
>
>
>> return OPAL_SUCCESS;
>
>
> return purge_l2_l3_caches(), otherwise the timeout error is lost.
>
>
>
>
>> }
>>
>> diff --git a/include/npu2-regs.h b/include/npu2-regs.h
>> index 165e0b79..20a61e28 100644
>> --- a/include/npu2-regs.h
>> +++ b/include/npu2-regs.h
>> @@ -749,4 +749,15 @@ void npu2_scom_write(uint64_t gcid, uint64_t scom_base,
>> #define OB3_ODL1_TRAINING_STATUS 0xC01082F
>> #define OB_ODL_TRAINING_STATUS_STS_RX_PATTERN_B PPC_BITMASK(8, 15)
>>
>> +/* Registers and bits used to clear the L2 and L3 cache */
>> +#define L2_PRD_PURGE_CMD_REG 0x1080E
>> +#define L2_PRD_PURGE_CMD_REG_BUSY 0x0040000000000000
>> +#define L2_PRD_PURGE_CMD_TYPE_MASK PPC_BIT(1) | PPC_BIT(2) | PPC_BIT(3) | PPC_BIT(4)
>> +#define L2_PRD_PURGE_CMD_TRIGGER PPC_BIT(0)
>> +#define L2CAC_FLUSH 0x0
>> +#define L3_PRD_PURGE_REG 0x1180E
>> +#define L3_PRD_PURGE_REQ PPC_BIT(0)
>> +#define L3_PRD_PURGE_TTYPE_MASK PPC_BIT(1) | PPC_BIT(2) | PPC_BIT(3) | PPC_BIT(4)
>> +#define L3_FULL_PURGE 0x0
>> +
>> #endif /* __NPU2_REGS_H */
>>
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com IBM Australia Limited
More information about the Skiboot
mailing list