| Message ID | a91c0223b827593f5c1a7ca7ece786266e5b8f52.1728308312.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address violations of MISRA C Rule 16.3 | expand |
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;
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.
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);
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(+)