[kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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

[kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Laurent Vivier]

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>

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Laurent Vivier]

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>

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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
> 

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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
> 

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Laurent Vivier]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Laurent Vivier]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] powerpc: Fix endless loop that occurs without a device tree

[Thomas Huth]

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