[PATCH 0/4] Fix MISRA C 2012 violations

[PATCH 0/4] Fix MISRA C 2012 violations

[Xenia Ragiadakou]

Resolve MISRA C 2012 Rule 8.4 violation warnings.

Xenia Ragiadakou (4):
  xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
  xen/common: time: Fix MISRA C 2012 Rule 8.7 violation
  xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
  xen/char: pv_console: Fix MISRA C 2012 Rule 8.4 violation

 xen/arch/arm/domain.c                |  2 +-
 xen/arch/arm/include/asm/processor.h | 14 ++++++++++++++
 xen/common/time.c                    |  2 +-
 xen/include/xen/pv_console.h         |  2 +-
 4 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

[Xenia Ragiadakou]

Add the function prototypes of the functions below in <asm/processor.h> header
file so that they are visible before the function definitions in traps.c.
enter_hypervisor_from_guest_preirq()
enter_hypervisor_from_guest()
do_trap_hyp_sync()
do_trap_guest_sync()
do_trap_irq()
do_trap_fiq()
leave_hypervisor_to_guest()

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/processor.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index c021160412..74cc07028f 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -576,10 +576,24 @@ void vcpu_regs_hyp_to_user(const struct vcpu *vcpu,
 void vcpu_regs_user_to_hyp(struct vcpu *vcpu,
                            const struct vcpu_guest_core_regs *regs);
 
+void enter_hypervisor_from_guest_preirq(void);
+
+void enter_hypervisor_from_guest(void);
+
+void do_trap_hyp_sync(struct cpu_user_regs *regs);
+
+void do_trap_guest_sync(struct cpu_user_regs *regs);
+
 void do_trap_hyp_serror(struct cpu_user_regs *regs);
 
 void do_trap_guest_serror(struct cpu_user_regs *regs);
 
+void do_trap_irq(struct cpu_user_regs *regs);
+
+void do_trap_fiq(struct cpu_user_regs *regs);
+
+void leave_hypervisor_to_guest(void);
+
 register_t get_default_hcr_flags(void);
 
 /*
-- 
2.34.1

[PATCH 2/4] xen/common: time: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

The variable __mon_lengths is referenced only in time.c.
Change its linkage from external to internal by adding the storage-class
specifier static to its definitions.

Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/common/time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/time.c b/xen/common/time.c
index 22379f4ae2..92f7b72464 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -28,7 +28,7 @@
   ((year) % 4 == 0 && ((year) % 100 != 0 || (year) % 400 == 0))
 
 /* How many days are in each month.  */
-const unsigned short int __mon_lengths[2][12] = {
+static const unsigned short int __mon_lengths[2][12] = {
     /* Normal years.  */
     {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31},
     /* Leap years.  */
-- 
2.34.1

[PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

The function idle_loop() is referenced only in domain.c.
Change its linkage from external to internal by adding the storage-class
specifier static to its definitions.

Since idle_loop() is referenced only in inline assembly, add the 'used'
attribute to suppress unused-function compiler warning.

Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2f8eaab7b5..2fa67266d0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
     rcu_idle_exit(cpu);
 }
 
-void idle_loop(void)
+static __used void idle_loop(void)
 {
     unsigned int cpu = smp_processor_id();
 
-- 
2.34.1

[PATCH 4/4] xen/char: pv_console: Fix MISRA C 2012 Rule 8.4 violation

[Xenia Ragiadakou]

The function pv_console_evtchn() is defined in the header <xen/pv_console.h>.
If the header happens to be included by multiple files, this can result in
linker errors due to multiple definitions,
So, it is more appropriate to resolve this MISRA C 2012 Rule 8.4 violation
warning by making pv_console_evtchn() inline with internal linkage.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/include/xen/pv_console.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/pv_console.h b/xen/include/xen/pv_console.h
index 4745f46f2d..a96a6368b1 100644
--- a/xen/include/xen/pv_console.h
+++ b/xen/include/xen/pv_console.h
@@ -19,7 +19,7 @@ static inline void pv_console_set_rx_handler(serial_rx_fn fn) { }
 static inline void pv_console_init_postirq(void) { }
 static inline void pv_console_puts(const char *buf, size_t nr) { }
 static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; }
-evtchn_port_t pv_console_evtchn(void)
+static inline evtchn_port_t pv_console_evtchn(void)
 {
     ASSERT_UNREACHABLE();
     return 0;
-- 
2.34.1

Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

[Julien Grall]

Hi Xenia,

On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> Add the function prototypes of the functions below in <asm/processor.h> header
> file so that they are visible before the function definitions in traps.c.
> enter_hypervisor_from_guest_preirq()
> enter_hypervisor_from_guest()
> do_trap_hyp_sync()
> do_trap_guest_sync()
> do_trap_irq()
> do_trap_fiq()
> leave_hypervisor_to_guest()

I have actually came across those missing prototypes in the past and I 
have refrained to add them because they are not meant to be called from C.

While I agree this is a violation of Misra C, I am still not convinced 
they should be added (there is no need to verify the prototype).

So IMHO, they should be marked as deviation.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/4] xen/common: time: Fix MISRA C 2012 Rule 8.7 violation

[Julien Grall]

Hi Xenia,

On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> The variable __mon_lengths is referenced only in time.c.
> Change its linkage from external to internal by adding the storage-class
> specifier static to its definitions.
> 
> Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
>   xen/common/time.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/time.c b/xen/common/time.c
> index 22379f4ae2..92f7b72464 100644
> --- a/xen/common/time.c
> +++ b/xen/common/time.c
> @@ -28,7 +28,7 @@
>     ((year) % 4 == 0 && ((year) % 100 != 0 || (year) % 400 == 0))
>   
>   /* How many days are in each month.  */
> -const unsigned short int __mon_lengths[2][12] = {
> +static const unsigned short int __mon_lengths[2][12] = {
>       /* Normal years.  */
>       {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31},
>       /* Leap years.  */

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Julien Grall]

Hi Xenia,

On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> The function idle_loop() is referenced only in domain.c.
> Change its linkage from external to internal by adding the storage-class
> specifier static to its definitions.
> 
> Since idle_loop() is referenced only in inline assembly, add the 'used'
> attribute to suppress unused-function compiler warning.
> 
> Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
>   xen/arch/arm/domain.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2f8eaab7b5..2fa67266d0 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -63,7 +63,7 @@ static void do_idle(void)
>       rcu_idle_exit(cpu);
>   }
>   
> -void idle_loop(void)
> +static __used void idle_loop(void)
>   {
>       unsigned int cpu = smp_processor_id();
>   

-- 
Julien Grall

Re: [PATCH 4/4] xen/char: pv_console: Fix MISRA C 2012 Rule 8.4 violation

[Julien Grall]

Hi Xenia,

On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> The function pv_console_evtchn() is defined in the header <xen/pv_console.h>.
> If the header happens to be included by multiple files, this can result in
> linker errors due to multiple definitions,
> So, it is more appropriate to resolve this MISRA C 2012 Rule 8.4 violation
> warning by making pv_console_evtchn() inline with internal linkage.

There are multiple version of pv_console_evtchn(). The version below is 
only used when !CONFIG_XEN_GUEST.

The header is also included multiple time within Xen. So I am a bit 
puzzled why we haven't seen this error before. Maybe this is unused when 
!CONFIG_XEN_GUEST?

If yes, I would remove the call. If no, then I think the commit message 
should be clarified.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

[Xenia Ragiadakou]

Hi Julien,

On 7/6/22 00:28, Julien Grall wrote:
> Hi Xenia,
> 
> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>> Add the function prototypes of the functions below in 
>> <asm/processor.h> header
>> file so that they are visible before the function definitions in traps.c.
>> enter_hypervisor_from_guest_preirq()
>> enter_hypervisor_from_guest()
>> do_trap_hyp_sync()
>> do_trap_guest_sync()
>> do_trap_irq()
>> do_trap_fiq()
>> leave_hypervisor_to_guest()
> 
> I have actually came across those missing prototypes in the past and I 
> have refrained to add them because they are not meant to be called from C.
> 
> While I agree this is a violation of Misra C, I am still not convinced 
> they should be added (there is no need to verify the prototype).

Yes, there is no need. Here, I decided to follow the example of 
do_trap_hyp_serror() and do_trap_guest_serror() for consistency.

> 
> So IMHO, they should be marked as deviation.
> 
> Cheers,
> 

-- 
Xenia

Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

[Julien Grall]

Hi Xenia,

On 05/07/2022 22:49, Xenia Ragiadakou wrote:
> On 7/6/22 00:28, Julien Grall wrote:
>> Hi Xenia,
>>
>> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>>> Add the function prototypes of the functions below in 
>>> <asm/processor.h> header
>>> file so that they are visible before the function definitions in 
>>> traps.c.
>>> enter_hypervisor_from_guest_preirq()
>>> enter_hypervisor_from_guest()
>>> do_trap_hyp_sync()
>>> do_trap_guest_sync()
>>> do_trap_irq()
>>> do_trap_fiq()
>>> leave_hypervisor_to_guest()
>>
>> I have actually came across those missing prototypes in the past and I 
>> have refrained to add them because they are not meant to be called 
>> from C.
>>
>> While I agree this is a violation of Misra C, I am still not convinced 
>> they should be added (there is no need to verify the prototype).
> 
> Yes, there is no need. Here, I decided to follow the example of 
> do_trap_hyp_serror() and do_trap_guest_serror() for consistency.

do_trap_guest_serror() is called from C code (see arch/arm32/traps.c).

do_trap_hyp_serror() is called only from assembly. I would argue that if 
you want consistency, then you should drop the prototype rather than 
modifying 90% of the other examples.

Otherwise, this is not really consistency but more a design choice (you 
want to be fully compliant with MISRA).

Cheers,

-- 
Julien Grall

Re: [PATCH 4/4] xen/char: pv_console: Fix MISRA C 2012 Rule 8.4 violation

[Xenia Ragiadakou]

Hi Julien,

On 7/6/22 00:38, Julien Grall wrote:
> Hi Xenia,
> 
> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>> The function pv_console_evtchn() is defined in the header 
>> <xen/pv_console.h>.
>> If the header happens to be included by multiple files, this can 
>> result in
>> linker errors due to multiple definitions,
>> So, it is more appropriate to resolve this MISRA C 2012 Rule 8.4 
>> violation
>> warning by making pv_console_evtchn() inline with internal linkage.
> 
> There are multiple version of pv_console_evtchn(). The version below is 
> only used when !CONFIG_XEN_GUEST.
> 
> The header is also included multiple time within Xen. So I am a bit 
> puzzled why we haven't seen this error before. Maybe this is unused when 
> !CONFIG_XEN_GUEST?
> 
> If yes, I would remove the call. If no, then I think the commit message 
> should be clarified.

You are right. I had to clarify that this holds when !CONFIG_XEN_GUEST.
So when !CONFIG_XEN_GUEST, the function pv_console_evtchn() is defined 
inside the header file and the header is included only by a single file. 
This is why the error has not been triggered.

> 
> Cheers,
> 

-- 
Xenia

Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

[Stefano Stabellini]

On Tue, 5 Jul 2022, Julien Grall wrote:
> Hi Xenia,
> 
> On 05/07/2022 22:49, Xenia Ragiadakou wrote:
> > On 7/6/22 00:28, Julien Grall wrote:
> > > Hi Xenia,
> > > 
> > > On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> > > > Add the function prototypes of the functions below in <asm/processor.h>
> > > > header
> > > > file so that they are visible before the function definitions in
> > > > traps.c.
> > > > enter_hypervisor_from_guest_preirq()
> > > > enter_hypervisor_from_guest()
> > > > do_trap_hyp_sync()
> > > > do_trap_guest_sync()
> > > > do_trap_irq()
> > > > do_trap_fiq()
> > > > leave_hypervisor_to_guest()
> > > 
> > > I have actually came across those missing prototypes in the past and I
> > > have refrained to add them because they are not meant to be called from C.
> > > 
> > > While I agree this is a violation of Misra C, I am still not convinced
> > > they should be added (there is no need to verify the prototype).
> > 
> > Yes, there is no need. Here, I decided to follow the example of
> > do_trap_hyp_serror() and do_trap_guest_serror() for consistency.
> 
> do_trap_guest_serror() is called from C code (see arch/arm32/traps.c).
> 
> do_trap_hyp_serror() is called only from assembly. I would argue that if you
> want consistency, then you should drop the prototype rather than modifying 90%
> of the other examples.
> 
> Otherwise, this is not really consistency but more a design choice (you want
> to be fully compliant with MISRA).

Actually I am not sure that MISRA requires prototypes for assembly
functions only meant to be called from assembly. I think MISRA requires
detailed docs for anything assembly, but I don't think it requires C
prototypes for all assembly functions.

So I think we could skip them for now. At some point we'll discuss what
we need to do for the assembly code but we haven't tackled that yet.

Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

[Xenia Ragiadakou]

Hi Julien,

On 7/6/22 00:56, Julien Grall wrote:
> On 05/07/2022 22:49, Xenia Ragiadakou wrote:
>> On 7/6/22 00:28, Julien Grall wrote:
>>> Hi Xenia,
>>>
>>> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>>>> Add the function prototypes of the functions below in 
>>>> <asm/processor.h> header
>>>> file so that they are visible before the function definitions in 
>>>> traps.c.
>>>> enter_hypervisor_from_guest_preirq()
>>>> enter_hypervisor_from_guest()
>>>> do_trap_hyp_sync()
>>>> do_trap_guest_sync()
>>>> do_trap_irq()
>>>> do_trap_fiq()
>>>> leave_hypervisor_to_guest()
>>>
>>> I have actually came across those missing prototypes in the past and 
>>> I have refrained to add them because they are not meant to be called 
>>> from C.
>>>
>>> While I agree this is a violation of Misra C, I am still not 
>>> convinced they should be added (there is no need to verify the 
>>> prototype).
>>
>> Yes, there is no need. Here, I decided to follow the example of 
>> do_trap_hyp_serror() and do_trap_guest_serror() for consistency.
> 
> do_trap_guest_serror() is called from C code (see arch/arm32/traps.c).

You are right, my mistake.

> 
> do_trap_hyp_serror() is called only from assembly. I would argue that if 
> you want consistency, then you should drop the prototype rather than 
> modifying 90% of the other examples.
> 
> Otherwise, this is not really consistency but more a design choice (you 
> want to be fully compliant with MISRA).

I agree with you that there is no need to declare the variables and 
functions that are called only from assembly, prior to their 
definitions. So, this kind of violations could be grouped together and 
provide a formal deviation. This seems to me reasonable.

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 05.07.2022 23:02, Xenia Ragiadakou wrote:
> The function idle_loop() is referenced only in domain.c.
> Change its linkage from external to internal by adding the storage-class
> specifier static to its definitions.
> 
> Since idle_loop() is referenced only in inline assembly, add the 'used'
> attribute to suppress unused-function compiler warning.

While I see that Julien has already acked the patch, I'd like to point
out that using __used here is somewhat bogus. Imo the better approach
is to properly make visible to the compiler that the symbol is used by
the asm(), by adding a fake(?) input.

Jan

Re: [PATCH 4/4] xen/char: pv_console: Fix MISRA C 2012 Rule 8.4 violation

[Jan Beulich]

On 06.07.2022 00:02, Xenia Ragiadakou wrote:
> Hi Julien,
> 
> On 7/6/22 00:38, Julien Grall wrote:
>> Hi Xenia,
>>
>> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>>> The function pv_console_evtchn() is defined in the header 
>>> <xen/pv_console.h>.
>>> If the header happens to be included by multiple files, this can 
>>> result in
>>> linker errors due to multiple definitions,
>>> So, it is more appropriate to resolve this MISRA C 2012 Rule 8.4 
>>> violation
>>> warning by making pv_console_evtchn() inline with internal linkage.
>>
>> There are multiple version of pv_console_evtchn(). The version below is 
>> only used when !CONFIG_XEN_GUEST.
>>
>> The header is also included multiple time within Xen. So I am a bit 
>> puzzled why we haven't seen this error before. Maybe this is unused when 
>> !CONFIG_XEN_GUEST?
>>
>> If yes, I would remove the call. If no, then I think the commit message 
>> should be clarified.
> 
> You are right. I had to clarify that this holds when !CONFIG_XEN_GUEST.
> So when !CONFIG_XEN_GUEST, the function pv_console_evtchn() is defined 
> inside the header file and the header is included only by a single file. 
> This is why the error has not been triggered.

Irrespective of that it is as Julien suspects: The function is only ever
referenced when XEN_GUEST. Hence the better course of action indeed looks
to be to ditch the unused stub; we don't even need DCE here for there to
not be any references.

Jan

Re: [PATCH 4/4] xen/char: pv_console: Fix MISRA C 2012 Rule 8.4 violation

[Xenia Ragiadakou]

On 7/6/22 10:18, Jan Beulich wrote:
> On 06.07.2022 00:02, Xenia Ragiadakou wrote:
>> Hi Julien,
>>
>> On 7/6/22 00:38, Julien Grall wrote:
>>> Hi Xenia,
>>>
>>> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>>>> The function pv_console_evtchn() is defined in the header
>>>> <xen/pv_console.h>.
>>>> If the header happens to be included by multiple files, this can
>>>> result in
>>>> linker errors due to multiple definitions,
>>>> So, it is more appropriate to resolve this MISRA C 2012 Rule 8.4
>>>> violation
>>>> warning by making pv_console_evtchn() inline with internal linkage.
>>>
>>> There are multiple version of pv_console_evtchn(). The version below is
>>> only used when !CONFIG_XEN_GUEST.
>>>
>>> The header is also included multiple time within Xen. So I am a bit
>>> puzzled why we haven't seen this error before. Maybe this is unused when
>>> !CONFIG_XEN_GUEST?
>>>
>>> If yes, I would remove the call. If no, then I think the commit message
>>> should be clarified.
>>
>> You are right. I had to clarify that this holds when !CONFIG_XEN_GUEST.
>> So when !CONFIG_XEN_GUEST, the function pv_console_evtchn() is defined
>> inside the header file and the header is included only by a single file.
>> This is why the error has not been triggered.
> 
> Irrespective of that it is as Julien suspects: The function is only ever
> referenced when XEN_GUEST. Hence the better course of action indeed looks
> to be to ditch the unused stub; we don't even need DCE here for there to
> not be any references.

Yes, if the function is not used at all when XEN_GUEST, then I think its 
definition is considered unreachable code (Rule 2.1) and needs to be 
removed.

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

Hi Jan,

On 7/6/22 10:10, Jan Beulich wrote:
> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>> The function idle_loop() is referenced only in domain.c.
>> Change its linkage from external to internal by adding the storage-class
>> specifier static to its definitions.
>>
>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>> attribute to suppress unused-function compiler warning.
> 
> While I see that Julien has already acked the patch, I'd like to point
> out that using __used here is somewhat bogus. Imo the better approach
> is to properly make visible to the compiler that the symbol is used by
> the asm(), by adding a fake(?) input.

I 'm afraid I do not understand what do you mean by "adding a fake(?) 
input". Can you please elaborate a little on your suggestion?

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> On 7/6/22 10:10, Jan Beulich wrote:
>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>> The function idle_loop() is referenced only in domain.c.
>>> Change its linkage from external to internal by adding the storage-class
>>> specifier static to its definitions.
>>>
>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>> attribute to suppress unused-function compiler warning.
>>
>> While I see that Julien has already acked the patch, I'd like to point
>> out that using __used here is somewhat bogus. Imo the better approach
>> is to properly make visible to the compiler that the symbol is used by
>> the asm(), by adding a fake(?) input.
> 
> I 'm afraid I do not understand what do you mean by "adding a fake(?) 
> input". Can you please elaborate a little on your suggestion?

Once the asm() in question takes the function as an input, the compiler
will know that the function has a user (unless, of course, it finds a
way to elide the asm() itself). The "fake(?)" was because I'm not deeply
enough into Arm inline assembly to know whether the input could then
also be used as an instruction operand (which imo would be preferable) -
if it can't (e.g. because there's no suitable constraint or operand
modifier), it still can be an input just to inform the compiler.

Jan

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

Hi Jan,

On 7/6/22 11:51, Jan Beulich wrote:
> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>> On 7/6/22 10:10, Jan Beulich wrote:
>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>> The function idle_loop() is referenced only in domain.c.
>>>> Change its linkage from external to internal by adding the storage-class
>>>> specifier static to its definitions.
>>>>
>>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>>> attribute to suppress unused-function compiler warning.
>>>
>>> While I see that Julien has already acked the patch, I'd like to point
>>> out that using __used here is somewhat bogus. Imo the better approach
>>> is to properly make visible to the compiler that the symbol is used by
>>> the asm(), by adding a fake(?) input.
>>
>> I 'm afraid I do not understand what do you mean by "adding a fake(?)
>> input". Can you please elaborate a little on your suggestion?
> 
> Once the asm() in question takes the function as an input, the compiler
> will know that the function has a user (unless, of course, it finds a
> way to elide the asm() itself). The "fake(?)" was because I'm not deeply
> enough into Arm inline assembly to know whether the input could then
> also be used as an instruction operand (which imo would be preferable) -
> if it can't (e.g. because there's no suitable constraint or operand
> modifier), it still can be an input just to inform the compiler.

According to the following statement, your approach is the recommended one:
"To prevent the compiler from removing global data or functions which 
are referenced from inline assembly statements, you can:
-use __attribute__((used)) with the global data or functions.
-pass the reference to global data or functions as operands to inline 
assembly statements.
Arm recommends passing the reference to global data or functions as 
operands to inline assembly statements so that if the final image does 
not require the inline assembly statements and the referenced global 
data or function, then they can be removed."

IIUC, you are suggesting to change
asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
into
asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );

This gives, respectively:
reset_stack_and_jump(idle_loop);

      460:	9100001f 	mov	sp, x0

      464:	14000007 	b	480 <idle_loop>


reset_stack_and_jump(idle_loop);

      460:	9100001f 	mov	sp, x0

      464:	14000000 	b	600 <idle_loop>


With this change, the functions return_to_new_vcpu32 and 
return_to_new_vcpu64, implemented in assembly and called in the same way 
as idle_loop(), need to be declared.

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 07.07.2022 09:27, Xenia Ragiadakou wrote:
> On 7/6/22 11:51, Jan Beulich wrote:
>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>> The function idle_loop() is referenced only in domain.c.
>>>>> Change its linkage from external to internal by adding the storage-class
>>>>> specifier static to its definitions.
>>>>>
>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>>>> attribute to suppress unused-function compiler warning.
>>>>
>>>> While I see that Julien has already acked the patch, I'd like to point
>>>> out that using __used here is somewhat bogus. Imo the better approach
>>>> is to properly make visible to the compiler that the symbol is used by
>>>> the asm(), by adding a fake(?) input.
>>>
>>> I 'm afraid I do not understand what do you mean by "adding a fake(?)
>>> input". Can you please elaborate a little on your suggestion?
>>
>> Once the asm() in question takes the function as an input, the compiler
>> will know that the function has a user (unless, of course, it finds a
>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply
>> enough into Arm inline assembly to know whether the input could then
>> also be used as an instruction operand (which imo would be preferable) -
>> if it can't (e.g. because there's no suitable constraint or operand
>> modifier), it still can be an input just to inform the compiler.
> 
> According to the following statement, your approach is the recommended one:
> "To prevent the compiler from removing global data or functions which 
> are referenced from inline assembly statements, you can:
> -use __attribute__((used)) with the global data or functions.
> -pass the reference to global data or functions as operands to inline 
> assembly statements.
> Arm recommends passing the reference to global data or functions as 
> operands to inline assembly statements so that if the final image does 
> not require the inline assembly statements and the referenced global 
> data or function, then they can be removed."
> 
> IIUC, you are suggesting to change
> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> into
> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );

