[Skiboot] [PATCH v2 5/7] skiboot: Add opal call to enable/disable Nest IMA microcode

[Hemant Kumar]

On 11/22/2016 07:25 AM, Oliver O'Halloran wrote:
> On Fri, Nov 18, 2016 at 1:10 PM, Hemant Kumar <hemant at linux.vnet.ibm.com> wrote:
>> Add a new opal call to start/stop the Nest IMA microcode running in the
>> OCC complex based on the "operation" parameter. Also, check the status
>> from the control block structure before starting/stopping the IMA
>> microcode.
>>
>> Signed-off-by: Hemant Kumar <hemant at linux.vnet.ibm.com>
>> ---
>> Changelog:
>> v1 -> v2:
>>   - Changed references from "pore_slw" to "IMA Microcode".
>>   - Changed the macro usage from "SLW_IMA_*" to "NEST_IMA_"*.
>>
>>   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 1fb07fd..5b012d5 100644
>> --- a/hw/ima.c
>> +++ b/hw/ima.c
>> @@ -189,3 +189,72 @@ void ima_init(void)
>>   err:
>>          free(buf);
>>   }
>> +
>> +/*
>> + * opal_nest_ima_counters_control : This call controls the nest IMA microcode.
>> + *
>> + * mode      : For now, this call supports only NEST_IMA_PRODUCTION_MODE.
>> + *             This mode can start/stop the Nest IMA Microcode for nest
>> + *             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.
> Honestly this looks like you're trying to build an extensible
> interface with no concrete plans for how it should be used. We *don't*
> want to create an interface that looks like ioctl() since that style
> of interface usually results in re-implementing parameter validation,
> etc on top of the existing OPAL call interface which does it's own
> parameter validation, etc. Keep it simple unless there's a really good
> reason to make it complicated.

Right. We need an extensible interface with plans to use the debug modes 
provided by the IMC interface (see "IMCRun Mode in patch 1/6 : 
https://lists.ozlabs.org/pipermail/skiboot/2016-December/005728.html). 
While we certainly don't want to make the interface complicated, we do 
want to keep the arguments for various debug modes.

All other issues below have been taken care of in v3 : 
https://lists.ozlabs.org/pipermail/skiboot/2016-December/005730.html

--
Thanks,
Hemant Kumar

>
>> + */
>> +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;
>> +       }
> Why not just return OPAL_PARAMETER? There's no cleanup to do so here
> so there's no advantage to having a single point of return. It just
> makes the control flow convoluted.
>
>> +
>> +       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;
> You might want to factor this out into a get_ima_cb() helper function.
>
>> +
>> +       switch (operation) {
>> +       case 0:
> Can you use an enum for the operations rather than fixed values? You
> can add them to opal-api.h so the enum names are part of the OPAL API.
>
>> +               /* Check whether the engine is already stopped */
>> +               if (cb->ima_chip_run_status == 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 == NEST_IMA_RESUME) {
>> +                       ret = OPAL_SUCCESS;
>> +                       goto nest_ret;
>> +               }
>> +
>> +               op = NEST_IMA_ENABLE;
>> +               break;
>> +       default:
>> +               prerror("IMA: unknown operation for nest ima\n");
>> +               ret = OPAL_PARAMETER;
>> +               goto nest_ret;
>> +       }
>> +
>> +       /* Write the command now to the control block */
>> +       cb->ima_chip_command = op;
>> +
>> +       ret = OPAL_SUCCESS;
>> +nest_ret:
>> +       return ret;
>> +}
>> +
>> +opal_call(OPAL_NEST_IMA_COUNTERS_CONTROL, opal_nest_ima_counters_control, 4);
>> diff --git a/include/opal-api.h b/include/opal-api.h
>> index 05ff51d..cbb5dcf 100644
>> --- a/include/opal-api.h
>> +++ b/include/opal-api.h
>> @@ -181,7 +181,8 @@
>>   #define OPAL_INT_SET_MFRR                      125
>>   #define OPAL_PCI_TCE_KILL                      126
>>   #define OPAL_NMMU_SET_PTCR                     127
>> -#define OPAL_LAST                              127
>> +#define OPAL_NEST_IMA_COUNTERS_CONTROL         128
>> +#define OPAL_LAST                              128
>>
>>   /* Device tree flags */
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot