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

Xenia Ragiadakou posted 1 patch 1 year, 8 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220728162151.1228747-1-burzalodowa@gmail.com
xen/arch/arm/domain.c              | 5 ++++-
xen/arch/arm/include/asm/current.h | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
[PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago
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.

Add the function as a 'fake' input operand to the inline assembly statement,
to make the compiler aware that the function is used.
Fake means that the function is not actually used as an operand by the asm code.
That is because there is not a suitable gcc arm32 asm constraint for labels.

Declare return_to_new_vcpu32() and return_to_new_vcpu64() that are also
referenced by this inline asm statement.

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

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v2:
- remove the 'used' attribute and pass the function as input operand to
the inline asm statement
- declare return_to_new_vcpu32() and return_to_new_vcpu64()

Changes in v3:
- remove the declarations of return_to_new_vcpu32() and return_to_new_vcpu64()
from asm/current.h because this is not the appropriate header
- place them in arm/domain.c to restrict their visibilty to this particular file
- declare them as noreturn

 xen/arch/arm/domain.c              | 5 ++++-
 xen/arch/arm/include/asm/current.h | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2f8eaab7b5..ce1089f0c6 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 void idle_loop(void)
 {
     unsigned int cpu = smp_processor_id();
 
@@ -331,6 +331,9 @@ static void schedule_tail(struct vcpu *prev)
     update_vcpu_system_time(current);
 }
 
+extern void noreturn return_to_new_vcpu32(void);
+extern void noreturn return_to_new_vcpu64(void);
+
 static void continue_new_vcpu(struct vcpu *prev)
 {
     current->arch.actlr = READ_SYSREG(ACTLR_EL1);
diff --git a/xen/arch/arm/include/asm/current.h b/xen/arch/arm/include/asm/current.h
index 73e81458e5..6973eeb1d1 100644
--- a/xen/arch/arm/include/asm/current.h
+++ b/xen/arch/arm/include/asm/current.h
@@ -45,7 +45,7 @@ static inline struct cpu_info *get_cpu_info(void)
 #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
 
 #define switch_stack_and_jump(stack, fn) do {                           \
-    asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ); \
+    asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \
     unreachable();                                                      \
 } while ( false )
 
-- 
2.34.1
Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Jan Beulich 1 year, 8 months ago
On 28.07.2022 18:21, Xenia Ragiadakou wrote:
> --- 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 void idle_loop(void)

While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago
Hi Jan,

On 7/29/22 09:26, Jan Beulich wrote:
> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>> --- 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 void idle_loop(void)
> 
> While you're adding "noreturn" below, shouldn't this one be marked so
> as well? Preferably with the addition:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 

Yes, but I was not sure if this should go in this patch or in a separate 
one.

-- 
Xenia
Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Bertrand Marquis 1 year, 8 months ago
Hi Xenia,

> On 29 Jul 2022, at 07:31, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> Hi Jan,
> 
> On 7/29/22 09:26, Jan Beulich wrote:
>> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>>> --- 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 void idle_loop(void)
>> While you're adding "noreturn" below, shouldn't this one be marked so
>> as well? Preferably with the addition:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Yes, but I was not sure if this should go in this patch or in a separate one.

As you modify the function to make it static, I think it is ok to also add the noreturn in the same patch.

With that done, you can add my:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand
> 
> -- 
> Xenia
Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago
On 7/29/22 16:41, Bertrand Marquis wrote:
> Hi Xenia,
> 
>> On 29 Jul 2022, at 07:31, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>
>> Hi Jan,
>>
>> On 7/29/22 09:26, Jan Beulich wrote:
>>> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>>>> --- 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 void idle_loop(void)
>>> While you're adding "noreturn" below, shouldn't this one be marked so
>>> as well? Preferably with the addition:
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Yes, but I was not sure if this should go in this patch or in a separate one.
> 
> As you modify the function to make it static, I think it is ok to also add the noreturn in the same patch.
> 
> With that done, you can add my:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Cheers
> Bertrand

I consider this change unrelated to the patch. I think it is a bad 
practice to squash unrelated changes in a single patch. Also, I do not 
think it is unfair to be obliged to make it in order for the patch to be 
accepted.
I could have taken the opportunity to fix this in the same patch but I 
decided to not take it.

-- 
Xenia
Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Julien Grall 1 year, 8 months ago
Hi Xenia,

On 29/07/2022 15:03, Xenia Ragiadakou wrote:
> 
> On 7/29/22 16:41, Bertrand Marquis wrote:
>> Hi Xenia,
>>
>>> On 29 Jul 2022, at 07:31, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>> wrote:
>>>
>>> Hi Jan,
>>>
>>> On 7/29/22 09:26, Jan Beulich wrote:
>>>> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>>>>> --- 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 void idle_loop(void)
>>>> While you're adding "noreturn" below, shouldn't this one be marked so
>>>> as well? Preferably with the addition:
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Yes, but I was not sure if this should go in this patch or in a 
>>> separate one.
>>
>> As you modify the function to make it static, I think it is ok to also 
>> add the noreturn in the same patch.
>>
>> With that done, you can add my:
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Cheers
>> Bertrand
> 
> I consider this change unrelated to the patch. I think it is a bad 
> practice to squash unrelated changes in a single patch. Also, I do not 
> think it is unfair to be obliged to make it in order for the patch to be 
> accepted.
> I could have taken the opportunity to fix this in the same patch but I 
> decided to not take it.

