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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/04/2026 12:01 pm, Jan Beulich wrote:
> 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.

I originally had this change in the patch, but it interferes with diff
showing (just) an indentation change.

I'm not fussed either way.

>
>> +    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?

Ok, but I'm not going to resend just for that.

~Andrew