Yes, except that I can't judge about the "S" constraint.

> This gives, respectively:
> reset_stack_and_jump(idle_loop);
> 
>       460:	9100001f 	mov	sp, x0
> 
>       464:	14000007 	b	480 <idle_loop>
> 
> 
> reset_stack_and_jump(idle_loop);
> 
>       460:	9100001f 	mov	sp, x0
> 
>       464:	14000000 	b	600 <idle_loop>
> 
> 
> With this change, the functions return_to_new_vcpu32 and 
> return_to_new_vcpu64, implemented in assembly and called in the same way 
> as idle_loop(), need to be declared.

Which imo is a good thing - these probably shouldn't have lived entirely
behind the back of the compiler.

Jan

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

On 7/7/22 10:55, Jan Beulich wrote:
> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>> On 7/6/22 11:51, Jan Beulich wrote:
>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>>> The function idle_loop() is referenced only in domain.c.
>>>>>> Change its linkage from external to internal by adding the storage-class
>>>>>> specifier static to its definitions.
>>>>>>
>>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>>>>> attribute to suppress unused-function compiler warning.
>>>>>
>>>>> While I see that Julien has already acked the patch, I'd like to point
>>>>> out that using __used here is somewhat bogus. Imo the better approach
>>>>> is to properly make visible to the compiler that the symbol is used by
>>>>> the asm(), by adding a fake(?) input.
>>>>
>>>> I 'm afraid I do not understand what do you mean by "adding a fake(?)
>>>> input". Can you please elaborate a little on your suggestion?
>>>
>>> Once the asm() in question takes the function as an input, the compiler
>>> will know that the function has a user (unless, of course, it finds a
>>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply
>>> enough into Arm inline assembly to know whether the input could then
>>> also be used as an instruction operand (which imo would be preferable) -
>>> if it can't (e.g. because there's no suitable constraint or operand
>>> modifier), it still can be an input just to inform the compiler.
>>
>> According to the following statement, your approach is the recommended one:
>> "To prevent the compiler from removing global data or functions which
>> are referenced from inline assembly statements, you can:
>> -use __attribute__((used)) with the global data or functions.
>> -pass the reference to global data or functions as operands to inline
>> assembly statements.
>> Arm recommends passing the reference to global data or functions as
>> operands to inline assembly statements so that if the final image does
>> not require the inline assembly statements and the referenced global
>> data or function, then they can be removed."
>>
>> IIUC, you are suggesting to change
>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
>> into
>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
> 
> Yes, except that I can't judge about the "S" constraint.
> 

