* [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-30 15:07 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-30 15:07 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, rkrcmar, lvivier, kvm-ppc
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);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-30 15:07 ` Thomas Huth
@ 2017-05-30 15:19 ` Paolo Bonzini
-1 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2017-05-30 15:19 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-30 15:19 ` Paolo Bonzini
0 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2017-05-30 15:19 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-30 15:19 ` Paolo Bonzini
@ 2017-05-30 16:13 ` Thomas Huth
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-30 16:13 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-30 16:13 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-30 16:13 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-30 16:13 ` Thomas Huth
@ 2017-05-30 17:36 ` Paolo Bonzini
-1 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2017-05-30 17:36 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-30 17:36 ` Paolo Bonzini
0 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2017-05-30 17:36 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-30 15:07 ` Thomas Huth
@ 2017-05-31 8:21 ` Andrew Jones
-1 siblings, 0 replies; 18+ messages in thread
From: Andrew Jones @ 2017-05-31 8:21 UTC (permalink / raw)
To: Thomas Huth; +Cc: kvm, pbonzini, rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-31 8:21 ` Andrew Jones
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Jones @ 2017-05-31 8:21 UTC (permalink / raw)
To: Thomas Huth; +Cc: kvm, pbonzini, rkrcmar, lvivier, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-31 8:21 ` Andrew Jones
@ 2017-05-31 8:32 ` Thomas Huth
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-31 8:32 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, pbonzini, rkrcmar, lvivier, kvm-ppc
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-31 8:32 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-31 8:32 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, pbonzini, rkrcmar, lvivier, kvm-ppc
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-31 8:32 ` Thomas Huth
@ 2017-05-31 9:00 ` Laurent Vivier
-1 siblings, 0 replies; 18+ messages in thread
From: Laurent Vivier @ 2017-05-31 9:00 UTC (permalink / raw)
To: Thomas Huth, Andrew Jones; +Cc: kvm, pbonzini, rkrcmar, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-31 9:00 ` Laurent Vivier
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Vivier @ 2017-05-31 9:00 UTC (permalink / raw)
To: Thomas Huth, Andrew Jones; +Cc: kvm, pbonzini, rkrcmar, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-31 9:00 ` Laurent Vivier
@ 2017-05-31 9:27 ` Thomas Huth
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-31 9:27 UTC (permalink / raw)
To: Laurent Vivier, Andrew Jones; +Cc: kvm, pbonzini, rkrcmar, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-31 9:27 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2017-05-31 9:27 UTC (permalink / raw)
To: Laurent Vivier, Andrew Jones; +Cc: kvm, pbonzini, rkrcmar, kvm-ppc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
2017-05-30 15:07 ` Thomas Huth
@ 2017-05-31 8:29 ` Laurent Vivier
-1 siblings, 0 replies; 18+ messages in thread
From: Laurent Vivier @ 2017-05-31 8:29 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: pbonzini, rkrcmar, kvm-ppc
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree
@ 2017-05-31 8:29 ` Laurent Vivier
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Vivier @ 2017-05-31 8:29 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: pbonzini, rkrcmar, kvm-ppc
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>
^ permalink raw reply [flat|nested] 18+ messages in thread