qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm,


From: David Gibson
Subject: Re: [Qemu-ppc] [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct
Date: Tue, 18 Aug 2015 18:15:35 -0700
User-agent: Mutt/1.5.23 (2014-03-12)

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?
> >
> 
> Yes, it's what Alexey suggested.

> >> Signed-off-by: Gavin Shan <address@hidden>
> >> ---
> >>  hw/ppc/spapr.c         |  6 ++++-
> >>  hw/ppc/spapr_rtas.c    | 66 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h | 10 +++++++-
> >>  3 files changed, 80 insertions(+), 2 deletions(-)
> >> 
> >> 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 
> >> version_id)
> >>  
> >>  static const VMStateDescription vmstate_spapr = {
> >>      .name = "spapr",
> >> -    .version_id = 3,
> >> +    .version_id = 4,
> >>      .minimum_version_id = 1,
> >>      .post_load = spapr_post_load,
> >>      .fields = (VMStateField[]) {
> >> @@ -1202,6 +1202,10 @@ static const VMStateDescription vmstate_spapr = {
> >>          VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, 
> >> version_before_3),
> >>  
> >>          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?
> >
> 
> 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 == (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);
> >>  }
> >>  
> >> +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)) {
> >
> >Uh, did Alexey infect you with paranthesitis?
> >
> 
> 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 = RTAS_OUT_PARAM_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Check if we already had token */
> >> +    if (spapr->is_errinjct_opened) {
> >> +        ret = RTAS_OUT_TOKEN_OPENED;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Grab the token */
> >> +    spapr->is_errinjct_opened = true;
> >> +    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->is_errinjct_opened) {
> >> +        ret = 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 == 0 for signalling that no injection progress is
> >currently taking place?
> >
> 
> 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 == 0" to indicate there
> 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. 
> 
> >> +    /* 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->is_errinjct_opened = false;
> >> +    ret = RTAS_OUT_SUCCESS;
> >> +out:
> >> +    rtas_st(rets, 0, ret);
> >> +}
> 
> Thanks,
> Gavin
> 

-- 
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

Attachment: pgpuV00KKxdZ_.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]