Re: [XEN PATCH v2 4/9] x86/efi: tidy switch statement and address MISRA violation

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 8 Apr 2024, Jan Beulich wrote:
> On 05.04.2024 11:14, Nicola Vetrini wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -169,20 +169,22 @@ static void __init 
> > efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
> >  
> >          switch ( desc->Type )
> >          {
> > +        default:
> > +            type = E820_RESERVED;
> > +            break;
> 
> This one I guess is tolerable duplication-wise, and I might guess others would
> even have preferred it like that from the beginning. A blank line below here
> would be nice, though (and at some point ahead of and between the two ACPI-
> related case labels blank lines would want adding, too).
> 
> However, ...
> 
> >          case EfiBootServicesCode:
> >          case EfiBootServicesData:
> >              if ( map_bs )
> >              {
> > -        default:
> >                  type = E820_RESERVED;
> >                  break;
> >              }
> > -            /* fall through */
> > +            fallthrough;
> >          case EfiConventionalMemory:
> >              if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 
> > &&
> >                   len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
> >                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & 
> > PAGE_MASK;
> > -            /* fall through */
> > +            fallthrough;
> >          case EfiLoaderCode:
> >          case EfiLoaderData:
> >              if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> 
> ... below here there is
> 
>             else
>         case EfiUnusableMemory:
>                 type = E820_UNUSABLE;
>             break;
> 
> I understand there are no figure braces here, and hence be the letter this
> isn't an issue with the Misra rule. But (a) some (e.g. Andrew, I guess)
> would likely argue for there wanting to be braces and (b) we don't want to
> be leaving this as is, when the spirit of the rule still suggests it should
> be done differently.

I agree. For clarity I would go with the following because it is easier
to read:

        case EfiLoaderData:
            if ( desc->Attribute & EFI_MEMORY_RUNTIME )
                type = E820_RESERVED;
            else if ( desc->Attribute & EFI_MEMORY_WB )
                type = E820_RAM;
            else
                type = E820_UNUSABLE;
            break;
        case EfiUnusableMemory:
            type = E820_UNUSABLE;
            break;