[XEN PATCH v2 0/4] x86: address violations of MISRA C Rule 16.3

[XEN PATCH v2 0/4] x86: address violations of MISRA C Rule 16.3

[Federico Serafini]

If these 4 patches will be committed, only 4 violations will be left:
these are under x86_emulate and involve some fallthrough cases.
I'll wait for instructions from x86 maintainers about the right "format" to make
the fallthrough intention explicit (i.e., a comment or the fallthrough keyword
that is not defined in x86-emulate.h at the moment).

Federico Serafini (4):
  x86/emul: add defensive code
  x86/emul: address violations of MISRA C Rule 16.3
  xen/vpci: address violations of MISRA C Rule 16.3
  xen/pci: address a violation of MISRA C Rule 16.3

 xen/arch/x86/x86_emulate/fpu.c         | 5 +++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 3 +++
 xen/drivers/passthrough/pci.c          | 4 +++-
 xen/drivers/vpci/msix.c                | 2 ++
 4 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.43.0

[XEN PATCH v2 1/4] x86/emul: add defensive code

[Federico Serafini]

Add defensive code after unreachable program points.
This also meets the requirements to deviate violations of MISRA C:2012
Rule 16.3: "An unconditional `break' statement shall terminate every
switch-clause".

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- use goto unhandleable.
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 0ea0d5e67e..6b6b8c8fe4 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6840,6 +6840,7 @@ x86_emulate(
             break;
         default:
             ASSERT_UNREACHABLE();
+            goto unhandleable;
         }
         break;
 #ifdef HAVE_AS_SSE4_2
@@ -6868,6 +6869,7 @@ x86_emulate(
 # endif
         default:
             ASSERT_UNREACHABLE();
+            goto unhandleable;
         }
         break;
 #endif
-- 
2.43.0

Re: [XEN PATCH v2 1/4] x86/emul: add defensive code

[Jan Beulich]

On 07.10.2024 16:16, Federico Serafini wrote:
> Add defensive code after unreachable program points.
> This also meets the requirements to deviate violations of MISRA C:2012
> Rule 16.3: "An unconditional `break' statement shall terminate every
> switch-clause".
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 2/4] x86/emul: address violations of MISRA C Rule 16.3

[Federico Serafini]

Add missing break statements to address violations of MISRA C:2012
Rule 16.3 (An unconditional `break' statement shall terminate
every switch-clause).

Make explicit unreachability of a program points with
ASSERT_UNREACHABLE().

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- unreachability made explicit.
---
 xen/arch/x86/x86_emulate/fpu.c         | 5 +++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 1 +
 2 files changed, 6 insertions(+)

diff --git a/xen/arch/x86/x86_emulate/fpu.c b/xen/arch/x86/x86_emulate/fpu.c
index 480d879657..f0dab346e0 100644
--- a/xen/arch/x86/x86_emulate/fpu.c
+++ b/xen/arch/x86/x86_emulate/fpu.c
@@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
              */
             if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
                 dst->type = OP_NONE;
+            break;
         }
         break;
 
@@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
             default:
                 generate_exception(X86_EXC_UD);
             }
+            break;
         }
         break;
 
@@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
              */
             if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
                 dst->type = OP_NONE;
+            break;
         }
         break;
 
@@ -457,6 +460,8 @@ int x86emul_fpu(struct x86_emulate_state *s,
             case 7: /* fistp m64i */
                 goto fpu_memdst64;
             }
+            ASSERT_UNREACHABLE();
+            break;
         }
         break;
 
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 6b6b8c8fe4..30674ec301 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -8310,6 +8310,7 @@ x86_emulate(
         }
         if ( rc != 0 )
             goto done;
+        break;
     default:
         break;
     }
-- 
2.43.0

Re: [XEN PATCH v2 2/4] x86/emul: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 07.10.2024 16:16, Federico Serafini wrote:
> @@ -457,6 +460,8 @@ int x86emul_fpu(struct x86_emulate_state *s,
>              case 7: /* fistp m64i */
>                  goto fpu_memdst64;
>              }
> +            ASSERT_UNREACHABLE();
> +            break;

"return X86EMUL_UNHANDLEABLE" like is also used just a few lines down
from here?

Jan

[XEN PATCH v2 3/4] xen/vpci: address violations of MISRA C Rule 16.3

[Federico Serafini]

Address violations of MISRA C:2012 Rule 16.3:
"An unconditional `break' statement shall terminate every
switch-clause".

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes from v2:
- simply break without returning X86EMUL_UNHANDLEABLE.

As pointed out by Jan, these functions only return X86EMUL_OKAY but:
https://lists.xenproject.org/archives/html/xen-devel/2024-09/msg00727.html

Do you have any comments?
---
 xen/drivers/vpci/msix.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index fbe710ab92..5bb4444ce2 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -364,6 +364,7 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
     spin_unlock(&vpci->lock);
 
@@ -512,6 +513,7 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
     spin_unlock(&vpci->lock);
 
-- 
2.43.0

Re: [XEN PATCH v2 3/4] xen/vpci: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 7 Oct 2024, Federico Serafini wrote:
> Address violations of MISRA C:2012 Rule 16.3:
> "An unconditional `break' statement shall terminate every
> switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
> Changes from v2:
> - simply break without returning X86EMUL_UNHANDLEABLE.
> 
> As pointed out by Jan, these functions only return X86EMUL_OKAY but:
> https://lists.xenproject.org/archives/html/xen-devel/2024-09/msg00727.html
> 
> Do you have any comments?
> ---
>  xen/drivers/vpci/msix.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index fbe710ab92..5bb4444ce2 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -364,6 +364,7 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        break;
>      }
>      spin_unlock(&vpci->lock);
>  
> @@ -512,6 +513,7 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        break;
>      }
>      spin_unlock(&vpci->lock);