This constraint does not work for arm32. Any other thoughts?

Another way, as Jan suggested, is to pass the function as a 'fake' 
(unused) input.
I 'm suspecting something like the following could work
asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory")
What do you think?

>> This gives, respectively:
>> reset_stack_and_jump(idle_loop);
>>
>>        460:	9100001f 	mov	sp, x0
>>
>>        464:	14000007 	b	480 <idle_loop>
>>
>>
>> reset_stack_and_jump(idle_loop);
>>
>>        460:	9100001f 	mov	sp, x0
>>
>>        464:	14000000 	b	600 <idle_loop>
>>
>>
>> With this change, the functions return_to_new_vcpu32 and
>> return_to_new_vcpu64, implemented in assembly and called in the same way
>> as idle_loop(), need to be declared.
> 
> Which imo is a good thing - these probably shouldn't have lived entirely
> behind the back of the compiler.
> 
> Jan

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> On 7/7/22 10:55, Jan Beulich wrote:
>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>>> On 7/6/22 11:51, Jan Beulich wrote:
>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>>>> The function idle_loop() is referenced only in domain.c.
>>>>>>> Change its linkage from external to internal by adding the storage-class
>>>>>>> specifier static to its definitions.
>>>>>>>
>>>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>>>>>> attribute to suppress unused-function compiler warning.
>>>>>>
>>>>>> While I see that Julien has already acked the patch, I'd like to point
>>>>>> out that using __used here is somewhat bogus. Imo the better approach
>>>>>> is to properly make visible to the compiler that the symbol is used by
>>>>>> the asm(), by adding a fake(?) input.
>>>>>
>>>>> I 'm afraid I do not understand what do you mean by "adding a fake(?)
>>>>> input". Can you please elaborate a little on your suggestion?
>>>>
>>>> Once the asm() in question takes the function as an input, the compiler
>>>> will know that the function has a user (unless, of course, it finds a
>>>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply
>>>> enough into Arm inline assembly to know whether the input could then
>>>> also be used as an instruction operand (which imo would be preferable) -
>>>> if it can't (e.g. because there's no suitable constraint or operand
>>>> modifier), it still can be an input just to inform the compiler.
>>>
>>> According to the following statement, your approach is the recommended one:
>>> "To prevent the compiler from removing global data or functions which
>>> are referenced from inline assembly statements, you can:
>>> -use __attribute__((used)) with the global data or functions.
>>> -pass the reference to global data or functions as operands to inline
>>> assembly statements.
>>> Arm recommends passing the reference to global data or functions as
>>> operands to inline assembly statements so that if the final image does
>>> not require the inline assembly statements and the referenced global
>>> data or function, then they can be removed."
>>>
>>> IIUC, you are suggesting to change
>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
>>> into
>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
>>
>> Yes, except that I can't judge about the "S" constraint.
>>
> 
> This constraint does not work for arm32. Any other thoughts?
> 
> Another way, as Jan suggested, is to pass the function as a 'fake' 
> (unused) input.
> I 'm suspecting something like the following could work
> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory")
> What do you think?

Well, yes, X should always be a fallback option. But I said already when
you suggested S that I can't judge about its use, so I guess I'm the
wrong one to ask. Even more so that you only say "does not work", without
any details ...

Jan

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

