* [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-08 15:11 ` Alexandru Elisei
0 siblings, 0 replies; 15+ messages in thread
From: Alexandru Elisei @ 2019-04-08 15:11 UTC (permalink / raw)
To: kvm; +Cc: kvmarm, drjones, vladimir.murzin
The prototype for the halt() function is incorrect, because halt() doesn't
take any arguments. Fix it by using the prototype from smp.h.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/io.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 8226b765bdc5..6d3d7afed002 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -15,11 +15,10 @@
#include <asm/psci.h>
#include <asm/spinlock.h>
#include <asm/io.h>
+#include <asm/smp.h>
#include "io.h"
-extern void halt(int code);
-
static struct spinlock uart_lock;
/*
* Use this guess for the uart base in order to make an attempt at
@@ -93,6 +92,6 @@ void exit(int code)
{
chr_testdev_exit(code);
psci_system_off();
- halt(code);
+ halt();
__builtin_unreachable();
}
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-08 15:11 ` Alexandru Elisei
0 siblings, 0 replies; 15+ messages in thread
From: Alexandru Elisei @ 2019-04-08 15:11 UTC (permalink / raw)
To: kvm; +Cc: kvmarm
The prototype for the halt() function is incorrect, because halt() doesn't
take any arguments. Fix it by using the prototype from smp.h.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/io.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 8226b765bdc5..6d3d7afed002 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -15,11 +15,10 @@
#include <asm/psci.h>
#include <asm/spinlock.h>
#include <asm/io.h>
+#include <asm/smp.h>
#include "io.h"
-extern void halt(int code);
-
static struct spinlock uart_lock;
/*
* Use this guess for the uart base in order to make an attempt at
@@ -93,6 +92,6 @@ void exit(int code)
{
chr_testdev_exit(code);
psci_system_off();
- halt(code);
+ halt();
__builtin_unreachable();
}
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-08 15:11 ` Alexandru Elisei
0 siblings, 0 replies; 15+ messages in thread
From: Alexandru Elisei @ 2019-04-08 15:11 UTC (permalink / raw)
To: kvm; +Cc: kvmarm
The prototype for the halt() function is incorrect, because halt() doesn't
take any arguments. Fix it by using the prototype from smp.h.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/io.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 8226b765bdc5..6d3d7afed002 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -15,11 +15,10 @@
#include <asm/psci.h>
#include <asm/spinlock.h>
#include <asm/io.h>
+#include <asm/smp.h>
#include "io.h"
-extern void halt(int code);
-
static struct spinlock uart_lock;
/*
* Use this guess for the uart base in order to make an attempt at
@@ -93,6 +92,6 @@ void exit(int code)
{
chr_testdev_exit(code);
psci_system_off();
- halt(code);
+ halt();
__builtin_unreachable();
}
--
2.17.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
2019-04-08 15:11 ` Alexandru Elisei
(?)
@ 2019-04-08 17:00 ` Sean Christopherson
-1 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:00 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvm, kvmarm, drjones, vladimir.murzin
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> The prototype for the halt() function is incorrect, because halt() doesn't
> take any arguments. Fix it by using the prototype from smp.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-08 17:00 ` Sean Christopherson
0 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:00 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvmarm, kvm
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> The prototype for the halt() function is incorrect, because halt() doesn't
> take any arguments. Fix it by using the prototype from smp.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-08 17:00 ` Sean Christopherson
0 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:00 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvmarm, kvm
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> The prototype for the halt() function is incorrect, because halt() doesn't
> take any arguments. Fix it by using the prototype from smp.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
2019-04-08 15:11 ` Alexandru Elisei
(?)
@ 2019-04-09 7:40 ` Andrew Jones
-1 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2019-04-09 7:40 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvm, kvmarm, vladimir.murzin
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> The prototype for the halt() function is incorrect, because halt() doesn't
> take any arguments. Fix it by using the prototype from smp.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> lib/arm/io.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 8226b765bdc5..6d3d7afed002 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -15,11 +15,10 @@
> #include <asm/psci.h>
> #include <asm/spinlock.h>
> #include <asm/io.h>
> +#include <asm/smp.h>
>
> #include "io.h"
>
> -extern void halt(int code);
> -
> static struct spinlock uart_lock;
> /*
> * Use this guess for the uart base in order to make an attempt at
> @@ -93,6 +92,6 @@ void exit(int code)
> {
> chr_testdev_exit(code);
> psci_system_off();
> - halt(code);
> + halt();
> __builtin_unreachable();
> }
> --
> 2.17.0
>
I don't mind this change, because per the code it is the "correct"
thing to do. However, I was being a bit tricky here when I wrote it.
By changing the prototype to take 'code' as argument we guarantee
that 'code' will be in x0/r0 when we halt, giving us a last chance
to see it when inspecting the halted unit test state.
Anyway, like I said, I'm fine with the cleanup, but the prototype
abuse does serve a purpose - maybe just not a good enough purpose
to justify the weirdness.
Thanks,
drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-09 7:40 ` Andrew Jones
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2019-04-09 7:40 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvmarm, kvm
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> The prototype for the halt() function is incorrect, because halt() doesn't
> take any arguments. Fix it by using the prototype from smp.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> lib/arm/io.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 8226b765bdc5..6d3d7afed002 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -15,11 +15,10 @@
> #include <asm/psci.h>
> #include <asm/spinlock.h>
> #include <asm/io.h>
> +#include <asm/smp.h>
>
> #include "io.h"
>
> -extern void halt(int code);
> -
> static struct spinlock uart_lock;
> /*
> * Use this guess for the uart base in order to make an attempt at
> @@ -93,6 +92,6 @@ void exit(int code)
> {
> chr_testdev_exit(code);
> psci_system_off();
> - halt(code);
> + halt();
> __builtin_unreachable();
> }
> --
> 2.17.0
>
I don't mind this change, because per the code it is the "correct"
thing to do. However, I was being a bit tricky here when I wrote it.
By changing the prototype to take 'code' as argument we guarantee
that 'code' will be in x0/r0 when we halt, giving us a last chance
to see it when inspecting the halted unit test state.
Anyway, like I said, I'm fine with the cleanup, but the prototype
abuse does serve a purpose - maybe just not a good enough purpose
to justify the weirdness.
Thanks,
drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-09 7:40 ` Andrew Jones
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2019-04-09 7:40 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvmarm, kvm
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> The prototype for the halt() function is incorrect, because halt() doesn't
> take any arguments. Fix it by using the prototype from smp.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> lib/arm/io.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 8226b765bdc5..6d3d7afed002 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -15,11 +15,10 @@
> #include <asm/psci.h>
> #include <asm/spinlock.h>
> #include <asm/io.h>
> +#include <asm/smp.h>
>
> #include "io.h"
>
> -extern void halt(int code);
> -
> static struct spinlock uart_lock;
> /*
> * Use this guess for the uart base in order to make an attempt at
> @@ -93,6 +92,6 @@ void exit(int code)
> {
> chr_testdev_exit(code);
> psci_system_off();
> - halt(code);
> + halt();
> __builtin_unreachable();
> }
> --
> 2.17.0
>
I don't mind this change, because per the code it is the "correct"
thing to do. However, I was being a bit tricky here when I wrote it.
By changing the prototype to take 'code' as argument we guarantee
that 'code' will be in x0/r0 when we halt, giving us a last chance
to see it when inspecting the halted unit test state.
Anyway, like I said, I'm fine with the cleanup, but the prototype
abuse does serve a purpose - maybe just not a good enough purpose
to justify the weirdness.
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
2019-04-09 7:40 ` Andrew Jones
(?)
@ 2019-04-09 9:15 ` Alexandru Elisei
-1 siblings, 0 replies; 15+ messages in thread
From: Alexandru Elisei @ 2019-04-09 9:15 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, kvmarm, vladimir.murzin
On 4/9/19 8:40 AM, Andrew Jones wrote:
> On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
>> The prototype for the halt() function is incorrect, because halt() doesn't
>> take any arguments. Fix it by using the prototype from smp.h.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> lib/arm/io.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 8226b765bdc5..6d3d7afed002 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -15,11 +15,10 @@
>> #include <asm/psci.h>
>> #include <asm/spinlock.h>
>> #include <asm/io.h>
>> +#include <asm/smp.h>
>>
>> #include "io.h"
>>
>> -extern void halt(int code);
>> -
>> static struct spinlock uart_lock;
>> /*
>> * Use this guess for the uart base in order to make an attempt at
>> @@ -93,6 +92,6 @@ void exit(int code)
>> {
>> chr_testdev_exit(code);
>> psci_system_off();
>> - halt(code);
>> + halt();
>> __builtin_unreachable();
>> }
>> --
>> 2.17.0
>>
> I don't mind this change, because per the code it is the "correct"
> thing to do. However, I was being a bit tricky here when I wrote it.
> By changing the prototype to take 'code' as argument we guarantee
> that 'code' will be in x0/r0 when we halt, giving us a last chance
> to see it when inspecting the halted unit test state.
>
> Anyway, like I said, I'm fine with the cleanup, but the prototype
> abuse does serve a purpose - maybe just not a good enough purpose
> to justify the weirdness.
Now it makes sense. I didn't think it was intentional, but now that you have
mentioned it, the same pattern is used by powerpc.
Perhaps a comment explaining that having different prototypes was on purpose
would be the best solution?
>
> Thanks,
> drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-09 9:15 ` Alexandru Elisei
0 siblings, 0 replies; 15+ messages in thread
From: Alexandru Elisei @ 2019-04-09 9:15 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvmarm, kvm
On 4/9/19 8:40 AM, Andrew Jones wrote:
> On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
>> The prototype for the halt() function is incorrect, because halt() doesn't
>> take any arguments. Fix it by using the prototype from smp.h.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> lib/arm/io.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 8226b765bdc5..6d3d7afed002 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -15,11 +15,10 @@
>> #include <asm/psci.h>
>> #include <asm/spinlock.h>
>> #include <asm/io.h>
>> +#include <asm/smp.h>
>>
>> #include "io.h"
>>
>> -extern void halt(int code);
>> -
>> static struct spinlock uart_lock;
>> /*
>> * Use this guess for the uart base in order to make an attempt at
>> @@ -93,6 +92,6 @@ void exit(int code)
>> {
>> chr_testdev_exit(code);
>> psci_system_off();
>> - halt(code);
>> + halt();
>> __builtin_unreachable();
>> }
>> --
>> 2.17.0
>>
> I don't mind this change, because per the code it is the "correct"
> thing to do. However, I was being a bit tricky here when I wrote it.
> By changing the prototype to take 'code' as argument we guarantee
> that 'code' will be in x0/r0 when we halt, giving us a last chance
> to see it when inspecting the halted unit test state.
>
> Anyway, like I said, I'm fine with the cleanup, but the prototype
> abuse does serve a purpose - maybe just not a good enough purpose
> to justify the weirdness.
Now it makes sense. I didn't think it was intentional, but now that you have
mentioned it, the same pattern is used by powerpc.
Perhaps a comment explaining that having different prototypes was on purpose
would be the best solution?
>
> Thanks,
> drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-09 9:15 ` Alexandru Elisei
0 siblings, 0 replies; 15+ messages in thread
From: Alexandru Elisei @ 2019-04-09 9:15 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvmarm, kvm
On 4/9/19 8:40 AM, Andrew Jones wrote:
> On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
>> The prototype for the halt() function is incorrect, because halt() doesn't
>> take any arguments. Fix it by using the prototype from smp.h.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> lib/arm/io.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 8226b765bdc5..6d3d7afed002 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -15,11 +15,10 @@
>> #include <asm/psci.h>
>> #include <asm/spinlock.h>
>> #include <asm/io.h>
>> +#include <asm/smp.h>
>>
>> #include "io.h"
>>
>> -extern void halt(int code);
>> -
>> static struct spinlock uart_lock;
>> /*
>> * Use this guess for the uart base in order to make an attempt at
>> @@ -93,6 +92,6 @@ void exit(int code)
>> {
>> chr_testdev_exit(code);
>> psci_system_off();
>> - halt(code);
>> + halt();
>> __builtin_unreachable();
>> }
>> --
>> 2.17.0
>>
> I don't mind this change, because per the code it is the "correct"
> thing to do. However, I was being a bit tricky here when I wrote it.
> By changing the prototype to take 'code' as argument we guarantee
> that 'code' will be in x0/r0 when we halt, giving us a last chance
> to see it when inspecting the halted unit test state.
>
> Anyway, like I said, I'm fine with the cleanup, but the prototype
> abuse does serve a purpose - maybe just not a good enough purpose
> to justify the weirdness.
Now it makes sense. I didn't think it was intentional, but now that you have
mentioned it, the same pattern is used by powerpc.
Perhaps a comment explaining that having different prototypes was on purpose
would be the best solution?
>
> Thanks,
> drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
2019-04-09 9:15 ` Alexandru Elisei
(?)
@ 2019-04-09 10:11 ` Andrew Jones
-1 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2019-04-09 10:11 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvm, kvmarm, vladimir.murzin
On Tue, Apr 09, 2019 at 10:15:59AM +0100, Alexandru Elisei wrote:
> On 4/9/19 8:40 AM, Andrew Jones wrote:
> > On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> >> The prototype for the halt() function is incorrect, because halt() doesn't
> >> take any arguments. Fix it by using the prototype from smp.h.
> >>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >> lib/arm/io.c | 5 ++---
> >> 1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >> index 8226b765bdc5..6d3d7afed002 100644
> >> --- a/lib/arm/io.c
> >> +++ b/lib/arm/io.c
> >> @@ -15,11 +15,10 @@
> >> #include <asm/psci.h>
> >> #include <asm/spinlock.h>
> >> #include <asm/io.h>
> >> +#include <asm/smp.h>
> >>
> >> #include "io.h"
> >>
> >> -extern void halt(int code);
> >> -
> >> static struct spinlock uart_lock;
> >> /*
> >> * Use this guess for the uart base in order to make an attempt at
> >> @@ -93,6 +92,6 @@ void exit(int code)
> >> {
> >> chr_testdev_exit(code);
> >> psci_system_off();
> >> - halt(code);
> >> + halt();
> >> __builtin_unreachable();
> >> }
> >> --
> >> 2.17.0
> >>
> > I don't mind this change, because per the code it is the "correct"
> > thing to do. However, I was being a bit tricky here when I wrote it.
> > By changing the prototype to take 'code' as argument we guarantee
> > that 'code' will be in x0/r0 when we halt, giving us a last chance
> > to see it when inspecting the halted unit test state.
> >
> > Anyway, like I said, I'm fine with the cleanup, but the prototype
> > abuse does serve a purpose - maybe just not a good enough purpose
> > to justify the weirdness.
>
> Now it makes sense. I didn't think it was intentional, but now that you have
> mentioned it, the same pattern is used by powerpc.
No surprise there. I wrote that too, based on the arm code :)
>
> Perhaps a comment explaining that having different prototypes was on purpose
> would be the best solution?
Yes, either a comment explaining the weirdness or your patch to
remove it would be a good idea in order to avoid future head
scratching. I'll send a patch that adds comments to both arm and
powerpc.
Thanks,
drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-09 10:11 ` Andrew Jones
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2019-04-09 10:11 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvmarm, kvm
On Tue, Apr 09, 2019 at 10:15:59AM +0100, Alexandru Elisei wrote:
> On 4/9/19 8:40 AM, Andrew Jones wrote:
> > On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> >> The prototype for the halt() function is incorrect, because halt() doesn't
> >> take any arguments. Fix it by using the prototype from smp.h.
> >>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >> lib/arm/io.c | 5 ++---
> >> 1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >> index 8226b765bdc5..6d3d7afed002 100644
> >> --- a/lib/arm/io.c
> >> +++ b/lib/arm/io.c
> >> @@ -15,11 +15,10 @@
> >> #include <asm/psci.h>
> >> #include <asm/spinlock.h>
> >> #include <asm/io.h>
> >> +#include <asm/smp.h>
> >>
> >> #include "io.h"
> >>
> >> -extern void halt(int code);
> >> -
> >> static struct spinlock uart_lock;
> >> /*
> >> * Use this guess for the uart base in order to make an attempt at
> >> @@ -93,6 +92,6 @@ void exit(int code)
> >> {
> >> chr_testdev_exit(code);
> >> psci_system_off();
> >> - halt(code);
> >> + halt();
> >> __builtin_unreachable();
> >> }
> >> --
> >> 2.17.0
> >>
> > I don't mind this change, because per the code it is the "correct"
> > thing to do. However, I was being a bit tricky here when I wrote it.
> > By changing the prototype to take 'code' as argument we guarantee
> > that 'code' will be in x0/r0 when we halt, giving us a last chance
> > to see it when inspecting the halted unit test state.
> >
> > Anyway, like I said, I'm fine with the cleanup, but the prototype
> > abuse does serve a purpose - maybe just not a good enough purpose
> > to justify the weirdness.
>
> Now it makes sense. I didn't think it was intentional, but now that you have
> mentioned it, the same pattern is used by powerpc.
No surprise there. I wrote that too, based on the arm code :)
>
> Perhaps a comment explaining that having different prototypes was on purpose
> would be the best solution?
Yes, either a comment explaining the weirdness or your patch to
remove it would be a good idea in order to avoid future head
scratching. I'll send a patch that adds comments to both arm and
powerpc.
Thanks,
drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h
@ 2019-04-09 10:11 ` Andrew Jones
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2019-04-09 10:11 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvmarm, kvm
On Tue, Apr 09, 2019 at 10:15:59AM +0100, Alexandru Elisei wrote:
> On 4/9/19 8:40 AM, Andrew Jones wrote:
> > On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote:
> >> The prototype for the halt() function is incorrect, because halt() doesn't
> >> take any arguments. Fix it by using the prototype from smp.h.
> >>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >> lib/arm/io.c | 5 ++---
> >> 1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >> index 8226b765bdc5..6d3d7afed002 100644
> >> --- a/lib/arm/io.c
> >> +++ b/lib/arm/io.c
> >> @@ -15,11 +15,10 @@
> >> #include <asm/psci.h>
> >> #include <asm/spinlock.h>
> >> #include <asm/io.h>
> >> +#include <asm/smp.h>
> >>
> >> #include "io.h"
> >>
> >> -extern void halt(int code);
> >> -
> >> static struct spinlock uart_lock;
> >> /*
> >> * Use this guess for the uart base in order to make an attempt at
> >> @@ -93,6 +92,6 @@ void exit(int code)
> >> {
> >> chr_testdev_exit(code);
> >> psci_system_off();
> >> - halt(code);
> >> + halt();
> >> __builtin_unreachable();
> >> }
> >> --
> >> 2.17.0
> >>
> > I don't mind this change, because per the code it is the "correct"
> > thing to do. However, I was being a bit tricky here when I wrote it.
> > By changing the prototype to take 'code' as argument we guarantee
> > that 'code' will be in x0/r0 when we halt, giving us a last chance
> > to see it when inspecting the halted unit test state.
> >
> > Anyway, like I said, I'm fine with the cleanup, but the prototype
> > abuse does serve a purpose - maybe just not a good enough purpose
> > to justify the weirdness.
>
> Now it makes sense. I didn't think it was intentional, but now that you have
> mentioned it, the same pattern is used by powerpc.
No surprise there. I wrote that too, based on the arm code :)
>
> Perhaps a comment explaining that having different prototypes was on purpose
> would be the best solution?
Yes, either a comment explaining the weirdness or your patch to
remove it would be a good idea in order to avoid future head
scratching. I'll send a patch that adds comments to both arm and
powerpc.
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-04-09 10:11 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08 15:11 [kvm-unit-tests PATCH] lib: arm: Use correct halt() prototype from smp.h Alexandru Elisei
2019-04-08 15:11 ` Alexandru Elisei
2019-04-08 15:11 ` Alexandru Elisei
2019-04-08 17:00 ` Sean Christopherson
2019-04-08 17:00 ` Sean Christopherson
2019-04-08 17:00 ` Sean Christopherson
2019-04-09 7:40 ` Andrew Jones
2019-04-09 7:40 ` Andrew Jones
2019-04-09 7:40 ` Andrew Jones
2019-04-09 9:15 ` Alexandru Elisei
2019-04-09 9:15 ` Alexandru Elisei
2019-04-09 9:15 ` Alexandru Elisei
2019-04-09 10:11 ` Andrew Jones
2019-04-09 10:11 ` Andrew Jones
2019-04-09 10:11 ` Andrew Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.