I think in both cases it should be:

spin_unlock(&vpci->lock);
return X86EMUL_UNHANDLEABLE;

In general, it seems to be that we want to return X86EMUL_UNHANDLEABLE
in these situations and either we returning it from the default label,
or we need to do rc = X86EMUL_UNHANDLEABLE; and later on return rc;

Re: [XEN PATCH v2 3/4] xen/vpci: address violations of MISRA C Rule 16.3

[Roger Pau Monné]

On Mon, Oct 07, 2024 at 02:44:22PM -0700, Stefano Stabellini wrote:
> On Mon, 7 Oct 2024, Federico Serafini wrote:
> > Address violations of MISRA C:2012 Rule 16.3:
> > "An unconditional `break' statement shall terminate every
> > switch-clause".
> > 
> > No functional change.
> > 
> > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> > ---
> > Changes from v2:
> > - simply break without returning X86EMUL_UNHANDLEABLE.
> > 
> > As pointed out by Jan, these functions only return X86EMUL_OKAY but:
> > https://lists.xenproject.org/archives/html/xen-devel/2024-09/msg00727.html
> > 
> > Do you have any comments?

I think it's a result of how the series that implemented
adjacent_{read,write}() evolved.  Originally it was supposed to return
X86EMUL_UNHANDLEABLE for the earlier error cases at the top of the
function.

Returning an error code however is not helpful in this context, as the
accesses belong to the IO region of the device, and hence must be
terminated here.  There's no reason to return X86EMUL_UNHANDLEABLE or
similar, because no other handler should be able to complete them
anyway (or else we have a bigger issue).

I would be happy if someone did a patch to convert
adjacent_{read,write}() to instead return void.

> > ---
> >  xen/drivers/vpci/msix.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> > index fbe710ab92..5bb4444ce2 100644
> > --- a/xen/drivers/vpci/msix.c
> > +++ b/xen/drivers/vpci/msix.c
> > @@ -364,6 +364,7 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
> >  
> >      default:
> >          ASSERT_UNREACHABLE();
> > +        break;
> >      }
> >      spin_unlock(&vpci->lock);
> >  
> > @@ -512,6 +513,7 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
> >  
> >      default:
> >          ASSERT_UNREACHABLE();
> > +        break;
> >      }
> >      spin_unlock(&vpci->lock);
> 
> I think in both cases it should be:
> 
> spin_unlock(&vpci->lock);
> return X86EMUL_UNHANDLEABLE;
> 
> In general, it seems to be that we want to return X86EMUL_UNHANDLEABLE
> in these situations and either we returning it from the default label,
> or we need to do rc = X86EMUL_UNHANDLEABLE; and later on return rc;

As said above, the accesses should be terminated here, hence returning
X86EMUL_UNHANDLEABLE doesn't seem appropriate.  The only way to signal
errors for such kind of IO access is to return ~0 in the read case, or
ignore the access in the write case.

