Use IS_ENABLED() rather than #ifdef to give the compiler visibility into the
block, which in turn removes the #ifdef from the varaible block.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
Best viewed with git show --ignore-all-space
XenServer has been carrying a patch with a typo in this block for a long time,
going entirely unnoticed.
---
xen/common/efi/boot.c | 47 +++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 0ddc7bfd1277..7445f88902cd 100644
--- 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
efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
&efi_mdesc_size, &mdesc_ver);
@@ -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 )
+ 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);
+ __clear_bit(EFI_RS, &efi_flags);
+ }
}
-#endif
/* Adjust pointers into EFI. */
efi_ct = (const void *)efi_ct + DIRECTMAP_VIRT_START;
--
2.39.5
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
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
On 4/9/26 07:11, Andrew Cooper wrote:
> 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
I'm good with fix up on commit.
Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
© 2016 - 2026 Red Hat, Inc.