From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRs3D-0007mQ-W5 for qemu-devel@nongnu.org; Tue, 18 Aug 2015 21:20:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRs39-0007Dy-Qk for qemu-devel@nongnu.org; Tue, 18 Aug 2015 21:20:23 -0400 Date: Tue, 18 Aug 2015 18:15:35 -0700 From: David Gibson Message-ID: <20150819011535.GK3157@voom> References: <1439862430-14996-1-git-send-email-gwshan@linux.vnet.ibm.com> <1439862430-14996-4-git-send-email-gwshan@linux.vnet.ibm.com> <55D36C1D.8040608@redhat.com> <20150818235200.GB8064@gwshan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="dwWFXG4JqVa0wfCP" Content-Disposition: inline In-Reply-To: <20150818235200.GB8064@gwshan> Subject: Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gavin Shan Cc: aik@ozlabs.ru, peter.maydell@linaro.org, Thomas Huth , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --dwWFXG4JqVa0wfCP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 19, 2015 at 09:52:00AM +1000, Gavin Shan wrote: > On Tue, Aug 18, 2015 at 10:32:13AM -0700, Thomas Huth wrote: > >On 17/08/15 18:47, 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. > > > >Looking at the code, you're using a sequence number now instead of a > >random number? > > >=20 > Yes, it's what Alexey suggested. > >> Signed-off-by: Gavin Shan > >> --- > >> hw/ppc/spapr.c | 6 ++++- > >> hw/ppc/spapr_rtas.c | 66 +++++++++++++++++++++++++++++++++++++++++= +++++++++ > >> include/hw/ppc/spapr.h | 10 +++++++- > >> 3 files changed, 80 insertions(+), 2 deletions(-) > >>=20 > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index 06d000d..591a1a7 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -1191,7 +1191,7 @@ static bool version_before_3(void *opaque, int v= ersion_id) > >> =20 > >> static const VMStateDescription vmstate_spapr =3D { > >> .name =3D "spapr", > >> - .version_id =3D 3, > >> + .version_id =3D 4, > >> .minimum_version_id =3D 1, > >> .post_load =3D spapr_post_load, > >> .fields =3D (VMStateField[]) { > >> @@ -1202,6 +1202,10 @@ static const VMStateDescription vmstate_spapr = =3D { > >> VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_be= fore_3), > >> =20 > >> VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2), > >> + > >> + /* Error injection token */ > >> + VMSTATE_UINT32_V(errinjct_token, sPAPRMachineState, 4), > > > >Ok, so you're only saving the errinjct_token here, but not > >is_errinjct_opened? > > >=20 > Yes, It's also something that Alexey suggested in last round of review. Um.. this doesn't look right. You absolutely need to record the opened bit. You might be able to encode it in the token, but that would require pre_save and post_load hooks which you haven't added. The other approach would be to only have the token, advance it on both open and close, then use opened =3D=3D (token & 1) > >> VMSTATE_END_OF_LIST() > >> }, > >> }; > >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > >> index e99e25f..8405056 100644 > >> --- a/hw/ppc/spapr_rtas.c > >> +++ b/hw/ppc/spapr_rtas.c > >> @@ -604,6 +604,68 @@ out: > >> rtas_st(rets, 0, rc); > >> } > >> =20 > >> +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 !=3D 0) || (nret !=3D 2)) { > > > >Uh, did Alexey infect you with paranthesitis? > > >=20 > hehe~, nope. I'll drop those unnecessary paranthesitis :-) I'd prefer you didn't. Unlike Thomas, I also don't remember C order of ops that well and would prefer the clarity. > >> + ret =3D RTAS_OUT_PARAM_ERROR; > >> + goto out; > >> + } > >> + > >> + /* Check if we already had token */ > >> + if (spapr->is_errinjct_opened) { > >> + ret =3D RTAS_OUT_TOKEN_OPENED; > >> + goto out; > >> + } > >> + > >> + /* Grab the token */ > >> + spapr->is_errinjct_opened =3D true; > >> + rtas_st(rets, 0, ++spapr->errinjct_token); > >> + ret =3D 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 !=3D 1) || (nret !=3D 1)) { > >> + ret =3D RTAS_OUT_PARAM_ERROR; > >> + goto out; > >> + } > >> + > >> + /* Check if we had opened token */ > >> + if (!spapr->is_errinjct_opened) { > >> + ret =3D RTAS_OUT_CLOSE_ERROR; > >> + goto out; > >> + } > > > >... and here you check that another status variable "is_errinjct_opened" > >which is not saved in the VMStateDescription and thus this information > >will get lost during migration, I think. That looks like a bug to me. > > > >Can you do your code completely without "is_errinjct_opened"? I.e. by > >using errinjct_token =3D=3D 0 for signalling that no injection progress = is > >currently taking place? > > >=20 > It's fine to lose "is_errinjct_opened" after migration. The user needs > another attempt to do error injection after migration. It's really not. This means error injection will be implicitly closed on migration, which the guest shouldn't be able to see. If you don't preserve this, there's no point preserving the token value. > umm, In v1, I used the condition "errinjct_token =3D=3D 0" to indicate th= ere > is no injection in progress. After that, I received the suggestion to > change the code to have two variables: one boolean for error injection > token opening state and another one for error injection (sequential) > token. I don't see any problem with it.=20 >=20 > >> + /* Match with the passed token */ > >> + open_token =3D rtas_ld(args, 0); > >> + if (spapr->errinjct_token !=3D open_token) { > >> + ret =3D RTAS_OUT_CLOSE_ERROR; > >> + goto out; > >> + } > >> + > >> + spapr->is_errinjct_opened =3D false; > >> + ret =3D RTAS_OUT_SUCCESS; > >> +out: > >> + rtas_st(rets, 0, ret); > >> +} >=20 > Thanks, > Gavin >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --dwWFXG4JqVa0wfCP Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV09i3AAoJEGw4ysog2bOSO7YP/0LmFUf5oijDfc9Z7BxQU8eh QbCrTQxILgGiXznzHG5UMy2S/X4zN89FsexfiUvy0UaY209z/Ms6GIp/1epLkdsy TBPNiJZfpCL1IV27tMpEOCWFPRmdJlV4EaCtJNI/MSTKcFeIcFYGkuyxRvcO0dA6 B/3a8pgAQIxzccYfLtB5PXYdmxpdOKKXX5d3C0yf7K8xHPd28OhdNQ1ljPQ1/0N1 rnsh/ZAb0EIRHBWH6pkya95f5zwvN5iXzcJMwUuwM6ZDlETe3c37/RkugA82ytj4 MkuMaCmOoocH7AueSXFIZq2iHL3KMrJq/ZBbr+KRP+xTNAC0AH9c+Vz/Glq9/OKS 9eXrDbyUzBMAZC4YmBWcWN6554M77pPv1xmwVS+QvFetuSruc5y/2KlRg1ovLGS2 qiYgwx7Z3G5Tn397BXO67zG6PhcEWCf+hI1AUoLNGxSC5UP+8gO35IXxbcIuhirt jMMQvIRBYfpaI7CnQQ+WPqrMflyzGuM2UcwMZk9nNJS6IYtnlx39J1iCgQ38lFfc IQwlJXuQxLIVou4w9/eukNRxpDbtRCLOmov5vdlGSN9ijRQzt/dGOVBbAlyyH3So qt4IdHd8x5yQ6uxObxWmi5zcjIVmGlQ7upyKBS/HGkAZXgGUpoTph2EeYUoqWZaZ eDI6oCsYwdmRyIezEvf5 =W8hq -----END PGP SIGNATURE----- --dwWFXG4JqVa0wfCP--