Re: [PATCH] xen/efi: Reduce ifdefary in efi_exit_boot()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.04.2026 12:38, Andrew Cooper wrote:
> Use IS_ENABLED() rather than #ifdef to give the compiler visibility into the
> block, which in turn removes the #ifdef from the varaible block.

Just to mention, if it was just / mainly ...

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1335,9 +1335,7 @@ static void __init efi_exit_boot(EFI_HANDLE 
> ImageHandle, EFI_SYSTEM_TABLE *Syste
>      EFI_STATUS status;
>      UINTN info_size = 0, map_key;
>      bool retry;
> -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
>      unsigned int i;
> -#endif

... this to be got rid of, we could as well use ...

> @@ -1371,31 +1369,32 @@ static void __init efi_exit_boot(EFI_HANDLE 
> ImageHandle, EFI_SYSTEM_TABLE *Syste
>      if ( EFI_ERROR(status) )
>          PrintErrMesg(L"Cannot exit boot services", status);
>  
> -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
> -    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )

    for ( unsigned int i = 0; i < efi_memmap_size; i += efi_mdesc_size )

now. But yes, the typo aspect you mention can be avoided altogether by what
you change things to.

> +    if ( IS_ENABLED(CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP) )
>      {
> -        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> +        for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> +        {
> +            EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>  
> -        /*
> -         * Runtime services regions are always mapped here.
> -         * Attributes may be adjusted in efi_init_memory().
> -         */
> -        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> -             desc->Type == EfiRuntimeServicesCode ||
> -             desc->Type == EfiRuntimeServicesData )
> -            desc->VirtualStart = desc->PhysicalStart;
> -        else
> -            desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> -    }
> -    status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
> -                                          mdesc_ver, efi_memmap);
> -    if ( status != EFI_SUCCESS )
> -    {
> -        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), 
> disabling runtime services\n",
> -               status);
> -        __clear_bit(EFI_RS, &efi_flags);
> +            /*
> +             * Runtime services regions are always mapped here.
> +             * Attributes may be adjusted in efi_init_memory().
> +             */
> +            if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> +                 desc->Type == EfiRuntimeServicesCode ||
> +                 desc->Type == EfiRuntimeServicesData )
> +                desc->VirtualStart = desc->PhysicalStart;
> +            else
> +                desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> +        }
> +        status = efi_rs->SetVirtualAddressMap(efi_memmap_size, 
> efi_mdesc_size,
> +                                              mdesc_ver, efi_memmap);
> +        if ( status != EFI_SUCCESS )
> +        {
> +            printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), 
> disabling runtime services\n",
> +                   status);

Could I talk you into switching to

            printk(XENLOG_ERR
                   "EFI: SetVirtualAddressMap() failed (%#lx), disabling 
runtime services\n",
                   status);

to make the line at least a little less long?

Jan