From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56446) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRkkK-00080G-As for qemu-devel@nongnu.org; Tue, 18 Aug 2015 13:32:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRkkG-00021N-G5 for qemu-devel@nongnu.org; Tue, 18 Aug 2015 13:32:24 -0400 Message-ID: <55D36C1D.8040608@redhat.com> Date: Tue, 18 Aug 2015 10:32:13 -0700 From: Thomas Huth MIME-Version: 1.0 References: <1439862430-14996-1-git-send-email-gwshan@linux.vnet.ibm.com> <1439862430-14996-4-git-send-email-gwshan@linux.vnet.ibm.com> In-Reply-To: <1439862430-14996-4-git-send-email-gwshan@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 , qemu-devel@nongnu.org Cc: aik@ozlabs.ru, peter.maydell@linaro.org, qemu-ppc@nongnu.org, david@gibson.dropbear.id.au 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? > 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 ve= rsion_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_bef= ore_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? > 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? > + 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 i= s currently taking place? > + /* 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); > +} Thomas