| Message ID | 1496156844-29196-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/05/2017 17:07, Thomas Huth wrote: > , NULL, -1, -1); > + int token, ret; > + > + token = rtas_token("power-off"); > + if (token < 0) { > + puts("RTAS power-off not available\n"); > + return; > + } Should this do some kind of infinite loop (Linux arch/x86 has play_dead which does cli;hlt, not sure if there's something similar for sPAPR)? Thanks, Paolo
On 30.05.2017 17:19, Paolo Bonzini wrote: > > > On 30/05/2017 17:07, Thomas Huth wrote: >> , NULL, -1, -1); >> + int token, ret; >> + >> + token = rtas_token("power-off"); >> + if (token < 0) { >> + puts("RTAS power-off not available\n"); >> + return; >> + } > > Should this do some kind of infinite loop (Linux arch/x86 has play_dead > which does cli;hlt, not sure if there's something similar for sPAPR)? The exit() function in lib/powerpc/io.c already calls halt() after trying to run rtas_power_off() ... I think that should be enough here. Thomas
On 30/05/2017 18:13, Thomas Huth wrote: > On 30.05.2017 17:19, Paolo Bonzini wrote: >> >> >> On 30/05/2017 17:07, Thomas Huth wrote: >>> , NULL, -1, -1); >>> + int token, ret; >>> + >>> + token = rtas_token("power-off"); >>> + if (token < 0) { >>> + puts("RTAS power-off not available\n"); >>> + return; >>> + } >> >> Should this do some kind of infinite loop (Linux arch/x86 has play_dead >> which does cli;hlt, not sure if there's something similar for sPAPR)? > > The exit() function in lib/powerpc/io.c already calls halt() after > trying to run rtas_power_off() ... I think that should be enough here. Indeed, thanks! Paolo
On Tue, May 30, 2017 at 05:07:24PM +0200, Thomas Huth wrote: > When running a powerpc kvm-unit-test, and there is accidentially > no device tree available, the test ends up in an endless loop, > spamming the console with "rtas_node: /rtas: FDT_ERR_BADMAGIC" > messages: Somewhere the code calls abort() due to the missing > device tree, and abort() calls exit() which in turn tries to > shut down the VM with rtas_power_off(). rtas_power_off() needs > the device tree again to look up the right RTAS token and we > then end up in the next iteration. > Fix it by adding some proper checks to rtas_power_off() and > rtas_token(). > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/powerpc/rtas.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c > index 3407e25..a1c560b 100644 > --- a/lib/powerpc/rtas.c > +++ b/lib/powerpc/rtas.c > @@ -74,6 +74,9 @@ int rtas_token(const char *service) > const struct fdt_property *prop; > u32 *token; > > + if (!dt_available()) > + return RTAS_UNKNOWN_SERVICE; > + > prop = fdt_get_property(dt_fdt(), rtas_node(), service, NULL); > if (prop) { > token = (u32 *)prop->data; > @@ -116,6 +119,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) > > void rtas_power_off(void) > { > - int ret = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1); > + int token, ret; > + > + token = rtas_token("power-off"); > + if (token < 0) { token == RTAS_UNKNOWN_SERVICE ? > + puts("RTAS power-off not available\n"); > + return; > + } > + > + ret = rtas_call(token, 2, 1, NULL, -1, -1); > printf("RTAS power-off returned %d\n", ret); > } > -- > 1.8.3.1 > Thanks, drew
On 30/05/2017 17:07, Thomas Huth wrote: > When running a powerpc kvm-unit-test, and there is accidentially > no device tree available, the test ends up in an endless loop, > spamming the console with "rtas_node: /rtas: FDT_ERR_BADMAGIC" > messages: Somewhere the code calls abort() due to the missing > device tree, and abort() calls exit() which in turn tries to > shut down the VM with rtas_power_off(). rtas_power_off() needs > the device tree again to look up the right RTAS token and we > then end up in the next iteration. > Fix it by adding some proper checks to rtas_power_off() and > rtas_token(). > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/powerpc/rtas.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c > index 3407e25..a1c560b 100644 > --- a/lib/powerpc/rtas.c > +++ b/lib/powerpc/rtas.c > @@ -74,6 +74,9 @@ int rtas_token(const char *service) > const struct fdt_property *prop; > u32 *token; > > + if (!dt_available()) > + return RTAS_UNKNOWN_SERVICE; > + > prop = fdt_get_property(dt_fdt(), rtas_node(), service, NULL); > if (prop) { > token = (u32 *)prop->data; > @@ -116,6 +119,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) > > void rtas_power_off(void) > { > - int ret = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1); > + int token, ret; > + > + token = rtas_token("power-off"); > + if (token < 0) { > + puts("RTAS power-off not available\n"); > + return; > + } > + > + ret = rtas_call(token, 2, 1, NULL, -1, -1); > printf("RTAS power-off returned %d\n", ret); > } > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On 31.05.2017 10:21, Andrew Jones wrote: > On Tue, May 30, 2017 at 05:07:24PM +0200, Thomas Huth wrote: >> When running a powerpc kvm-unit-test, and there is accidentially >> no device tree available, the test ends up in an endless loop, >> spamming the console with "rtas_node: /rtas: FDT_ERR_BADMAGIC" >> messages: Somewhere the code calls abort() due to the missing >> device tree, and abort() calls exit() which in turn tries to >> shut down the VM with rtas_power_off(). rtas_power_off() needs >> the device tree again to look up the right RTAS token and we >> then end up in the next iteration. >> Fix it by adding some proper checks to rtas_power_off() and >> rtas_token(). >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> lib/powerpc/rtas.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c >> index 3407e25..a1c560b 100644 >> --- a/lib/powerpc/rtas.c >> +++ b/lib/powerpc/rtas.c >> @@ -74,6 +74,9 @@ int rtas_token(const char *service) >> const struct fdt_property *prop; >> u32 *token; >> >> + if (!dt_available()) >> + return RTAS_UNKNOWN_SERVICE; >> + >> prop = fdt_get_property(dt_fdt(), rtas_node(), service, NULL); >> if (prop) { >> token = (u32 *)prop->data; >> @@ -116,6 +119,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) >> >> void rtas_power_off(void) >> { >> - int ret = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1); >> + int token, ret; >> + >> + token = rtas_token("power-off"); >> + if (token < 0) { > > token == RTAS_UNKNOWN_SERVICE ? Yes, that's likely better ... though the tokens are normally > 0, I just had a look at the spec and it does not say anything about whether they have to be positive or not, so negative tokens could happen, too. I'll send a v2... Thanks, Thomas >> + puts("RTAS power-off not available\n"); >> + return; >> + } >> + >> + ret = rtas_call(token, 2, 1, NULL, -1, -1); >> printf("RTAS power-off returned %d\n", ret); >> } >> -- >> 1.8.3.1 >> > > Thanks, > drew >
On 31/05/2017 10:32, Thomas Huth wrote: > On 31.05.2017 10:21, Andrew Jones wrote: >> On Tue, May 30, 2017 at 05:07:24PM +0200, Thomas Huth wrote: >>> When running a powerpc kvm-unit-test, and there is accidentially >>> no device tree available, the test ends up in an endless loop, >>> spamming the console with "rtas_node: /rtas: FDT_ERR_BADMAGIC" >>> messages: Somewhere the code calls abort() due to the missing >>> device tree, and abort() calls exit() which in turn tries to >>> shut down the VM with rtas_power_off(). rtas_power_off() needs >>> the device tree again to look up the right RTAS token and we >>> then end up in the next iteration. >>> Fix it by adding some proper checks to rtas_power_off() and >>> rtas_token(). >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> lib/powerpc/rtas.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c >>> index 3407e25..a1c560b 100644 >>> --- a/lib/powerpc/rtas.c >>> +++ b/lib/powerpc/rtas.c >>> @@ -74,6 +74,9 @@ int rtas_token(const char *service) >>> const struct fdt_property *prop; >>> u32 *token; >>> >>> + if (!dt_available()) >>> + return RTAS_UNKNOWN_SERVICE; >>> + >>> prop = fdt_get_property(dt_fdt(), rtas_node(), service, NULL); >>> if (prop) { >>> token = (u32 *)prop->data; >>> @@ -116,6 +119,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) >>> >>> void rtas_power_off(void) >>> { >>> - int ret = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1); >>> + int token, ret; >>> + >>> + token = rtas_token("power-off"); >>> + if (token < 0) { >> >> token == RTAS_UNKNOWN_SERVICE ? > > Yes, that's likely better ... though the tokens are normally > 0, I just > had a look at the spec and it does not say anything about whether they > have to be positive or not, so negative tokens could happen, too. > I'll send a v2... in rtas_token(), token is u32, so a very big token number can appear as a negative value. But how do you return an error code if the return can be negative? Do you plan to use something like "error = rtas_token(&token, "power-off");"? Thanks, Laurent
On 31.05.2017 11:00, Laurent Vivier wrote: > On 31/05/2017 10:32, Thomas Huth wrote: >> On 31.05.2017 10:21, Andrew Jones wrote: >>> On Tue, May 30, 2017 at 05:07:24PM +0200, Thomas Huth wrote: >>>> When running a powerpc kvm-unit-test, and there is accidentially >>>> no device tree available, the test ends up in an endless loop, >>>> spamming the console with "rtas_node: /rtas: FDT_ERR_BADMAGIC" >>>> messages: Somewhere the code calls abort() due to the missing >>>> device tree, and abort() calls exit() which in turn tries to >>>> shut down the VM with rtas_power_off(). rtas_power_off() needs >>>> the device tree again to look up the right RTAS token and we >>>> then end up in the next iteration. >>>> Fix it by adding some proper checks to rtas_power_off() and >>>> rtas_token(). >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> lib/powerpc/rtas.c | 13 ++++++++++++- >>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c >>>> index 3407e25..a1c560b 100644 >>>> --- a/lib/powerpc/rtas.c >>>> +++ b/lib/powerpc/rtas.c >>>> @@ -74,6 +74,9 @@ int rtas_token(const char *service) >>>> const struct fdt_property *prop; >>>> u32 *token; >>>> >>>> + if (!dt_available()) >>>> + return RTAS_UNKNOWN_SERVICE; >>>> + >>>> prop = fdt_get_property(dt_fdt(), rtas_node(), service, NULL); >>>> if (prop) { >>>> token = (u32 *)prop->data; >>>> @@ -116,6 +119,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) >>>> >>>> void rtas_power_off(void) >>>> { >>>> - int ret = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1); >>>> + int token, ret; >>>> + >>>> + token = rtas_token("power-off"); >>>> + if (token < 0) { >>> >>> token == RTAS_UNKNOWN_SERVICE ? >> >> Yes, that's likely better ... though the tokens are normally > 0, I just >> had a look at the spec and it does not say anything about whether they >> have to be positive or not, so negative tokens could happen, too. >> I'll send a v2... > > in rtas_token(), token is u32, so a very big token number can appear as > a negative value. But how do you return an error code if the return can > be negative? > Do you plan to use something like > "error = rtas_token(&token, "power-off");"? Hmmm, though it is very unlikely that we ever encounter an RTAS implementation that uses 0xffffffff as token, it still could theoretically happen. So I guess I have to bite the bullet, implement something like you suggested and change all spots that currently use rtas_token()... Thomas
diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c index 3407e25..a1c560b 100644 --- a/lib/powerpc/rtas.c +++ b/lib/powerpc/rtas.c @@ -74,6 +74,9 @@ int rtas_token(const char *service) const struct fdt_property *prop; u32 *token; + if (!dt_available()) + return RTAS_UNKNOWN_SERVICE; + prop = fdt_get_property(dt_fdt(), rtas_node(), service, NULL); if (prop) { token = (u32 *)prop->data; @@ -116,6 +119,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) void rtas_power_off(void) { - int ret = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1); + int token, ret; + + token = rtas_token("power-off"); + if (token < 0) { + puts("RTAS power-off not available\n"); + return; + } + + ret = rtas_call(token, 2, 1, NULL, -1, -1); printf("RTAS power-off returned %d\n", ret); }
When running a powerpc kvm-unit-test, and there is accidentially no device tree available, the test ends up in an endless loop, spamming the console with "rtas_node: /rtas: FDT_ERR_BADMAGIC" messages: Somewhere the code calls abort() due to the missing device tree, and abort() calls exit() which in turn tries to shut down the VM with rtas_power_off(). rtas_power_off() needs the device tree again to look up the right RTAS token and we then end up in the next iteration. Fix it by adding some proper checks to rtas_power_off() and rtas_token(). Signed-off-by: Thomas Huth <thuth@redhat.com> --- lib/powerpc/rtas.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)