We could consider raising a different kind of error (not
X86EMUL_UNHANDLEABLE) when an otherwise unreachable state is reached,
but I'm struggling as to what kind of action would the caller need to
take in that case, kill the guest?

Thanks, Roger.

[XEN PATCH v2 4/4] xen/pci: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Refactor the code to avoid an implicit fallthrough and address
a violation of MISRA C:2012 Rule 16.3: "An unconditional `break'
statement shall terminate every switch-clause".

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- improved description.
---
 xen/drivers/passthrough/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 5a446d3dce..a5705def3f 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
     {
     case 1: case 2: case 4:
         if ( *s )
-    default:
             return -EINVAL;
+        break;
+    default:
+        return -EINVAL;
     }
 
     phantom_devs[nr_phantom_devs++] = phantom;
-- 
2.43.0

Re: [XEN PATCH v2 4/4] xen/pci: address a violation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 7 Oct 2024, Federico Serafini wrote:
> Refactor the code to avoid an implicit fallthrough and address
> a violation of MISRA C:2012 Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes in v2:
> - improved description.
> ---
>  xen/drivers/passthrough/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 5a446d3dce..a5705def3f 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
>      {
>      case 1: case 2: case 4:
>          if ( *s )
> -    default:
>              return -EINVAL;
> +        break;
> +    default:
> +        return -EINVAL;
>      }
>  
>      phantom_devs[nr_phantom_devs++] = phantom;
> -- 
> 2.43.0
> 

Re: [XEN PATCH v2 4/4] xen/pci: address a violation of MISRA C Rule 16.3

[Roger Pau Monné]

On Mon, Oct 07, 2024 at 04:16:19PM +0200, Federico Serafini wrote:
> Refactor the code to avoid an implicit fallthrough and address
> a violation of MISRA C:2012 Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
> Changes in v2:
> - improved description.
> ---
>  xen/drivers/passthrough/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 5a446d3dce..a5705def3f 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
>      {
>      case 1: case 2: case 4:
>          if ( *s )
> -    default:
>              return -EINVAL;
> +        break;

Would you mind adding a newline here between the break and the default
case?

With that:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [XEN PATCH v2 4/4] xen/pci: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 11.10.2024 10:45, Roger Pau Monné wrote:
> On Mon, Oct 07, 2024 at 04:16:19PM +0200, Federico Serafini wrote:
>> Refactor the code to avoid an implicit fallthrough and address
>> a violation of MISRA C:2012 Rule 16.3: "An unconditional `break'
>> statement shall terminate every switch-clause".
>>
>> No functional change.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>> ---
>> Changes in v2:
>> - improved description.
>> ---
>>  xen/drivers/passthrough/pci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 5a446d3dce..a5705def3f 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
>>      {
>>      case 1: case 2: case 4:
>>          if ( *s )
>> -    default:
>>              return -EINVAL;
>> +        break;
> 
> Would you mind adding a newline here between the break and the default
> case?

I actually took the liberty to do so while committing (already a few days
ago).

Jan

Re: [XEN PATCH v2 4/4] xen/pci: address a violation of MISRA C Rule 16.3

[Roger Pau Monné]

On Fri, Oct 11, 2024 at 10:48:13AM +0200, Jan Beulich wrote:
> On 11.10.2024 10:45, Roger Pau Monné wrote:
> > On Mon, Oct 07, 2024 at 04:16:19PM +0200, Federico Serafini wrote:
> >> Refactor the code to avoid an implicit fallthrough and address
> >> a violation of MISRA C:2012 Rule 16.3: "An unconditional `break'
> >> statement shall terminate every switch-clause".
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> >> ---
> >> Changes in v2:
> >> - improved description.
> >> ---
> >>  xen/drivers/passthrough/pci.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> >> index 5a446d3dce..a5705def3f 100644
> >> --- a/xen/drivers/passthrough/pci.c
> >> +++ b/xen/drivers/passthrough/pci.c
> >> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
> >>      {
> >>      case 1: case 2: case 4:
> >>          if ( *s )
> >> -    default:
> >>              return -EINVAL;
> >> +        break;
> > 
> > Would you mind adding a newline here between the break and the default
> > case?
> 
> I actually took the liberty to do so while committing (already a few days
> ago).

Oh, thanks, should have looked at xen.git before commenting.

Roger.