[Skiboot] [RFC PATCH 5/7] skiboot: Add opal call for nest IMA configuration

[Hemant Kumar]

On 11/02/2016 12:09 PM, Madhavan Srinivasan wrote:
>
>
> On Tuesday 25 October 2016 06:15 PM, Hemant Kumar wrote:
>> Add a new opal call to control the nest IMA counters.
>> opal_nest_ima_counters_control() is to start and stop OCC
>> microcode for nest PMU counter collection based on the "operation"
>> parameter.
>
> Would prefer the title of the patch to be
>
> "skiboot: Add opal call to enable/disable Nest IMA microcode"
>
> OPAL Call is to start/stop the Nest microcode running in the OCC
> complex.
>
> Nest counters are configured by the microcode and not by OPAL
> or kernel.
>

Makes sense. Will change the title and commit message and remove the 
misleading "configuration from OPAL".

>>
>> Signed-off-by: Hemant Kumar <hemant at linux.vnet.ibm.com>
>> ---
>>   hw/ima.c           | 69 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/opal-api.h |  3 ++-
>>   2 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ima.c b/hw/ima.c
>> index 7dd8d8c..cce5473 100644
>> --- a/hw/ima.c
>> +++ b/hw/ima.c
>> @@ -139,3 +139,72 @@ void ima_init(void)
>>   err:
>>       free(buf);
>>   }
>> +
>> +/*
>> + * opal_nest_ima_counters_control : This call controls the nest IMA 
>> counters.
> nest IMA "microcode" and not "counters".
>

Yeah, will change it.

>> + *
>> + * mode      : For now, this call supports only 
>> NEST_IMA_PRODUCTION_MODE.
>> + *             This mode can start/stop the PORE SLW IMA engine for 
>> nest
> Instead of "PORE SLW IMA", can it be "Nest IMA Microcode"?

Yes, it should be. Will fix it.

>> + *             instrumentation from Host OS.
>> + * operation : Start(0x0) or Stop(0x1) the engine.
>> + *
>> + * This call can be extended to include more operations, if needed 
>> and the
>> + * other two parameters value_1 and value_2 are for the very same 
>> purpose.
>> + * Right now, they are unused and should be zero.
>> + */
>> +static int64_t opal_nest_ima_counters_control(uint64_t mode,
>> +                          uint64_t operation,
>> +                          uint64_t value_1,
>> +                          uint64_t value_2)
>> +{
>> +    struct proc_chip *chip;
>> +    u64 op;
>> +    int ret;
>> +    uint64_t cb_loc;
>> +    struct ima_chip_cb *cb;
>> +
>> +    if ((mode != NEST_IMA_PRODUCTION_MODE) || value_1 || value_2) {
>> +        ret = OPAL_PARAMETER;
>> +        goto nest_ret;
>> +    }
>> +
>> +    chip = get_chip(this_cpu()->chip_id);
>> +
>> +    /* Fetch the IMA control block structure */
>> +    cb_loc = chip->homer_base + CB_STRUCT_OFFSET;
>> +    cb = (struct ima_chip_cb *)cb_loc;
>> +
>> +    switch (operation) {
>> +    case 0:
>> +        /* Check whether the engine is already stopped */
>> +        if (cb->ima_chip_run_status == SLW_IMA_PAUSE) {
>
> Can it be "NEST_IMA_PAUSE" instead of  "SLW_IMA_PAUSE"?
>

Yup, will change it to NEST_IMA_PAUSE.

>
>> +            ret = OPAL_SUCCESS;
>> +            goto nest_ret;
>> +        }
>> +
>> +        op = NEST_IMA_DISABLE;
>> +        break;
>> +    case 1:
>> +        /* Check whether the engine is already running */
>> +        if (cb->ima_chip_run_status == SLW_IMA_RESUME) {
>
> Same here. Can it be NEST_IMA_RESUME instead of SLW_IMA_RESUME

Yes.

--
Thanks,
Hemant