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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxxx>

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

> > ---
> > 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.