| Message ID | 1439862430-14996-4-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <gwshan@linux.vnet.ibm.com> > --- > 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? > 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? > + 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? > + /* 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); > +} Thomas
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 <gwshan@linux.vnet.ibm.com> >> --- >> 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. >> 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 :-) >> + 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. 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
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 <gwshan@linux.vnet.ibm.com> > >> --- > >> 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 >
On 18/08/15 18:15, David Gibson wrote: > 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. Then please update the commit message accordingly. >>>> 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. You can always look it up if you're unsure, e.g.: http://en.cppreference.com/w/c/language/operator_precedence And once you've learnt it, the additional paranthesis just look cumbersome. So please remove them! Thomas
On Wed, Aug 19, 2015 at 09:15:26AM -0700, Thomas Huth wrote: >On 18/08/15 18:15, David Gibson wrote: >> 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. > >Then please update the commit message accordingly. > Yes, I'll update changelog accordingly. >>>>> 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. > >You can always look it up if you're unsure, e.g.: > >http://en.cppreference.com/w/c/language/operator_precedence > >And once you've learnt it, the additional paranthesis just look >cumbersome. So please remove them! > Ok. I'll check the code and remove unnecessary paranthesis in next revision. Thanks, Gavin
On 08/20/2015 02:15 AM, Thomas Huth wrote: > On 18/08/15 18:15, David Gibson wrote: >> 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. > > Then please update the commit message accordingly. > >>>>> 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. > > You can always look it up if you're unsure, e.g.: > > http://en.cppreference.com/w/c/language/operator_precedence > > And once you've learnt it, the additional paranthesis just look > cumbersome. So please remove them! I still prefer the clarity and therefore more braces which make no harm whatsoever.
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), + 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)) { + 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; + } + + /* 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); +} + static struct rtas_call { const char *name; spapr_rtas_fn fn; @@ -754,6 +816,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 d87c6d4..a3ec01d 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -73,6 +73,10 @@ struct sPAPRMachineState { int htab_fd; bool htab_fd_stale; + /* Error injection token */ + bool is_errinjct_opened; + uint32_t errinjct_token; + /* RTAS state */ QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; @@ -412,6 +416,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 +461,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
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 <gwshan@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 6 ++++- hw/ppc/spapr_rtas.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr.h | 10 +++++++- 3 files changed, 80 insertions(+), 2 deletions(-)