The ASSERT_UNREACHABLE() assertion is added before a definitely
unreachable return statement to address MISRA C:2012 Rule 2.1,
because the explicit return from a non-void function is a defensive
coding measure, and thus intended to be unreachable.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/mm.c         | 1 +
 xen/arch/x86/mm/p2m-pod.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index be2b10a391..ebd4f3827a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4879,6 +4879,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         return subarch_memory_op(cmd, arg);
     }
 
+    ASSERT_UNREACHABLE();
     return 0;
 }
 
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 9969eb45fa..cd5501217f 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
     }
 
     return;
+    ASSERT_UNREACHABLE();
 
 out_unmap:
     /*
-- 
2.34.1

On 02.08.2023 16:38, Nicola Vetrini wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4879,6 +4879,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return subarch_memory_op(cmd, arg);
>      }
>  
> +    ASSERT_UNREACHABLE();
>      return 0;
>  }

I'd prefer to instead switch earlier "return 0" to "break".

> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
>      }
>  
>      return;
> +    ASSERT_UNREACHABLE();
>  
>  out_unmap:
>      /*

In the description you say "before", but here you add something _after_
"return". What's the deal?

Jan

On 03/08/2023 11:20, Jan Beulich wrote:
> On 02.08.2023 16:38, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4879,6 +4879,7 @@ long arch_memory_op(unsigned long cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          return subarch_memory_op(cmd, arg);
>>      }
>> 
>> +    ASSERT_UNREACHABLE();
>>      return 0;
>>  }
> 
> I'd prefer to instead switch earlier "return 0" to "break".

Ok

> 
>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const 
>> gfn_t *gfns, unsigned int count
>>      }
>> 
>>      return;
>> +    ASSERT_UNREACHABLE();
>> 
>>  out_unmap:
>>      /*
> 
> In the description you say "before", but here you add something _after_
> "return". What's the deal?
> 
> Jan

In this case the unreachable part is that after the label (looking at it 
now, I should have
put the assert after the label to make it clear), because earlier all 
jumps to
'out_unmap' are like this:

   ASSERT_UNREACHABLE();
   domain_crash(d);
   goto out_unmap;

As I understood it, this is a defensive coding measure, preventing pages 
to remain mapped if,
for some reason the above code actually executes. Am I correct?

Regards,

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

On 03.08.2023 11:30, Nicola Vetrini wrote:
> On 03/08/2023 11:20, Jan Beulich wrote:
>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>> @@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const 
>>> gfn_t *gfns, unsigned int count
>>>      }
>>>
>>>      return;
>>> +    ASSERT_UNREACHABLE();
>>>
>>>  out_unmap:
>>>      /*
>>
>> In the description you say "before", but here you add something _after_
>> "return". What's the deal?
> 
> In this case the unreachable part is that after the label (looking at it 
> now, I should have
> put the assert after the label to make it clear), because earlier all 
> jumps to
> 'out_unmap' are like this:
> 
>    ASSERT_UNREACHABLE();
>    domain_crash(d);
>    goto out_unmap;
> 
> As I understood it, this is a defensive coding measure, preventing pages 
> to remain mapped if,
> for some reason the above code actually executes. Am I correct?

The comment there says "probably". So the label and following code might
be used for another error condition as well. Furthermore both paths
presently using "goto out_unmap" already have ASSERT_UNREACHABLE(), so
it's hard to see why we would need yet one more.

Jan