On Tue, Aug 04, 2015 at 02:49:14PM +1000, Alexey Kardashevskiy wrote:
>On 08/03/2015 01:32 PM, Gavin Shan wrote:
>>On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
>>>On Mon, Aug 03, 2015 at 09:23:19AM +1000, Gavin Shan wrote:
>>>>The patch supports RTAS calls "ibm,{open,close}-errinjct" to
>>>>manupliate the token, which is passed to RTAS call "ibm,errinjct"
>>>>to indicate the valid context for error injection. Each VM is
>>>>permitted to have only one token at once and we simply have one
>>>>random number for that.
>>>>
>>>>Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com>
>>>>---
>>>> hw/ppc/spapr_rtas.c | 71
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> include/hw/ppc/spapr.h | 9 ++++++-
>>>> 2 files changed, 79 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>index e99e25f..0a9c904 100644
>>>>--- a/hw/ppc/spapr_rtas.c
>>>>+++ b/hw/ppc/spapr_rtas.c
>>>>@@ -604,6 +604,73 @@ out:
>>>> rtas_st(rets, 0, rc);
>>>> }
>>>>
>>>>+static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
>>>>+ sPAPRMachineState *spapr,
>>>>+ uint32_t token, uint32_t nargs,
>>>>+ target_ulong args, uint32_t nret,
>>>>+ target_ulong rets)
>>>>+{
>>>>+ int32_t ret;
>>>>+
>>>>+ /* Sanity check on number of arguments */
>>>>+ if ((nargs != 0) || (nret != 2)) {
>>>>+ ret = RTAS_OUT_PARAM_ERROR;
>>>>+ goto out;
>>>>+ }
>>>>+
>>>>+ /* Check if we already had token */
>>>>+ if (spapr->errinjct_token) {
>>>>+ ret = RTAS_OUT_TOKEN_OPENED;
>>>>+ goto out;
>>>>+ }
>>>>+
>>>>+ /* Grab random number as token */
>>>>+ spapr->errinjct_token = random();
>>>
>>>I don't quite understand the function of this token. Using random()
>>>seems a very, very odd way of doing things. Is it supposed to be a
>>>security thing?
>>>
>>
>>Yes, the token is allocated by "ibm,open-errinjct". The token will be
>>passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
>>perspecitve, the token owner is allowed to do error injection and it's
>>for security. Apart from having random number as the token, is there
>>better (fast) way to produce it?
>>
>>>>+ if (spapr->errinjct_token == 0) {
>>>>+ ret = RTAS_OUT_BUSY;
>>>
>>>AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
>>>for no particular reason.
>>>
>>
>>Yes, "0" represents invalid token (not opened). Maybe here we can retry
>>for a bit more like below. 0 returned from 10 successive random() would
>>be rare.
>>
>> uint32_t retries;
>>
>> while (!spapr->errinjct_token && retries++ < 10)
>> spapr->errinjct_token = random();
>> if (!spapr->errinjct_token) {
>> ret = RTAS_OUT_BUSY;
>> goto out;
>> }
>
>
>No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX
>which is 0x7fffffff, you could do something simple like this:
>
>spapr->errinjct_token = (rand % 32767) + 1
>
Good idea. I'll have it in next revision.
Thanks,
Gavin
>
>But for debugging purposes it makes more sense just to initialize it to 1 and
>then increment it in every call of rtas_ibm_open_errinjct().
>
>
>>
>>>>+ goto out;
>>>>+ }
>>>>+
>>>>+ rtas_st(rets, 0, spapr->errinjct_token);
>>>>+ ret = RTAS_OUT_SUCCESS;
>>>>+out:
>>>>+ rtas_st(rets, 1, ret);
>>>>+}
>>>>+
>>>>+static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
>>>>+ sPAPRMachineState *spapr,
>>>>+ uint32_t token, uint32_t nargs,
>>>>+ target_ulong args, uint32_t nret,
>>>>+ target_ulong rets)
>>>>+{
>>>>+ uint32_t open_token;
>>>>+ int32_t ret;
>>>>+
>>>>+ /* Sanity check on number of arguments */
>>>>+ if ((nargs != 1) || (nret != 1)) {
>>>>+ ret = RTAS_OUT_PARAM_ERROR;
>>>>+ goto out;
>>>>+ }
>>>>+
>>>>+ /* Check if we had opened token */
>>>>+ if (!spapr->errinjct_token) {
>>>>+ ret = RTAS_OUT_CLOSE_ERROR;
>>>>+ goto out;
>>>>+ }
>>>>+
>>>>+ /* Match with the passed token */
>>>>+ open_token = rtas_ld(args, 0);
>>>>+ if (spapr->errinjct_token != open_token) {
>>>>+ ret = RTAS_OUT_CLOSE_ERROR;
>>>>+ goto out;
>>>>+ }
>>>>+
>>>>+ spapr->errinjct_token = 0;
>>>>+ ret = RTAS_OUT_SUCCESS;
>>>>+out:
>>>>+ rtas_st(rets, 0, ret);
>>>>+}
>>>>+
>>>> static struct rtas_call {
>>>> const char *name;
>>>> spapr_rtas_fn fn;
>>>>@@ -754,6 +821,10 @@ static void core_rtas_register_types(void)
>>>> rtas_get_sensor_state);
>>>> spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR,
>>>> "ibm,configure-connector",
>>>> rtas_ibm_configure_connector);
>>>>+ spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct",
>>>>+ rtas_ibm_open_errinjct);
>>>>+ spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct",
>>>>+ rtas_ibm_close_errinjct);
>>>> }
>>>>
>>>> type_init(core_rtas_register_types)
>>>>diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>>index b6cb0d0..30d9854 100644
>>>>--- a/include/hw/ppc/spapr.h
>>>>+++ b/include/hw/ppc/spapr.h
>>>>@@ -73,6 +73,9 @@ struct sPAPRMachineState {
>>>> int htab_fd;
>>>> bool htab_fd_stale;
>>>>
>>>>+ /* Error injection token */
>>>>+ uint32_t errinjct_token;
>>>>+
>>>> /* RTAS state */
>>>> QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>>>>
>>>>@@ -412,6 +415,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool
>>>>msi);
>>>> #define RTAS_OUT_BUSY -2
>>>> #define RTAS_OUT_PARAM_ERROR -3
>>>> #define RTAS_OUT_NOT_SUPPORTED -3
>>>>+#define RTAS_OUT_TOKEN_OPENED -4
>>>>+#define RTAS_OUT_CLOSE_ERROR -4
>>>> #define RTAS_OUT_NOT_AUTHORIZED -9002
>>>>
>>>> /* RTAS tokens */
>>>>@@ -455,8 +460,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool
>>>>msi);
>>>> #define RTAS_IBM_SET_SLOT_RESET (RTAS_TOKEN_BASE + 0x23)
>>>> #define RTAS_IBM_CONFIGURE_PE (RTAS_TOKEN_BASE + 0x24)
>>>> #define RTAS_IBM_SLOT_ERROR_DETAIL (RTAS_TOKEN_BASE + 0x25)
>>>>+#define RTAS_IBM_OPEN_ERRINJCT (RTAS_TOKEN_BASE + 0x26)
>>>>+#define RTAS_IBM_CLOSE_ERRINJCT (RTAS_TOKEN_BASE + 0x27)
>>>>
>>>>-#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x26)
>>>>+#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x28)
>>>>
>>>> /* RTAS ibm,get-system-parameter token values */
>>>> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
>>
>>Thanks,
>>Gavin
>>
>
>
>--
>Alexey
>
