Rule 2.1 states: "A project shall not contain unreachable code".

The placement of the loop after "out_unmap" can be moved earlier
in order to avoid the unconditional return to be marked as a cause of
unreachability for the loop, as this is a consequence of
"__builtin_unreachable" being configured in ECLAIR as being deliberately
unreachable, and therefore not reported as causing the code after the
"out_unmap" label to be unreachable.

Replacing one instance of "goto out_unmap" with the loop avoids
considering the unconditional return at the end of the function as a cause
of unreachability, while preserving the semantics of the function.

No functional change intended.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- rebased against current staging
Changes in v3:
- move the loop inside the if and avoid one goto
---
 xen/arch/x86/mm/p2m-pod.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index bd84fe9e27ee..8b6f9909c5a1 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1005,7 +1005,14 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
             {
                 ASSERT_UNREACHABLE();
                 domain_crash(d);
-                goto out_unmap;
+out_unmap:
+                /*
+                 * Something went wrong, probably crashing the domain.  Unmap
+                 * everything and return.
+                 */
+                for ( i = 0; i < count; i++ )
+                    if ( map[i] )
+                        unmap_domain_page(map[i]);
             }
         }
         else
@@ -1032,17 +1039,6 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
             ioreq_request_mapcache_invalidate(d);
         }
     }
-
-    return;
-
-out_unmap:
-    /*
-     * Something went wrong, probably crashing the domain.  Unmap
-     * everything and return.
-     */
-    for ( i = 0; i < count; i++ )
-        if ( map[i] )
-            unmap_domain_page(map[i]);
 }

 static void
--
2.43.0

On 10.12.2024 11:54, Nicola Vetrini wrote:
> Rule 2.1 states: "A project shall not contain unreachable code".
> 
> The placement of the loop after "out_unmap" can be moved earlier
> in order to avoid the unconditional return to be marked as a cause of
> unreachability for the loop, as this is a consequence of
> "__builtin_unreachable" being configured in ECLAIR as being deliberately
> unreachable, and therefore not reported as causing the code after the
> "out_unmap" label to be unreachable.
> 
> Replacing one instance of "goto out_unmap" with the loop avoids
> considering the unconditional return at the end of the function as a cause
> of unreachability, while preserving the semantics of the function.
> 
> No functional change intended.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with ...

> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -1005,7 +1005,14 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
>              {
>                  ASSERT_UNREACHABLE();
>                  domain_crash(d);
> -                goto out_unmap;
> +out_unmap:

... the label indented by one or more blanks, as per ./CODING_STYLE.
Happy to adjust while committing.

Jan

On 2024-12-10 13:43, Jan Beulich wrote:
> On 10.12.2024 11:54, Nicola Vetrini wrote:
>> Rule 2.1 states: "A project shall not contain unreachable code".
>> 
>> The placement of the loop after "out_unmap" can be moved earlier
>> in order to avoid the unconditional return to be marked as a cause of
>> unreachability for the loop, as this is a consequence of
>> "__builtin_unreachable" being configured in ECLAIR as being 
>> deliberately
>> unreachable, and therefore not reported as causing the code after the
>> "out_unmap" label to be unreachable.
>> 
>> Replacing one instance of "goto out_unmap" with the loop avoids
>> considering the unconditional return at the end of the function as a 
>> cause
>> of unreachability, while preserving the semantics of the function.
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with ...
> 

Thanks

>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -1005,7 +1005,14 @@ p2m_pod_zero_check(struct p2m_domain *p2m, 
>> const gfn_t *gfns, unsigned int count
>>              {
>>                  ASSERT_UNREACHABLE();
>>                  domain_crash(d);
>> -                goto out_unmap;
>> +out_unmap:
> 
> ... the label indented by one or more blanks, as per ./CODING_STYLE.
> Happy to adjust while committing.
> 

Right, I followed the style used in this file assuming that it was in 
line with CODING_STYLE, but I now see that this is not the case. No 
problem either way.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)