[Skiboot] [PATCH v12 08/10] skiboot: Add opal call to enable/disable Nest IMC

[Madhavan Srinivasan]

On Wednesday 14 June 2017 10:06 AM, Michael Neuling wrote:
> On Sun, 2017-05-21 at 20:40 +0530, Madhavan Srinivasan wrote:
>> From: Anju T Sudhakar <anju at linux.vnet.ibm.com>
>>
>> Add new opal calls to start, stop the Nest IMC microcode running in the
>> OCC complex. Also, check the status from the control block structure before
>> starting/stopping the IMC microcode.
>>
>> This patch also add an opal call to init the counters. But for Nest IMC this
>> function is a no-op currently.
>>
>> Signed-off-by: Hemant Kumar <hemant at linux.vnet.ibm.com>
>> Signed-off-by: Anju T Sudhakar <anju at linux.vnet.ibm.com>
>> Signed-off-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
>> ---
>>   hw/imc.c           | 77
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/opal-api.h | 11 +++++++-
>>   2 files changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/imc.c b/hw/imc.c
>> index 00e19154d641..d56b3eb251b2 100644
>> --- a/hw/imc.c
>> +++ b/hw/imc.c
>> @@ -336,3 +336,80 @@ err:
>>   	prerror("IMC Devices not added\n");
>>   	free(imc_catalog_buf);
>>   }
>> +
>> +/*
>> + * opal_imc_counters_init : This call initialize the IMC engine.
>> + *
>> + * For Nest IMC, this is no-op and returns OPAL_SUCCESS at this point.
>> + */
>> +static int64_t opal_imc_counters_init(uint32_t type)
>> +{
>> +	return OPAL_SUCCESS;
>> +}
> Why don't we do this on in imc_init()?  Why do we rely on the OS doing this?

Yes. From the nest imc point of view, we dont have any initialization
today. But incase of core imc, we do have initialization procedure.
That is, we need to update couple of scoms with a static value and
one scom called "PDBAR" (scom spec did not expand what is PDBAR
but will dig more and update it here) needs to be initialized with
a memory location (where the core imc counter data will be posted
per-the core logic). And this is a per-core resource.

Now, not doing this memory location by OPAL will avoid a
complicated device tree node update from OPAL to pass this
information to kernel. Secondly size request for the core imc
is 8K (per core) and in a 2 socket or more we will increase
the opal reserve memory area alot.

Now incase of doing it in kernel, at the kexec scenario, we
first stop the core imc engine via OPAL_IMC_COUNTER_STOP
and then we free the memory. And this is primarily done
in the powernv device shutdown path.

And in the powernv device probe path we have a check
for the kdump kernel, so we dont load or create IMC devices
and hence will not do any OPAL_IMC* calls.

>
> Anyway... as mentioned before, this alone doesn't compile.

Yes i will merge the patch 8 and 9 as mentioned before.

>
>> +opal_call(OPAL_IMC_COUNTERS_INIT, opal_imc_counters_init, 1);
>> +
>> +/* opal_imc_counters_control_start: This call starts the nest imc engine. */
>> +static int64_t opal_imc_counters_start(uint32_t type)
>> +{
>> +	u64 op, status;
>> +	struct imc_chip_cb *cb;
>> +	int ret = OPAL_SUCCESS;
> I'm not sure there is any benefit in having ret.  Makes the code less readable.
> Just do return OPAL_SUCCESS below.

Ok sure. Will make the changes.

>> +
>> +	switch (type) {
>> +	case OPAL_IMC_COUNTERS_NEST:
>> +		/* Fetch the IMC control block structure */
>> +		cb = get_imc_cb();
>> +		status = be64_to_cpu(cb->imc_chip_run_status);
>> +
>> +		/* Check whether the engine is already running */
>> +		if (status == NEST_IMC_RUNNING)
>> +			return ret;
> Do we need a memory barrier here?  What ensures ordering between the status read
> above and the command below?

Currently nest counters are per-chip and only one thread from a chip
will update the command at a give time. Secondly, "command" is more
an input to the microcode. OPAL will not read it back.


>
>> +
>> +		/* Set the run command */
>> +		op = NEST_IMC_ENABLE;
>> +
>> +		/* Write the command to the control block now */
>> +		cb->imc_chip_command = op;
> Do we need a memory barrier before we can start reading these after this op?
> and if so, who's responsibility is it do do that?  OPAL, or Linux?

Linux will pass on the state the engine should be and OPAL will update
the command request from the linux. And microcode is the consumer
when comes to "command" field. OPAL does not read it back. Now thinking
more on it having a memory barrier will help. Will add it here.

Thanks for the review

Maddy

>
>> +
>> +		break;
>> +	default:
>> +		prerror("IMC: Unknown Domain in _START\n");
>> +		return OPAL_PARAMETER;
>> +	}
>> +
>> +	return ret;
>> +}
>> +opal_call(OPAL_IMC_COUNTERS_START, opal_imc_counters_start, 1);
>> +
>> +/* opal_imc_counters_control_stop: This call stops the nest imc engine. */
>> +static int64_t opal_imc_counters_stop(uint32_t type)
>> +{
>> +	u64 op, status;
>> +	struct imc_chip_cb *cb;
>> +	int ret = OPAL_SUCCESS;
>> +
>> +	switch (type) {
>> +	case OPAL_IMC_COUNTERS_NEST:
>> +		/* Fetch the IMC control block structure */
>> +		cb = get_imc_cb();
>> +		status = be64_to_cpu(cb->imc_chip_run_status);
> again... memory barriers?
>
>> +		/* Check whether the engine is already stopped */
>> +		if (status == NEST_IMC_PAUSE)
>> +			return ret;
>> +
>> +		/* Set the run command */
>> +		op = NEST_IMC_DISABLE;
>> +
>> +		/* Write the command to the control block now */
>> +		cb->imc_chip_command = op;
>> +
>> +		break;
>> +	default:
>> +		prerror("IMC: Unknown Domain in _STOP\n");
>> +		return OPAL_PARAMETER;
>> +	}
>> +
>> +	return ret;
>> +}
>> +opal_call(OPAL_IMC_COUNTERS_STOP, opal_imc_counters_stop, 1);
>> diff --git a/include/opal-api.h b/include/opal-api.h
>> index 44276553b818..02927356d033 100644
>> --- a/include/opal-api.h
>> +++ b/include/opal-api.h
>> @@ -204,7 +204,10 @@
>>   #define OPAL_NPU_INIT_CONTEXT			146
>>   #define OPAL_NPU_DESTROY_CONTEXT		147
>>   #define OPAL_NPU_MAP_LPAR			148
>> -#define OPAL_LAST				148
>> +#define OPAL_IMC_COUNTERS_INIT			149
>> +#define OPAL_IMC_COUNTERS_START			150
>> +#define OPAL_IMC_COUNTERS_STOP			151
>> +#define OPAL_LAST				151
>>   
>>   /* Device tree flags */
>>   
>> @@ -1226,6 +1229,12 @@ enum {
>>   	IMC_COUNTER_PER_SYSTEM		= 0x40,
>>   };
>>   
>> +/* Operation argument to IMC Microcode */
>> +enum {
>> +	OPAL_IMC_COUNTERS_NEST = 1,
>> +};
>> +
>> +
>>   #endif /* __ASSEMBLY__ */
>>   
>>   #endif /* __OPAL_API_H */