[Skiboot] [PATCH] Add in new OPAL call to flush the L2 and L3 caches.

Alexey Kardashevskiy aik at ozlabs.ru
Fri Nov 2 14:14:57 AEDT 2018



On 02/11/2018 13:48, Rashmica Gupta wrote:
> On Fri, 2018-11-02 at 13:17 +1100, Alexey Kardashevskiy wrote:
>>
>> On 29/10/2018 17:19, Rashmica Gupta wrote:
>>> Using the dcbf instruction is really slow... This is much faster.
>>>
>>> Suggested-by: Alistair Popple <alistair at popple.id.au>
>>> Signed-off-by: Rashmica Gupta <rashmica.g at gmail.com>
>>> ---
>>> Stewart: I realise that cpu.c is probably not where this should
>>> live...
>>> Thoughts on where it should go?
>>
>> I cannot really see any better place for this though.
>>
>>
>>>
>>> Context: When resetting a GPU we want to make sure all dirty cache
>>> lines
>>> in the CPU cache are cleared. Hit up Alexey and Alistair for the
>>> nitty
>>> gritty details.
>>>
>>>  core/cpu.c         | 108
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>  include/cpu.h      |   2 +
>>>  include/opal-api.h |   3 +-
>>>  3 files changed, 112 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/core/cpu.c b/core/cpu.c
>>> index 4f518a4c..bc4fcaf8 100644
>>> --- a/core/cpu.c
>>> +++ b/core/cpu.c
>>> @@ -1630,3 +1630,111 @@ static int64_t opal_nmmu_set_ptcr(uint64_t
>>> chip_id, uint64_t ptcr)
>>>  	return rc;
>>>  }
>>>  opal_call(OPAL_NMMU_SET_PTCR, opal_nmmu_set_ptcr, 2);
>>> +
>>> +
>>
>> One empty line should be fine :)
>>
>>> +#define L2_PRD_PURGE_CMD_REG 0x1080E
>>> +#define L2_PRD_PURGE_CMD_REG_BUSY 0x0040000000000000
>>> +#define L2_PRD_PURGE_CMD_TRIGGER 0x8000000000000000
>>> +#define L3_PRD_PURGE_REG 0x1180E
>>> +#define L3_PRD_PURGE_REQ 0x8000000000000000
>>
>> My guess is that these should go to include/xscom-p9-regs.h, where
>> other
>> registers used with XSCOM_ADDR_P9_EX live. Also, the values written
>> to
>> the registers are better be defined with PPC_BIT, just like
>> everything
>> else in that file, it makes it easier to match the numbers with
>> scoms/ex_chiplet.html
>>
> 
> Good point :)
> 
>>
>>> +#define TIMEOUT_MS 2
>>> +
>>> +static inline bool time_expired(unsigned long start)
>>> +{
>>> +	unsigned long time = tb_to_msecs(mftb());
>>> +
>>> +	if (time - start > TIMEOUT_MS) {
>>> +		return true;
>>> +	}
>>
>>
>> Does Stewart^wskiboot require curly braces for single line
>> statements?
>> Also, is there 80 chars requirement (just curious)?
>>
> I'm not sure... but I think I sent the wrong version - I had tidied up
> the lines to 80 chars and was returning rc (which you mentioned below).
> 
>>
>>> +	return false;
>>> +}
>>> +
>>> +static int flush_l2_caches(uint32_t chip_id, uint32_t core_id)
>>> +{
>>> +	int rc, timeout = 0;
>>> +	unsigned long start_time;
>>> +	uint64_t val = L2_PRD_PURGE_CMD_REG_BUSY;
>>> +	uint64_t addr = XSCOM_ADDR_P9_EX(core_id,
>>> L2_PRD_PURGE_CMD_REG);
>>> +
>>> +	rc = xscom_write_mask(chip_id, addr, L2_PRD_PURGE_CMD_TRIGGER,
>>> +			      L2_PRD_PURGE_CMD_TRIGGER);
>>
>> The advise from the hw folks was:
>>
>>  putspy pu.ex EXP.L2.L2MISC.L2CERRS.PRD_PURGE_CMD_TYPE L2CAC_FLUSH
>> -all
>>
>>  putspy pu.ex EXP.L2.L2MISC.L2CERRS.PRD_PURGE_CMD_TRIGGER ON -all
>>
>> I can see trigger action but not L2CAC_FLUSH.
>>
> 
> I lumped them together (as L2CAC_FLUSH=>0b0000). I'll split it up and
> make it clearer though.


Ah, my bad, I was reading it as 0x0b0000 and was looking for 'b'
:)Anyway, useful to see them defined somewhere. Also, can it be a single
write to the register (looks like it can and then there is no need to
separation) or should it be 2 writes as the hw folks strangely suggested?

