From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMUAH-0003nV-T6 for qemu-devel@nongnu.org; Tue, 04 Aug 2015 00:49:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZMUAD-0000GM-OY for qemu-devel@nongnu.org; Tue, 04 Aug 2015 00:49:25 -0400 Received: from mail-pa0-f53.google.com ([209.85.220.53]:33493) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMUAD-0000G6-GQ for qemu-devel@nongnu.org; Tue, 04 Aug 2015 00:49:21 -0400 Received: by padck2 with SMTP id ck2so106603914pad.0 for ; Mon, 03 Aug 2015 21:49:19 -0700 (PDT) References: <1438557800-6947-1-git-send-email-gwshan@linux.vnet.ibm.com> <1438557800-6947-3-git-send-email-gwshan@linux.vnet.ibm.com> <20150803025109.GE15727@voom.fritz.box> <20150803033253.GA18268@gwshan> From: Alexey Kardashevskiy Message-ID: <55C0444A.2070206@ozlabs.ru> Date: Tue, 4 Aug 2015 14:49:14 +1000 MIME-Version: 1.0 In-Reply-To: <20150803033253.GA18268@gwshan> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RESEND v2 2/3] sPAPR: Support RTAS call ibm, {open, close}-errinjct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gavin Shan , David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 >>> --- >>> 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 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