On 7/25/22 09:32, Jan Beulich wrote:
> On 24.07.2022 19:20, Xenia Ragiadakou wrote:
>> On 7/7/22 10:55, Jan Beulich wrote:
>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>>>> On 7/6/22 11:51, Jan Beulich wrote:
>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>>>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>>>>> The function idle_loop() is referenced only in domain.c.
>>>>>>>> Change its linkage from external to internal by adding the storage-class
>>>>>>>> specifier static to its definitions.
>>>>>>>>
>>>>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>>>>>>> attribute to suppress unused-function compiler warning.
>>>>>>>
>>>>>>> While I see that Julien has already acked the patch, I'd like to point
>>>>>>> out that using __used here is somewhat bogus. Imo the better approach
>>>>>>> is to properly make visible to the compiler that the symbol is used by
>>>>>>> the asm(), by adding a fake(?) input.
>>>>>>
>>>>>> I 'm afraid I do not understand what do you mean by "adding a fake(?)
>>>>>> input". Can you please elaborate a little on your suggestion?
>>>>>
>>>>> Once the asm() in question takes the function as an input, the compiler
>>>>> will know that the function has a user (unless, of course, it finds a
>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply
>>>>> enough into Arm inline assembly to know whether the input could then
>>>>> also be used as an instruction operand (which imo would be preferable) -
>>>>> if it can't (e.g. because there's no suitable constraint or operand
>>>>> modifier), it still can be an input just to inform the compiler.
>>>>
>>>> According to the following statement, your approach is the recommended one:
>>>> "To prevent the compiler from removing global data or functions which
>>>> are referenced from inline assembly statements, you can:
>>>> -use __attribute__((used)) with the global data or functions.
>>>> -pass the reference to global data or functions as operands to inline
>>>> assembly statements.
>>>> Arm recommends passing the reference to global data or functions as
>>>> operands to inline assembly statements so that if the final image does
>>>> not require the inline assembly statements and the referenced global
>>>> data or function, then they can be removed."
>>>>
>>>> IIUC, you are suggesting to change
>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
>>>> into
>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
>>>
>>> Yes, except that I can't judge about the "S" constraint.
>>>
>>
>> This constraint does not work for arm32. Any other thoughts?
>>
>> Another way, as Jan suggested, is to pass the function as a 'fake'
>> (unused) input.
>> I 'm suspecting something like the following could work
>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory")
>> What do you think?
> 
> Well, yes, X should always be a fallback option. But I said already when
> you suggested S that I can't judge about its use, so I guess I'm the
> wrong one to ask. Even more so that you only say "does not work", without
> any details ...
> 

The question is addressed to anyone familiar with arm inline assembly.
I added the arm maintainers as primary recipients to this email to make 
this perfectly clear.

When cross-compiling Xen on x86 for arm32 with
asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
compilation fails with the error: impossible constraint in ‘asm'

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6608 bytes --]

On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
> On 7/25/22 09:32, Jan Beulich wrote:
> > On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> > > On 7/7/22 10:55, Jan Beulich wrote:
> > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote:
> > > > > On 7/6/22 11:51, Jan Beulich wrote:
> > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> > > > > > > On 7/6/22 10:10, Jan Beulich wrote:
> > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote:
> > > > > > > > > The function idle_loop() is referenced only in domain.c.
> > > > > > > > > Change its linkage from external to internal by adding the
> > > > > > > > > storage-class
> > > > > > > > > specifier static to its definitions.
> > > > > > > > > 
> > > > > > > > > Since idle_loop() is referenced only in inline assembly, add
> > > > > > > > > the 'used'
> > > > > > > > > attribute to suppress unused-function compiler warning.
> > > > > > > > 
> > > > > > > > While I see that Julien has already acked the patch, I'd like to
> > > > > > > > point
> > > > > > > > out that using __used here is somewhat bogus. Imo the better
> > > > > > > > approach
> > > > > > > > is to properly make visible to the compiler that the symbol is
> > > > > > > > used by
> > > > > > > > the asm(), by adding a fake(?) input.
> > > > > > > 
> > > > > > > I 'm afraid I do not understand what do you mean by "adding a
> > > > > > > fake(?)
> > > > > > > input". Can you please elaborate a little on your suggestion?
> > > > > > 
> > > > > > Once the asm() in question takes the function as an input, the
> > > > > > compiler
> > > > > > will know that the function has a user (unless, of course, it finds
> > > > > > a
> > > > > > way to elide the asm() itself). The "fake(?)" was because I'm not
> > > > > > deeply
> > > > > > enough into Arm inline assembly to know whether the input could then
> > > > > > also be used as an instruction operand (which imo would be
> > > > > > preferable) -
> > > > > > if it can't (e.g. because there's no suitable constraint or operand
> > > > > > modifier), it still can be an input just to inform the compiler.
> > > > > 
> > > > > According to the following statement, your approach is the recommended
> > > > > one:
> > > > > "To prevent the compiler from removing global data or functions which
> > > > > are referenced from inline assembly statements, you can:
> > > > > -use __attribute__((used)) with the global data or functions.
> > > > > -pass the reference to global data or functions as operands to inline
> > > > > assembly statements.
> > > > > Arm recommends passing the reference to global data or functions as
> > > > > operands to inline assembly statements so that if the final image does
> > > > > not require the inline assembly statements and the referenced global
> > > > > data or function, then they can be removed."
> > > > > 
> > > > > IIUC, you are suggesting to change
> > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> > > > > into
> > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
> > > > > );
> > > > 
> > > > Yes, except that I can't judge about the "S" constraint.
> > > > 
> > > 
> > > This constraint does not work for arm32. Any other thoughts?
> > > 
> > > Another way, as Jan suggested, is to pass the function as a 'fake'
> > > (unused) input.
> > > I 'm suspecting something like the following could work
> > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
> > > "memory")
> > > What do you think?
> > 
> > Well, yes, X should always be a fallback option. But I said already when
> > you suggested S that I can't judge about its use, so I guess I'm the
> > wrong one to ask. Even more so that you only say "does not work", without
> > any details ...
> > 
> 
> The question is addressed to anyone familiar with arm inline assembly.
> I added the arm maintainers as primary recipients to this email to make this
> perfectly clear.
> 
> When cross-compiling Xen on x86 for arm32 with
> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
> compilation fails with the error: impossible constraint in ‘asm'

Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
possible. The problem is that the definition of "S" changes between
aarch64 and arm (the 32-bit version).

For aarch64:

S   An absolute symbolic address or a label reference

This is what we want. For arm instead:

S   A symbol in the text segment of the current file

This is not useful for what we need to do here. As far as I can tell,
there is no other way in GCC assembly inline for arm to do this.

So we have 2 choices: we use the __used keyword as Xenia did in this
patch. Or we move the implementation of switch_stack_and_jump in
assembly. I attempted a prototype of the latter to see how it would come
out, see below.

I don't like it very much. I prefer the version with __used that Xenia
had in this patch. But I am OK either way.



diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 38826142ad..4d28f7e9f7 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -442,6 +442,10 @@ ENTRY(__context_switch)
         add     r4, r1, #VCPU_arch_saved_context
         ldmia   r4, {r4 - sl, fp, sp, pc}       /* Load registers and return */
 