When you post another version, please include those 'putspy' commands to
the commit log for the reference.


> 
>>> +	if (rc) {
>>> +		prlog(PR_ERR, "FLUSH L2 on core 0x%x: XSCOM write_mask
>>> failed %i\n", core_id, rc);
>>
>> Any reason not to "return rc" right here?
> 
> Not that I can think of. 
> 
>>
>>> +	}
>>> +	start_time = tb_to_msecs(mftb());
>>> +	while ((val & L2_PRD_PURGE_CMD_REG_BUSY) && !(timeout =
>>> time_expired(start_time))) {
>>
>>
>> Can we please not have '=' in a condition?
>>
> 
> That will disapear with Al's suggestion.
> 
>>
>>> +		rc = xscom_read(chip_id, addr, &val);
>>> +		if (rc) {
>>> +			prlog(PR_ERR, "FLUSH L2 on core 0x%x: XSCOM
>>> read failed %i\n", core_id, rc);
>>> +			break;
>>> +		}
>>
>>
>> if (tb_to_msecs(mftb()) - start_time > TIMEOUT_MS) {
>> 	prlog(PR_ERR, "FLUSH L3 on core 0x%x timed out %i\n", core_id,
>> rc);
>> 	return OPAL_BUSY;
>> }
>>
>> ...
>>
>>
>>> +	}
>>> +	if (timeout) {
>>> +		prlog(PR_ERR, "FLUSH L3 on core 0x%x timed out %i\n",
>>> core_id, rc);
>>> +		return OPAL_BUSY;
>>> +	}
>>
>> ... and ditch this check. 
> 
> Why? You don't care if we timed out trying to clear the caches?

I meant if you check-and-return for timeout inside the loop without any
extra @timeout variable, you won't need this one.



> 
>> And the time_expired() helper (which does not
>> seem to help much anyway). And the local @timeout variable. And '='
>> in
>> the condition :)
>>
>>
>>> +
>>> +	/* 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, "FLUSH L2 on core 0x%x: XSCOM write
>>> failed %i\n", core_id, rc);
>>> +	return 0;
>>
>>
>> Why not return rc?
>>
> 
> As mentioned above
> 
>>
>>> +
>>> +}
>>> +
>>> +static int flush_l3_caches(uint32_t chip_id, uint32_t core_id)
>>> +{
>>> +	int rc, timeout = 0;
>>> +	unsigned long start_time;
>>> +	uint64_t val = L3_PRD_PURGE_REQ;
>>> +	uint64_t addr = XSCOM_ADDR_P9_EX(core_id, L3_PRD_PURGE_REG);
>>> +
>>> +	rc = xscom_write_mask(chip_id, addr, L3_PRD_PURGE_REQ,
>>> L3_PRD_PURGE_REQ);
>>
>> Similar to L2CAC_FLUSH question here:
>>
>>  putspy pu.ex EXP.L3.L3_MISC.L3CERRS.L3_PRD_PURGE_TTYPE FULL_PURGE
>> -all
>>
>>  putspy pu.ex EXP.L3.L3_MISC.L3CERRS.L3_PRD_PURGE_REQ ON -all
>>
>>
>> There is L3_PRD_PURGE_REQ but no FULL_PURGE.
>>
> 
> Same as above, I combined them as FULL_PURGE=>0b0000. Will seperate and
> make clearer :)
> 
>>
>>> +	if (rc) {
>>> +		prlog(PR_ERR, "FLUSH L3 on core 0x%x: XSCOM write_mask
>>> failed %i\n", core_id, rc);
>>
>>
>> return rc?
>>
>>> +	}
>>> +
>>> +	/* Trigger bit is automatically set to zero when flushing is
>>> done*/
>>> +	start_time = tb_to_msecs(mftb());
>>> +	while ((val & L3_PRD_PURGE_REQ) && !(timeout =
>>> time_expired(start_time) )) {
>>> +		rc = xscom_read(chip_id, addr, &val);
>>> +		if (rc) {
>>> +			prlog(PR_ERR, "FLUSH L3 on core 0x%x: XSCOM
>>> read failed %i\n", core_id, rc);
>>> +			break;
>>> +		}
>>> +	}
>>> +	if (timeout) {
>>> +		prlog(PR_ERR, "FLUSH L3 on core 0x%x timed out %i\n",
>>> core_id, rc);
>>> +		return OPAL_BUSY;
>>> +	}
>>> +
>>> +	return 0;
>>
>> return rc?
>>
>>
>>> +}
>>> +
>>> +int flush_caches(void)
>>
>> OPAL calls return int64_t. And the handlers are static so there is no
>> reason to export the symbol via include/cpu.h below.
>>
> 
> Ok.
> 
>>
>>> +{
>>> +	int rc = 0;
>>> +	struct cpu_thread *t;
>>> +	uint64_t chip_id, core_id, prev_core_id = 0xdeadbeef;
>>> +
>>> +	if ((mfspr(SPR_PVR) & PVR_TYPE_P9) != PVR_TYPE_P9)
>>> +		return OPAL_UNSUPPORTED;
>>> +
>>> +	for_each_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;
>>> +		chip_id = t->chip_id;
>>
>>
>> The chip_id local variable seems pretty useless, could just use t-
>>> chip_id.
> 
> Yep.
> 
>>
>>> +
>>> +		rc |= flush_l2_caches(chip_id, core_id);
>>> +		rc |= flush_l3_caches(chip_id, core_id);
>>> +	}
>>> +
>>> +	return rc;
>>> +}
>>> +
>>> +
>>
>> Extra empty line?
>>
>>> +opal_call(OPAL_CLEAR_CACHE, flush_caches, 0);
>>> diff --git a/include/cpu.h b/include/cpu.h
>>> index 2fe47982..04c862c5 100644
>>> --- a/include/cpu.h
>>> +++ b/include/cpu.h
>>> @@ -329,4 +329,6 @@ int dctl_set_special_wakeup(struct cpu_thread
>>> *t);
>>>  int dctl_clear_special_wakeup(struct cpu_thread *t);
>>>  int dctl_core_is_gated(struct cpu_thread *t);
>>>  
>>> +extern int flush_caches(void);
>>> +
>>>  #endif /* __CPU_H */
>>> diff --git a/include/opal-api.h b/include/opal-api.h
>>> index 5f397c8e..c24838d2 100644
>>> --- a/include/opal-api.h
>>> +++ b/include/opal-api.h
>>> @@ -226,7 +226,8 @@
>>>  #define OPAL_NX_COPROC_INIT			167
>>>  #define OPAL_NPU_SET_RELAXED_ORDER		168
>>>  #define OPAL_NPU_GET_RELAXED_ORDER		169
>>> -#define OPAL_LAST				169
>>> +#define OPAL_CLEAR_CACHE			170
>>
>>
>> s/clear/purge/. The scoms calls it "purge", let's stick to this name.
>>
> 
> Good point :)
> 
>>
>>> +#define OPAL_LAST				170
>>>  
>>>  #define QUIESCE_HOLD			1 /* Spin all calls at
>>> entry */
>>>  #define QUIESCE_REJECT			2 /* Fail all calls
>>> with OPAL_BUSY */
>>>
>>
>>
> 

-- 
Alexey


More information about the Skiboot mailing list