Re: [XEN PATCH v3 09/12] x86/mm: add defensive return

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote:
> On 26.06.2024 11:28, Federico Serafini wrote:
> > Add defensive return statement at the end of an unreachable
> > default case. Other than improve safety, this meets the requirements
> > to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> > statement shall terminate every switch-clause".
> > 
> > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>
> Tentatively
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -916,6 +916,7 @@ get_page_from_l1e(
> >                  return 0;
> >              default:
> >                  ASSERT_UNREACHABLE();
> > +                return -EPERM;
> >              }
> >          }
> >          else if ( l1f & _PAGE_RW )
>
> I don't like the use of -EPERM here very much, but I understand that there's
> no really suitable errno value. I wonder though whether something far more
> "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
> Just to mention it: -EPERM is what failed XSM checks would typically yield,
> so from that perspective alone even switching to -EACCES might be a little
> bit better.
>

fwiw: EACCES, being typically used for interface version mismatches, would
confuse me a lot.

> I further wonder whether, with the assertion catching an issue with the
> implementation, we shouldn't consider using BUG() here instead. Input from
> in particular the other x86 maintainers appreciated.
>
> Jan

Cheers,
Alejandro