+ENTRY(__switch_stack_and_jump)
+        mov sp, r0
+        bx r1
+
 /*
  * Local variables:
  * mode: ASM
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 95f1a92684..5d5d713f80 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -618,6 +618,10 @@ ENTRY(__context_switch)
         mov     sp, x9
         ret
 
+ENTRY(__switch_stack_and_jump)
+        mov sp, x0
+        br x1
+
 /*
  * Local variables:
  * mode: ASM
diff --git a/xen/arch/arm/include/asm/current.h b/xen/arch/arm/include/asm/current.h
index 73e81458e5..7696440a57 100644
--- a/xen/arch/arm/include/asm/current.h
+++ b/xen/arch/arm/include/asm/current.h
@@ -44,8 +44,12 @@ static inline struct cpu_info *get_cpu_info(void)
 
 #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
 
+void return_to_new_vcpu32(void);
+void return_to_new_vcpu64(void);
+void __switch_stack_and_jump(void *p, void *f);
+
 #define switch_stack_and_jump(stack, fn) do {                           \
-    asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ); \
+    __switch_stack_and_jump(stack, fn);                                 \
     unreachable();                                                      \
 } while ( false )
 


[1] https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 26.07.2022 02:33, Stefano Stabellini wrote:
> On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
>> On 7/25/22 09:32, Jan Beulich wrote:
>>> On 24.07.2022 19:20, Xenia Ragiadakou wrote:
>>>> On 7/7/22 10:55, Jan Beulich wrote:
>>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>>>>>> On 7/6/22 11:51, Jan Beulich wrote:
>>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>>>>>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>>>>>>> The function idle_loop() is referenced only in domain.c.
>>>>>>>>>> Change its linkage from external to internal by adding the
>>>>>>>>>> storage-class
>>>>>>>>>> specifier static to its definitions.
>>>>>>>>>>
>>>>>>>>>> Since idle_loop() is referenced only in inline assembly, add
>>>>>>>>>> the 'used'
>>>>>>>>>> attribute to suppress unused-function compiler warning.
>>>>>>>>>
>>>>>>>>> While I see that Julien has already acked the patch, I'd like to
>>>>>>>>> point
>>>>>>>>> out that using __used here is somewhat bogus. Imo the better
>>>>>>>>> approach
>>>>>>>>> is to properly make visible to the compiler that the symbol is
>>>>>>>>> used by
>>>>>>>>> the asm(), by adding a fake(?) input.
>>>>>>>>
>>>>>>>> I 'm afraid I do not understand what do you mean by "adding a
>>>>>>>> fake(?)
>>>>>>>> input". Can you please elaborate a little on your suggestion?
>>>>>>>
>>>>>>> Once the asm() in question takes the function as an input, the
>>>>>>> compiler
>>>>>>> will know that the function has a user (unless, of course, it finds
>>>>>>> a
>>>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not
>>>>>>> deeply
>>>>>>> enough into Arm inline assembly to know whether the input could then
>>>>>>> also be used as an instruction operand (which imo would be
>>>>>>> preferable) -
>>>>>>> if it can't (e.g. because there's no suitable constraint or operand
>>>>>>> modifier), it still can be an input just to inform the compiler.
>>>>>>
>>>>>> According to the following statement, your approach is the recommended
>>>>>> one:
>>>>>> "To prevent the compiler from removing global data or functions which
>>>>>> are referenced from inline assembly statements, you can:
>>>>>> -use __attribute__((used)) with the global data or functions.
>>>>>> -pass the reference to global data or functions as operands to inline
>>>>>> assembly statements.
>>>>>> Arm recommends passing the reference to global data or functions as
>>>>>> operands to inline assembly statements so that if the final image does
>>>>>> not require the inline assembly statements and the referenced global
>>>>>> data or function, then they can be removed."
>>>>>>
>>>>>> IIUC, you are suggesting to change
>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
>>>>>> into
>>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
>>>>>> );
>>>>>
>>>>> Yes, except that I can't judge about the "S" constraint.
>>>>>
>>>>
>>>> This constraint does not work for arm32. Any other thoughts?
>>>>
>>>> Another way, as Jan suggested, is to pass the function as a 'fake'
>>>> (unused) input.
>>>> I 'm suspecting something like the following could work
>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
>>>> "memory")
>>>> What do you think?
>>>
>>> Well, yes, X should always be a fallback option. But I said already when
>>> you suggested S that I can't judge about its use, so I guess I'm the
>>> wrong one to ask. Even more so that you only say "does not work", without
>>> any details ...
>>>
>>
>> The question is addressed to anyone familiar with arm inline assembly.
>> I added the arm maintainers as primary recipients to this email to make this
>> perfectly clear.
>>
>> When cross-compiling Xen on x86 for arm32 with
>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
>> compilation fails with the error: impossible constraint in ‘asm'
> 
> Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
> possible. The problem is that the definition of "S" changes between
> aarch64 and arm (the 32-bit version).
> 
> For aarch64:
> 
> S   An absolute symbolic address or a label reference
> 
> This is what we want. For arm instead:
> 
> S   A symbol in the text segment of the current file
> 
> This is not useful for what we need to do here. As far as I can tell,
> there is no other way in GCC assembly inline for arm to do this.
> 
> So we have 2 choices: we use the __used keyword as Xenia did in this
> patch. Or we move the implementation of switch_stack_and_jump in
> assembly. I attempted a prototype of the latter to see how it would come
> out, see below.
> 
> I don't like it very much. I prefer the version with __used that Xenia
> had in this patch. But I am OK either way.

You forgot the imo better intermediate option of using the "X" constraint.

Jan

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5246 bytes --]

On Tue, 26 Jul 2022, Jan Beulich wrote:
> On 26.07.2022 02:33, Stefano Stabellini wrote:
> > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
> >> On 7/25/22 09:32, Jan Beulich wrote:
> >>> On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> >>>> On 7/7/22 10:55, Jan Beulich wrote:
> >>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
> >>>>>> On 7/6/22 11:51, Jan Beulich wrote:
> >>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> >>>>>>>> On 7/6/22 10:10, Jan Beulich wrote:
> >>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
> >>>>>>>>>> The function idle_loop() is referenced only in domain.c.
> >>>>>>>>>> Change its linkage from external to internal by adding the
> >>>>>>>>>> storage-class
> >>>>>>>>>> specifier static to its definitions.
> >>>>>>>>>>
> >>>>>>>>>> Since idle_loop() is referenced only in inline assembly, add
> >>>>>>>>>> the 'used'
> >>>>>>>>>> attribute to suppress unused-function compiler warning.
> >>>>>>>>>
> >>>>>>>>> While I see that Julien has already acked the patch, I'd like to
> >>>>>>>>> point
> >>>>>>>>> out that using __used here is somewhat bogus. Imo the better
> >>>>>>>>> approach
> >>>>>>>>> is to properly make visible to the compiler that the symbol is
> >>>>>>>>> used by
> >>>>>>>>> the asm(), by adding a fake(?) input.
> >>>>>>>>
> >>>>>>>> I 'm afraid I do not understand what do you mean by "adding a
> >>>>>>>> fake(?)
> >>>>>>>> input". Can you please elaborate a little on your suggestion?
> >>>>>>>
> >>>>>>> Once the asm() in question takes the function as an input, the
> >>>>>>> compiler
> >>>>>>> will know that the function has a user (unless, of course, it finds
> >>>>>>> a
> >>>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not
> >>>>>>> deeply
> >>>>>>> enough into Arm inline assembly to know whether the input could then
> >>>>>>> also be used as an instruction operand (which imo would be
> >>>>>>> preferable) -
> >>>>>>> if it can't (e.g. because there's no suitable constraint or operand
> >>>>>>> modifier), it still can be an input just to inform the compiler.
> >>>>>>
> >>>>>> According to the following statement, your approach is the recommended
> >>>>>> one:
> >>>>>> "To prevent the compiler from removing global data or functions which
> >>>>>> are referenced from inline assembly statements, you can:
> >>>>>> -use __attribute__((used)) with the global data or functions.
> >>>>>> -pass the reference to global data or functions as operands to inline
> >>>>>> assembly statements.
> >>>>>> Arm recommends passing the reference to global data or functions as
> >>>>>> operands to inline assembly statements so that if the final image does
> >>>>>> not require the inline assembly statements and the referenced global
> >>>>>> data or function, then they can be removed."
> >>>>>>
> >>>>>> IIUC, you are suggesting to change
> >>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> >>>>>> into
> >>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
> >>>>>> );
> >>>>>
> >>>>> Yes, except that I can't judge about the "S" constraint.
> >>>>>
> >>>>
> >>>> This constraint does not work for arm32. Any other thoughts?
> >>>>
> >>>> Another way, as Jan suggested, is to pass the function as a 'fake'
> >>>> (unused) input.
> >>>> I 'm suspecting something like the following could work
> >>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
> >>>> "memory")
> >>>> What do you think?
> >>>
> >>> Well, yes, X should always be a fallback option. But I said already when
> >>> you suggested S that I can't judge about its use, so I guess I'm the
> >>> wrong one to ask. Even more so that you only say "does not work", without
> >>> any details ...
> >>>
> >>
> >> The question is addressed to anyone familiar with arm inline assembly.
> >> I added the arm maintainers as primary recipients to this email to make this
> >> perfectly clear.
> >>
> >> When cross-compiling Xen on x86 for arm32 with
> >> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
> >> compilation fails with the error: impossible constraint in ‘asm'
> > 
> > Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
> > possible. The problem is that the definition of "S" changes between
> > aarch64 and arm (the 32-bit version).
> > 
> > For aarch64:
> > 
> > S   An absolute symbolic address or a label reference
> > 
> > This is what we want. For arm instead:
> > 
> > S   A symbol in the text segment of the current file
> > 
> > This is not useful for what we need to do here. As far as I can tell,
> > there is no other way in GCC assembly inline for arm to do this.
> > 
> > So we have 2 choices: we use the __used keyword as Xenia did in this
> > patch. Or we move the implementation of switch_stack_and_jump in
> > assembly. I attempted a prototype of the latter to see how it would come
> > out, see below.
> > 
> > I don't like it very much. I prefer the version with __used that Xenia
> > had in this patch. But I am OK either way.
> 
> You forgot the imo better intermediate option of using the "X" constraint.

I couldn't get "X" to compile in any way (not even for arm64). Do you
have a concrete example that you think should work using "X" as
constraint?

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

On 7/27/22 03:10, Stefano Stabellini wrote:
> On Tue, 26 Jul 2022, Jan Beulich wrote:
>> On 26.07.2022 02:33, Stefano Stabellini wrote:
>>> On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
>>>> On 7/25/22 09:32, Jan Beulich wrote:
>>>>> On 24.07.2022 19:20, Xenia Ragiadakou wrote:
>>>>>> On 7/7/22 10:55, Jan Beulich wrote:
>>>>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>>>>>>>> On 7/6/22 11:51, Jan Beulich wrote:
>>>>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>>>>>>>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>>>>>>>>> The function idle_loop() is referenced only in domain.c.
>>>>>>>>>>>> Change its linkage from external to internal by adding the
>>>>>>>>>>>> storage-class
>>>>>>>>>>>> specifier static to its definitions.
>>>>>>>>>>>>
>>>>>>>>>>>> Since idle_loop() is referenced only in inline assembly, add
>>>>>>>>>>>> the 'used'
>>>>>>>>>>>> attribute to suppress unused-function compiler warning.
>>>>>>>>>>>
>>>>>>>>>>> While I see that Julien has already acked the patch, I'd like to
>>>>>>>>>>> point
>>>>>>>>>>> out that using __used here is somewhat bogus. Imo the better
>>>>>>>>>>> approach
>>>>>>>>>>> is to properly make visible to the compiler that the symbol is
>>>>>>>>>>> used by
>>>>>>>>>>> the asm(), by adding a fake(?) input.
>>>>>>>>>>
>>>>>>>>>> I 'm afraid I do not understand what do you mean by "adding a
>>>>>>>>>> fake(?)
>>>>>>>>>> input". Can you please elaborate a little on your suggestion?
>>>>>>>>>
>>>>>>>>> Once the asm() in question takes the function as an input, the
>>>>>>>>> compiler
>>>>>>>>> will know that the function has a user (unless, of course, it finds
>>>>>>>>> a
>>>>>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not
>>>>>>>>> deeply
>>>>>>>>> enough into Arm inline assembly to know whether the input could then
>>>>>>>>> also be used as an instruction operand (which imo would be
>>>>>>>>> preferable) -
>>>>>>>>> if it can't (e.g. because there's no suitable constraint or operand
>>>>>>>>> modifier), it still can be an input just to inform the compiler.
>>>>>>>>
>>>>>>>> According to the following statement, your approach is the recommended
>>>>>>>> one:
>>>>>>>> "To prevent the compiler from removing global data or functions which
>>>>>>>> are referenced from inline assembly statements, you can:
>>>>>>>> -use __attribute__((used)) with the global data or functions.
>>>>>>>> -pass the reference to global data or functions as operands to inline
>>>>>>>> assembly statements.
>>>>>>>> Arm recommends passing the reference to global data or functions as
>>>>>>>> operands to inline assembly statements so that if the final image does
>>>>>>>> not require the inline assembly statements and the referenced global
>>>>>>>> data or function, then they can be removed."
>>>>>>>>
>>>>>>>> IIUC, you are suggesting to change
>>>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
>>>>>>>> into
>>>>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
>>>>>>>> );
>>>>>>>
>>>>>>> Yes, except that I can't judge about the "S" constraint.
>>>>>>>
>>>>>>
>>>>>> This constraint does not work for arm32. Any other thoughts?
>>>>>>
>>>>>> Another way, as Jan suggested, is to pass the function as a 'fake'
>>>>>> (unused) input.
>>>>>> I 'm suspecting something like the following could work
>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
>>>>>> "memory")
>>>>>> What do you think?
>>>>>
>>>>> Well, yes, X should always be a fallback option. But I said already when
>>>>> you suggested S that I can't judge about its use, so I guess I'm the
>>>>> wrong one to ask. Even more so that you only say "does not work", without
>>>>> any details ...
>>>>>
>>>>
>>>> The question is addressed to anyone familiar with arm inline assembly.
>>>> I added the arm maintainers as primary recipients to this email to make this
>>>> perfectly clear.
>>>>
>>>> When cross-compiling Xen on x86 for arm32 with
>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
>>>> compilation fails with the error: impossible constraint in ‘asm'
>>>
>>> Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
>>> possible. The problem is that the definition of "S" changes between
>>> aarch64 and arm (the 32-bit version).
>>>
>>> For aarch64:
>>>
>>> S   An absolute symbolic address or a label reference
>>>
>>> This is what we want. For arm instead:
>>>
>>> S   A symbol in the text segment of the current file
>>>
>>> This is not useful for what we need to do here. As far as I can tell,
>>> there is no other way in GCC assembly inline for arm to do this.
>>>
>>> So we have 2 choices: we use the __used keyword as Xenia did in this
>>> patch. Or we move the implementation of switch_stack_and_jump in
>>> assembly. I attempted a prototype of the latter to see how it would come
>>> out, see below.
>>>
>>> I don't like it very much. I prefer the version with __used that Xenia
>>> had in this patch. But I am OK either way.
>>
>> You forgot the imo better intermediate option of using the "X" constraint.
> 
> I couldn't get "X" to compile in any way (not even for arm64). Do you
> have a concrete example that you think should work using "X" as
> constraint?

I proposed the X constraint for the case that the function is used as a 
fake (unused) input operand to the inline asm.
For instance, in the example below, the function is listed in the input 
operands but there is not corresponding reference to it.

asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : 
"memory" );

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 27.07.2022 02:10, Stefano Stabellini wrote:
> On Tue, 26 Jul 2022, Jan Beulich wrote:
>> You forgot the imo better intermediate option of using the "X" constraint.
> 
> I couldn't get "X" to compile in any way (not even for arm64). Do you
> have a concrete example that you think should work using "X" as
> constraint?

Perhaps you tried to use the respective input then as an operand to
the insn? That won't work afaik - as Xenia says, it can be used only
as a "fake" operand (i.e. one that tells the compiler something, but
having no direct meaning for the insn).

Actually I thought we had uses of "X" already somewhere in Xen and/or
XTF, but now that I looked I can't find any. (Anymore?)

Jan

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 9339 bytes --]

On Wed, 27 Jul 2022, Xenia Ragiadakou wrote:
> On 7/27/22 03:10, Stefano Stabellini wrote:
> > On Tue, 26 Jul 2022, Jan Beulich wrote:
> > > On 26.07.2022 02:33, Stefano Stabellini wrote:
> > > > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
> > > > > On 7/25/22 09:32, Jan Beulich wrote:
> > > > > > On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> > > > > > > On 7/7/22 10:55, Jan Beulich wrote:
> > > > > > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote:
> > > > > > > > > On 7/6/22 11:51, Jan Beulich wrote:
> > > > > > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> > > > > > > > > > > On 7/6/22 10:10, Jan Beulich wrote:
> > > > > > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote:
> > > > > > > > > > > > > The function idle_loop() is referenced only in
> > > > > > > > > > > > > domain.c.
> > > > > > > > > > > > > Change its linkage from external to internal by adding
> > > > > > > > > > > > > the
> > > > > > > > > > > > > storage-class
> > > > > > > > > > > > > specifier static to its definitions.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Since idle_loop() is referenced only in inline
> > > > > > > > > > > > > assembly, add
> > > > > > > > > > > > > the 'used'
> > > > > > > > > > > > > attribute to suppress unused-function compiler
> > > > > > > > > > > > > warning.
> > > > > > > > > > > > 
> > > > > > > > > > > > While I see that Julien has already acked the patch, I'd
> > > > > > > > > > > > like to
> > > > > > > > > > > > point
> > > > > > > > > > > > out that using __used here is somewhat bogus. Imo the
> > > > > > > > > > > > better
> > > > > > > > > > > > approach
> > > > > > > > > > > > is to properly make visible to the compiler that the
> > > > > > > > > > > > symbol is
> > > > > > > > > > > > used by
> > > > > > > > > > > > the asm(), by adding a fake(?) input.
> > > > > > > > > > > 
> > > > > > > > > > > I 'm afraid I do not understand what do you mean by
> > > > > > > > > > > "adding a
> > > > > > > > > > > fake(?)
> > > > > > > > > > > input". Can you please elaborate a little on your
> > > > > > > > > > > suggestion?
> > > > > > > > > > 
> > > > > > > > > > Once the asm() in question takes the function as an input,
> > > > > > > > > > the
> > > > > > > > > > compiler
> > > > > > > > > > will know that the function has a user (unless, of course,
> > > > > > > > > > it finds
> > > > > > > > > > a
> > > > > > > > > > way to elide the asm() itself). The "fake(?)" was because
> > > > > > > > > > I'm not
> > > > > > > > > > deeply
> > > > > > > > > > enough into Arm inline assembly to know whether the input
> > > > > > > > > > could then
> > > > > > > > > > also be used as an instruction operand (which imo would be
> > > > > > > > > > preferable) -
> > > > > > > > > > if it can't (e.g. because there's no suitable constraint or
> > > > > > > > > > operand
> > > > > > > > > > modifier), it still can be an input just to inform the
> > > > > > > > > > compiler.
> > > > > > > > > 
> > > > > > > > > According to the following statement, your approach is the
> > > > > > > > > recommended
> > > > > > > > > one:
> > > > > > > > > "To prevent the compiler from removing global data or
> > > > > > > > > functions which
> > > > > > > > > are referenced from inline assembly statements, you can:
> > > > > > > > > -use __attribute__((used)) with the global data or functions.
> > > > > > > > > -pass the reference to global data or functions as operands to
> > > > > > > > > inline
> > > > > > > > > assembly statements.
> > > > > > > > > Arm recommends passing the reference to global data or
> > > > > > > > > functions as
> > > > > > > > > operands to inline assembly statements so that if the final
> > > > > > > > > image does
> > > > > > > > > not require the inline assembly statements and the referenced
> > > > > > > > > global
> > > > > > > > > data or function, then they can be removed."
> > > > > > > > > 
> > > > > > > > > IIUC, you are suggesting to change
> > > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) :
> > > > > > > > > "memory" )
> > > > > > > > > into
> > > > > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) :
> > > > > > > > > "memory"
> > > > > > > > > );
> > > > > > > > 
> > > > > > > > Yes, except that I can't judge about the "S" constraint.
> > > > > > > > 
> > > > > > > 
> > > > > > > This constraint does not work for arm32. Any other thoughts?
> > > > > > > 
> > > > > > > Another way, as Jan suggested, is to pass the function as a 'fake'
> > > > > > > (unused) input.
> > > > > > > I 'm suspecting something like the following could work
> > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
> > > > > > > "memory")
> > > > > > > What do you think?
> > > > > > 
> > > > > > Well, yes, X should always be a fallback option. But I said already
> > > > > > when
> > > > > > you suggested S that I can't judge about its use, so I guess I'm the
> > > > > > wrong one to ask. Even more so that you only say "does not work",
> > > > > > without
> > > > > > any details ...
> > > > > > 
> > > > > 
> > > > > The question is addressed to anyone familiar with arm inline assembly.
> > > > > I added the arm maintainers as primary recipients to this email to
> > > > > make this
> > > > > perfectly clear.
> > > > > 
> > > > > When cross-compiling Xen on x86 for arm32 with
> > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
> > > > > );
> > > > > compilation fails with the error: impossible constraint in ‘asm'
> > > > 
> > > > Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
> > > > possible. The problem is that the definition of "S" changes between
> > > > aarch64 and arm (the 32-bit version).
> > > > 
> > > > For aarch64:
> > > > 
> > > > S   An absolute symbolic address or a label reference
> > > > 
> > > > This is what we want. For arm instead:
> > > > 
> > > > S   A symbol in the text segment of the current file
> > > > 
> > > > This is not useful for what we need to do here. As far as I can tell,
> > > > there is no other way in GCC assembly inline for arm to do this.
> > > > 
> > > > So we have 2 choices: we use the __used keyword as Xenia did in this
> > > > patch. Or we move the implementation of switch_stack_and_jump in
> > > > assembly. I attempted a prototype of the latter to see how it would come
> > > > out, see below.
> > > > 
> > > > I don't like it very much. I prefer the version with __used that Xenia
> > > > had in this patch. But I am OK either way.
> > > 
> > > You forgot the imo better intermediate option of using the "X" constraint.
> > 
> > I couldn't get "X" to compile in any way (not even for arm64). Do you
> > have a concrete example that you think should work using "X" as
> > constraint?
> 
> I proposed the X constraint for the case that the function is used as a fake
> (unused) input operand to the inline asm.
> For instance, in the example below, the function is listed in the input
> operands but there is not corresponding reference to it.
> 
> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" );


Also replying to Jan, yes, this doesn't build for me, not even for
arm64. The error is below.


arch/arm/domain.c: In function ‘continue_new_vcpu’:
arch/arm/domain.c:345:30: error: ‘return_to_new_vcpu32’ undeclared (first use in this function)
  345 |         reset_stack_and_jump(return_to_new_vcpu32);
      |                              ^~~~~~~~~~~~~~~~~~~~
./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’
   48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
      |                                                                 ^~
arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’
  345 |         reset_stack_and_jump(return_to_new_vcpu32);
      |         ^~~~~~~~~~~~~~~~~~~~
arch/arm/domain.c:345:30: note: each undeclared identifier is reported only once for each function it appears in
  345 |         reset_stack_and_jump(return_to_new_vcpu32);
      |                              ^~~~~~~~~~~~~~~~~~~~
./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’
   48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
      |                                                                 ^~
arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’
  345 |         reset_stack_and_jump(return_to_new_vcpu32);
      |         ^~~~~~~~~~~~~~~~~~~~
  CC      arch/arm/domain_build.o
arch/arm/domain.c:348:30: error: ‘return_to_new_vcpu64’ undeclared (first use in this function)
  348 |         reset_stack_and_jump(return_to_new_vcpu64);
      |                              ^~~~~~~~~~~~~~~~~~~~
./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’
   48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
      |                                                                 ^~
arch/arm/domain.c:348:9: note: in expansion of macro ‘reset_stack_and_jump’
  348 |         reset_stack_and_jump(return_to_new_vcpu64);
      |         ^~~~~~~~~~~~~~~~~~~~
make[2]: *** [Rules.mk:246: arch/arm/domain.o] Error 1

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

Hi Stefano,

On 7/27/22 23:26, Stefano Stabellini wrote:
> On Wed, 27 Jul 2022, Xenia Ragiadakou wrote:
>> On 7/27/22 03:10, Stefano Stabellini wrote:
>>> On Tue, 26 Jul 2022, Jan Beulich wrote:
>>>> On 26.07.2022 02:33, Stefano Stabellini wrote:
>>>>> On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
>>>>>> On 7/25/22 09:32, Jan Beulich wrote:
>>>>>>> On 24.07.2022 19:20, Xenia Ragiadakou wrote:
>>>>>>>> On 7/7/22 10:55, Jan Beulich wrote:
>>>>>>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>>>>>>>>>> On 7/6/22 11:51, Jan Beulich wrote:
>>>>>>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote:
>>>>>>>>>>>> On 7/6/22 10:10, Jan Beulich wrote:
>>>>>>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>>>>>>>>>>>>> The function idle_loop() is referenced only in
>>>>>>>>>>>>>> domain.c.
>>>>>>>>>>>>>> Change its linkage from external to internal by adding
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> storage-class
>>>>>>>>>>>>>> specifier static to its definitions.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Since idle_loop() is referenced only in inline
>>>>>>>>>>>>>> assembly, add
>>>>>>>>>>>>>> the 'used'
>>>>>>>>>>>>>> attribute to suppress unused-function compiler
>>>>>>>>>>>>>> warning.
>>>>>>>>>>>>>
>>>>>>>>>>>>> While I see that Julien has already acked the patch, I'd
>>>>>>>>>>>>> like to
>>>>>>>>>>>>> point
>>>>>>>>>>>>> out that using __used here is somewhat bogus. Imo the
>>>>>>>>>>>>> better
>>>>>>>>>>>>> approach
>>>>>>>>>>>>> is to properly make visible to the compiler that the
>>>>>>>>>>>>> symbol is
>>>>>>>>>>>>> used by
>>>>>>>>>>>>> the asm(), by adding a fake(?) input.
>>>>>>>>>>>>
>>>>>>>>>>>> I 'm afraid I do not understand what do you mean by
>>>>>>>>>>>> "adding a
>>>>>>>>>>>> fake(?)
>>>>>>>>>>>> input". Can you please elaborate a little on your
>>>>>>>>>>>> suggestion?
>>>>>>>>>>>
>>>>>>>>>>> Once the asm() in question takes the function as an input,
>>>>>>>>>>> the
>>>>>>>>>>> compiler
>>>>>>>>>>> will know that the function has a user (unless, of course,
>>>>>>>>>>> it finds
>>>>>>>>>>> a
>>>>>>>>>>> way to elide the asm() itself). The "fake(?)" was because
>>>>>>>>>>> I'm not
>>>>>>>>>>> deeply
>>>>>>>>>>> enough into Arm inline assembly to know whether the input
>>>>>>>>>>> could then
>>>>>>>>>>> also be used as an instruction operand (which imo would be
>>>>>>>>>>> preferable) -
>>>>>>>>>>> if it can't (e.g. because there's no suitable constraint or
>>>>>>>>>>> operand
>>>>>>>>>>> modifier), it still can be an input just to inform the
>>>>>>>>>>> compiler.
>>>>>>>>>>
>>>>>>>>>> According to the following statement, your approach is the
>>>>>>>>>> recommended
>>>>>>>>>> one:
>>>>>>>>>> "To prevent the compiler from removing global data or
>>>>>>>>>> functions which
>>>>>>>>>> are referenced from inline assembly statements, you can:
>>>>>>>>>> -use __attribute__((used)) with the global data or functions.
>>>>>>>>>> -pass the reference to global data or functions as operands to
>>>>>>>>>> inline
>>>>>>>>>> assembly statements.
>>>>>>>>>> Arm recommends passing the reference to global data or
>>>>>>>>>> functions as
>>>>>>>>>> operands to inline assembly statements so that if the final
>>>>>>>>>> image does
>>>>>>>>>> not require the inline assembly statements and the referenced
>>>>>>>>>> global
>>>>>>>>>> data or function, then they can be removed."
>>>>>>>>>>
>>>>>>>>>> IIUC, you are suggesting to change
>>>>>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) :
>>>>>>>>>> "memory" )
>>>>>>>>>> into
>>>>>>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) :
>>>>>>>>>> "memory"
>>>>>>>>>> );
>>>>>>>>>
>>>>>>>>> Yes, except that I can't judge about the "S" constraint.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This constraint does not work for arm32. Any other thoughts?
>>>>>>>>
>>>>>>>> Another way, as Jan suggested, is to pass the function as a 'fake'
>>>>>>>> (unused) input.
>>>>>>>> I 'm suspecting something like the following could work
>>>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
>>>>>>>> "memory")
>>>>>>>> What do you think?
>>>>>>>
>>>>>>> Well, yes, X should always be a fallback option. But I said already
>>>>>>> when
>>>>>>> you suggested S that I can't judge about its use, so I guess I'm the
>>>>>>> wrong one to ask. Even more so that you only say "does not work",
>>>>>>> without
>>>>>>> any details ...
>>>>>>>
>>>>>>
>>>>>> The question is addressed to anyone familiar with arm inline assembly.
>>>>>> I added the arm maintainers as primary recipients to this email to
>>>>>> make this
>>>>>> perfectly clear.
>>>>>>
>>>>>> When cross-compiling Xen on x86 for arm32 with
>>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
>>>>>> );
>>>>>> compilation fails with the error: impossible constraint in ‘asm'
>>>>>
>>>>> Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
>>>>> possible. The problem is that the definition of "S" changes between
>>>>> aarch64 and arm (the 32-bit version).
>>>>>
>>>>> For aarch64:
>>>>>
>>>>> S   An absolute symbolic address or a label reference
>>>>>
>>>>> This is what we want. For arm instead:
>>>>>
>>>>> S   A symbol in the text segment of the current file
>>>>>
>>>>> This is not useful for what we need to do here. As far as I can tell,
>>>>> there is no other way in GCC assembly inline for arm to do this.
>>>>>
>>>>> So we have 2 choices: we use the __used keyword as Xenia did in this
>>>>> patch. Or we move the implementation of switch_stack_and_jump in
>>>>> assembly. I attempted a prototype of the latter to see how it would come
>>>>> out, see below.
>>>>>
>>>>> I don't like it very much. I prefer the version with __used that Xenia
>>>>> had in this patch. But I am OK either way.
>>>>
>>>> You forgot the imo better intermediate option of using the "X" constraint.
>>>
>>> I couldn't get "X" to compile in any way (not even for arm64). Do you
>>> have a concrete example that you think should work using "X" as
>>> constraint?
>>
>> I proposed the X constraint for the case that the function is used as a fake
>> (unused) input operand to the inline asm.
>> For instance, in the example below, the function is listed in the input
>> operands but there is not corresponding reference to it.
>>
>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" );
> 
> 
> Also replying to Jan, yes, this doesn't build for me, not even for
> arm64. The error is below.
> 
> 
> arch/arm/domain.c: In function ‘continue_new_vcpu’:
> arch/arm/domain.c:345:30: error: ‘return_to_new_vcpu32’ undeclared (first use in this function)
>    345 |         reset_stack_and_jump(return_to_new_vcpu32);
>        |                              ^~~~~~~~~~~~~~~~~~~~
> ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’
>     48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
>        |                                                                 ^~
> arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’
>    345 |         reset_stack_and_jump(return_to_new_vcpu32);
>        |         ^~~~~~~~~~~~~~~~~~~~
> arch/arm/domain.c:345:30: note: each undeclared identifier is reported only once for each function it appears in
>    345 |         reset_stack_and_jump(return_to_new_vcpu32);
>        |                              ^~~~~~~~~~~~~~~~~~~~
> ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’
>     48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
>        |                                                                 ^~
> arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’
>    345 |         reset_stack_and_jump(return_to_new_vcpu32);
>        |         ^~~~~~~~~~~~~~~~~~~~
>    CC      arch/arm/domain_build.o
> arch/arm/domain.c:348:30: error: ‘return_to_new_vcpu64’ undeclared (first use in this function)
>    348 |         reset_stack_and_jump(return_to_new_vcpu64);
>        |                              ^~~~~~~~~~~~~~~~~~~~
> ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’
>     48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
>        |                                                                 ^~
> arch/arm/domain.c:348:9: note: in expansion of macro ‘reset_stack_and_jump’
>    348 |         reset_stack_and_jump(return_to_new_vcpu64);
>        |         ^~~~~~~~~~~~~~~~~~~~
> make[2]: *** [Rules.mk:246: arch/arm/domain.o] Error 1

With this change, the compiler becomes aware that the functions 
idle_loop, return_to_new_vcpu32 and return_to_new_vcpu64 are used by the 
inline assembly.
For idle loop, there is a previous declaration but for the other two 
there is not and when the compiler encounters their references complains.
So, for this to compile, it is also required to declare
return_to_new_vcpu32 and return_to_new_vcpu64.

-- 
Xenia

Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 11334 bytes --]

On Thu, 28 Jul 2022, Xenia Ragiadakou wrote:
> Hi Stefano,
> 
> On 7/27/22 23:26, Stefano Stabellini wrote:
> > On Wed, 27 Jul 2022, Xenia Ragiadakou wrote:
> > > On 7/27/22 03:10, Stefano Stabellini wrote:
> > > > On Tue, 26 Jul 2022, Jan Beulich wrote:
> > > > > On 26.07.2022 02:33, Stefano Stabellini wrote:
> > > > > > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
> > > > > > > On 7/25/22 09:32, Jan Beulich wrote:
> > > > > > > > On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> > > > > > > > > On 7/7/22 10:55, Jan Beulich wrote:
> > > > > > > > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote:
> > > > > > > > > > > On 7/6/22 11:51, Jan Beulich wrote:
> > > > > > > > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> > > > > > > > > > > > > On 7/6/22 10:10, Jan Beulich wrote:
> > > > > > > > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote:
> > > > > > > > > > > > > > > The function idle_loop() is referenced only in
> > > > > > > > > > > > > > > domain.c.
> > > > > > > > > > > > > > > Change its linkage from external to internal by
> > > > > > > > > > > > > > > adding
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > storage-class
> > > > > > > > > > > > > > > specifier static to its definitions.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Since idle_loop() is referenced only in inline
> > > > > > > > > > > > > > > assembly, add
> > > > > > > > > > > > > > > the 'used'
> > > > > > > > > > > > > > > attribute to suppress unused-function compiler
> > > > > > > > > > > > > > > warning.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > While I see that Julien has already acked the patch,
> > > > > > > > > > > > > > I'd
> > > > > > > > > > > > > > like to
> > > > > > > > > > > > > > point
> > > > > > > > > > > > > > out that using __used here is somewhat bogus. Imo
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > better
> > > > > > > > > > > > > > approach
> > > > > > > > > > > > > > is to properly make visible to the compiler that the
> > > > > > > > > > > > > > symbol is
> > > > > > > > > > > > > > used by
> > > > > > > > > > > > > > the asm(), by adding a fake(?) input.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I 'm afraid I do not understand what do you mean by
> > > > > > > > > > > > > "adding a
> > > > > > > > > > > > > fake(?)
> > > > > > > > > > > > > input". Can you please elaborate a little on your
> > > > > > > > > > > > > suggestion?
> > > > > > > > > > > > 
> > > > > > > > > > > > Once the asm() in question takes the function as an
> > > > > > > > > > > > input,
> > > > > > > > > > > > the
> > > > > > > > > > > > compiler
> > > > > > > > > > > > will know that the function has a user (unless, of
> > > > > > > > > > > > course,
> > > > > > > > > > > > it finds
> > > > > > > > > > > > a
> > > > > > > > > > > > way to elide the asm() itself). The "fake(?)" was
> > > > > > > > > > > > because
> > > > > > > > > > > > I'm not
> > > > > > > > > > > > deeply
> > > > > > > > > > > > enough into Arm inline assembly to know whether the
> > > > > > > > > > > > input
> > > > > > > > > > > > could then
> > > > > > > > > > > > also be used as an instruction operand (which imo would
> > > > > > > > > > > > be
> > > > > > > > > > > > preferable) -
> > > > > > > > > > > > if it can't (e.g. because there's no suitable constraint
> > > > > > > > > > > > or
> > > > > > > > > > > > operand
> > > > > > > > > > > > modifier), it still can be an input just to inform the
> > > > > > > > > > > > compiler.
> > > > > > > > > > > 
> > > > > > > > > > > According to the following statement, your approach is the
> > > > > > > > > > > recommended
> > > > > > > > > > > one:
> > > > > > > > > > > "To prevent the compiler from removing global data or
> > > > > > > > > > > functions which
> > > > > > > > > > > are referenced from inline assembly statements, you can:
> > > > > > > > > > > -use __attribute__((used)) with the global data or
> > > > > > > > > > > functions.
> > > > > > > > > > > -pass the reference to global data or functions as
> > > > > > > > > > > operands to
> > > > > > > > > > > inline
> > > > > > > > > > > assembly statements.
> > > > > > > > > > > Arm recommends passing the reference to global data or
> > > > > > > > > > > functions as
> > > > > > > > > > > operands to inline assembly statements so that if the
> > > > > > > > > > > final
> > > > > > > > > > > image does
> > > > > > > > > > > not require the inline assembly statements and the
> > > > > > > > > > > referenced
> > > > > > > > > > > global
> > > > > > > > > > > data or function, then they can be removed."
> > > > > > > > > > > 
> > > > > > > > > > > IIUC, you are suggesting to change
> > > > > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) :
> > > > > > > > > > > "memory" )
> > > > > > > > > > > into
> > > > > > > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn)
> > > > > > > > > > > :
> > > > > > > > > > > "memory"
> > > > > > > > > > > );
> > > > > > > > > > 
> > > > > > > > > > Yes, except that I can't judge about the "S" constraint.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This constraint does not work for arm32. Any other thoughts?
> > > > > > > > > 
> > > > > > > > > Another way, as Jan suggested, is to pass the function as a
> > > > > > > > > 'fake'
> > > > > > > > > (unused) input.
> > > > > > > > > I 'm suspecting something like the following could work
> > > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X"
> > > > > > > > > (fn) :
> > > > > > > > > "memory")
> > > > > > > > > What do you think?
> > > > > > > > 
> > > > > > > > Well, yes, X should always be a fallback option. But I said
> > > > > > > > already
> > > > > > > > when
> > > > > > > > you suggested S that I can't judge about its use, so I guess I'm
> > > > > > > > the
> > > > > > > > wrong one to ask. Even more so that you only say "does not
> > > > > > > > work",
> > > > > > > > without
> > > > > > > > any details ...
> > > > > > > > 
> > > > > > > 
> > > > > > > The question is addressed to anyone familiar with arm inline
> > > > > > > assembly.
> > > > > > > I added the arm maintainers as primary recipients to this email to
> > > > > > > make this
> > > > > > > perfectly clear.
> > > > > > > 
> > > > > > > When cross-compiling Xen on x86 for arm32 with
> > > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) :
> > > > > > > "memory"
> > > > > > > );
> > > > > > > compilation fails with the error: impossible constraint in ‘asm'
> > > > > > 
> > > > > > Unfortunately looking at the GCC manual pages [1], it doesn't seem
> > > > > > to be
> > > > > > possible. The problem is that the definition of "S" changes between
> > > > > > aarch64 and arm (the 32-bit version).
> > > > > > 
> > > > > > For aarch64:
> > > > > > 
> > > > > > S   An absolute symbolic address or a label reference
> > > > > > 
> > > > > > This is what we want. For arm instead:
> > > > > > 
> > > > > > S   A symbol in the text segment of the current file
> > > > > > 
> > > > > > This is not useful for what we need to do here. As far as I can
> > > > > > tell,
> > > > > > there is no other way in GCC assembly inline for arm to do this.
> > > > > > 
> > > > > > So we have 2 choices: we use the __used keyword as Xenia did in this
> > > > > > patch. Or we move the implementation of switch_stack_and_jump in
> > > > > > assembly. I attempted a prototype of the latter to see how it would
> > > > > > come
> > > > > > out, see below.
> > > > > > 
> > > > > > I don't like it very much. I prefer the version with __used that
> > > > > > Xenia
> > > > > > had in this patch. But I am OK either way.
> > > > > 
> > > > > You forgot the imo better intermediate option of using the "X"
> > > > > constraint.
> > > > 
> > > > I couldn't get "X" to compile in any way (not even for arm64). Do you
> > > > have a concrete example that you think should work using "X" as
> > > > constraint?
> > > 
> > > I proposed the X constraint for the case that the function is used as a
> > > fake
> > > (unused) input operand to the inline asm.
> > > For instance, in the example below, the function is listed in the input
> > > operands but there is not corresponding reference to it.
> > > 
> > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory"
> > > );
> > 
> > 
> > Also replying to Jan, yes, this doesn't build for me, not even for
> > arm64. The error is below.
> > 
> > 
> > arch/arm/domain.c: In function ‘continue_new_vcpu’:
> > arch/arm/domain.c:345:30: error: ‘return_to_new_vcpu32’ undeclared (first
> > use in this function)
> >    345 |         reset_stack_and_jump(return_to_new_vcpu32);
> >        |                              ^~~~~~~~~~~~~~~~~~~~
> > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro
> > ‘switch_stack_and_jump’
> >     48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn)
> > : "memory" ); \
> >        |                                                                 ^~
> > arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’
> >    345 |         reset_stack_and_jump(return_to_new_vcpu32);
> >        |         ^~~~~~~~~~~~~~~~~~~~
> > arch/arm/domain.c:345:30: note: each undeclared identifier is reported only
> > once for each function it appears in
> >    345 |         reset_stack_and_jump(return_to_new_vcpu32);
> >        |                              ^~~~~~~~~~~~~~~~~~~~
> > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro
> > ‘switch_stack_and_jump’
> >     48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn)
> > : "memory" ); \
> >        |                                                                 ^~
> > arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’
> >    345 |         reset_stack_and_jump(return_to_new_vcpu32);
> >        |         ^~~~~~~~~~~~~~~~~~~~
> >    CC      arch/arm/domain_build.o
> > arch/arm/domain.c:348:30: error: ‘return_to_new_vcpu64’ undeclared (first
> > use in this function)
> >    348 |         reset_stack_and_jump(return_to_new_vcpu64);
> >        |                              ^~~~~~~~~~~~~~~~~~~~
> > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro
> > ‘switch_stack_and_jump’
> >     48 |     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn)
> > : "memory" ); \
> >        |                                                                 ^~
> > arch/arm/domain.c:348:9: note: in expansion of macro ‘reset_stack_and_jump’
> >    348 |         reset_stack_and_jump(return_to_new_vcpu64);
> >        |         ^~~~~~~~~~~~~~~~~~~~
> > make[2]: *** [Rules.mk:246: arch/arm/domain.o] Error 1
> 
> With this change, the compiler becomes aware that the functions idle_loop,
> return_to_new_vcpu32 and return_to_new_vcpu64 are used by the inline assembly.
> For idle loop, there is a previous declaration but for the other two there is
> not and when the compiler encounters their references complains.
> So, for this to compile, it is also required to declare
> return_to_new_vcpu32 and return_to_new_vcpu64.

Ah, right! I didn't read close enough the error message. I can see now
that it builds just fine on both arm32 and arm64. I am fine with this.