In general, I don't like having multiple changes within a patch. 
However, here this is a consistency problem. You are modifying the 3 
prototypes (well one is technically a declaration) and it reads odd that 
2 are using noreturn but not the other one.

I would actually argue that if this patch goes in like that, then the 
commit message ought to explain why there is a lack of consistency.

Anyway, I agree with Bertrand that it would be preferable to add 
noreturn to the declaration of idle_loop() in this patch.

To avoid a round trip, I would be OK to handle on commit.

Cheers,

-- 
Julien Grall

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago
Hi Julien,

Hi Julien,

On 7/29/22 18:15, Julien Grall wrote:
> Hi Xenia,
> 
> On 29/07/2022 15:03, Xenia Ragiadakou wrote:
>>
>> On 7/29/22 16:41, Bertrand Marquis wrote:
>>> Hi Xenia,
>>>
>>>> On 29 Jul 2022, at 07:31, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>> wrote:
>>>>
>>>> Hi Jan,
>>>>
>>>> On 7/29/22 09:26, Jan Beulich wrote:
>>>>> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>>>>>> --- 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 void idle_loop(void)
>>>>> While you're adding "noreturn" below, shouldn't this one be marked so
>>>>> as well? Preferably with the addition:
>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Yes, but I was not sure if this should go in this patch or in a 
>>>> separate one.
>>>
>>> As you modify the function to make it static, I think it is ok to 
>>> also add the noreturn in the same patch.
>>>
>>> With that done, you can add my:
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>
>>> Cheers
>>> Bertrand
>>
>> I consider this change unrelated to the patch. I think it is a bad 
>> practice to squash unrelated changes in a single patch. Also, I do not 
>> think it is unfair to be obliged to make it in order for the patch to 
>> be accepted.
>> I could have taken the opportunity to fix this in the same patch but I 
>> decided to not take it.
> 
> In general, I don't like having multiple changes within a patch. 
> However, here this is a consistency problem. You are modifying the 3 
> prototypes (well one is technically a declaration) and it reads odd that 
> 2 are using noreturn but not the other one.
> 

The patch adds the 2 function declarations, it does not modify them. 
Since they do not return, they are declared noreturn.
If the function idle_loop was not declared noreturn, although it should 
have been, for me is a completely different issue.

> I would actually argue that if this patch goes in like that, then the 
> commit message ought to explain why there is a lack of consistency.
> 

I do not agree with you saying that the patch introduced this 
inconsistency. The function idle_loop should have been declared noreturn 
in the first place. If you would like to fix this in the current patch, 
it is up to you.

> Anyway, I agree with Bertrand that it would be preferable to add 
> noreturn to the declaration of idle_loop() in this patch.
> 
> To avoid a round trip, I would be OK to handle on commit.
> 
> Cheers,
> 

-- 
Xenia

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Posted by Julien Grall 1 year, 8 months ago
Hi,

On 29/07/2022 16:47, Xenia Ragiadakou wrote:
> On 7/29/22 18:15, Julien Grall wrote:
>> Hi Xenia,
>>
>> On 29/07/2022 15:03, Xenia Ragiadakou wrote:
>>>
>>> On 7/29/22 16:41, Bertrand Marquis wrote:
>>>> Hi Xenia,
>>>>
>>>>> On 29 Jul 2022, at 07:31, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>>> wrote:
>>>>>
>>>>> Hi Jan,
>>>>>
>>>>> On 7/29/22 09:26, Jan Beulich wrote:
>>>>>> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>>>>>>> --- 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 void idle_loop(void)
>>>>>> While you're adding "noreturn" below, shouldn't this one be marked so
>>>>>> as well? Preferably with the addition:
>>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> Yes, but I was not sure if this should go in this patch or in a 
>>>>> separate one.
>>>>
>>>> As you modify the function to make it static, I think it is ok to 
>>>> also add the noreturn in the same patch.
>>>>
>>>> With that done, you can add my:
>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>>
>>>> Cheers
>>>> Bertrand
>>>
>>> I consider this change unrelated to the patch. I think it is a bad 
>>> practice to squash unrelated changes in a single patch. Also, I do 
>>> not think it is unfair to be obliged to make it in order for the 
>>> patch to be accepted.
>>> I could have taken the opportunity to fix this in the same patch but 
>>> I decided to not take it.
>>
>> In general, I don't like having multiple changes within a patch. 
>> However, here this is a consistency problem. You are modifying the 3 
>> prototypes (well one is technically a declaration) and it reads odd 
>> that 2 are using noreturn but not the other one.
>>
> 
> The patch adds the 2 function declarations, it does not modify them.

Fair enough, you are adding 2 declarations and modifying one. This 
doesn't change the inconsistency problem though.

> Since they do not return, they are declared noreturn.
> If the function idle_loop was not declared noreturn, although it should 
> have been, for me is a completely different issue.
[...]

> I do not agree with you saying that the patch introduced this 
> inconsistency. The function idle_loop should have been declared noreturn 
> in the first place.

I think everyone involved in the discussion agrees that idle_loop() 
should have been marked as noreturn from the start.

And we all agree that this needs to be fixed. So I don't think this is 
particularly useful to spend time arguing on whether this needs to be 
handled within or separately. 3 of the reviewers agree that this should 
be preferably added here. So...

> If you would like to fix this in the current patch, 
> it is up to you.

... I will commit it with the change on the next swipe.

Cheers,

-- 
Julien Grall