Re: [PATCH 2/2] efi: runtime: accept 1:1 mappings in SetVirtualAddressMap

[Johannes Schauer Marin Rodrigues <josch@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 6366 bytes --]

Tested-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>

Quoting Ahmad Fatoum (2026-10-17 13:50:15)
> We do not support SetVirtualAddressMap() and UEFI v2.8 permits
> this as long as our EFI_RT_PROPERTIES_TABLE doesn't claim
> EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP, which we adhere to[1].
> 
> NetBSD's efiboot doesn't seem to care for lack of this bit and
> calls SetVirtualAddressMap() unconditionally[2]. This will return with
> an error, which has NetBSD call printf() _after_ ExitBootServices[3]
> leading to a crash[4].
> 
> Fortunately, barebox is not the only firmware that doesn't implement
> a full-fledged SetVirtualAddressMap().
> Quoting NetBSD commit 41096fd ("efiboot: Use 1:1 PA to VA mappings for
> RT services.")[5]:
> 
> | UEFI firmware on Snapdragon X Elite devices crashes when
> | SetVirtualAddressMap is called with runtime mappings that are not 1:1.
> | Always do this on arm64 as it won't interfere with KVA and will help us
> | avoid avoid a very common class of firmware bugs.
> 
> This means we have an easy way for barebox to workaround this:
> Instead of returning EFI_UNSUPPORTED unconditionally, we can handle the
> trivial identity mapping case specially and report EFI_SUCCESS for it.
> 
> While NetBSD doesn't need it, also implement a 1:1 ConvertPointer for
> good measure.
> 
> [1]: https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html
> [2]: https://github.com/NetBSD/src/blob/41096fd/sys/stand/efiboot/efiboot.c#L135
> [3]: https://github.com/NetBSD/src/blob/41096fd/sys/stand/efiboot/efifdt.c#L813
> [4]: https://gnats.netbsd.org/60890
> [5]: https://github.com/NetBSD/src/commit/41096fd4f7bac2ade3f29e9e6e00951aef9e16ce
> 
> Reported-by: Johannes Schauer Marin Rodrigues <josch@debian.org>
> Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
> ---
>  efi/runtime/services.c | 67 ++++++++++++++++++++++++++++++++++++++++--
>  include/efi/memory.h   |  6 ++++
>  include/efi/services.h |  2 ++
>  3 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/efi/runtime/services.c b/efi/runtime/services.c
> index dc4960ac4763..ca60ae512a2d 100644
> --- a/efi/runtime/services.c
> +++ b/efi/runtime/services.c
> @@ -171,17 +171,80 @@ static efi_status_t EFIAPI efirt_set_wakeup_time(char enabled,
>         return efirt_unimplemented();
>  }
>  
> +/**
> + * efirt_set_virtual_address_map() - change from physical to virtual mapping
> + *
> + * This function implements the SetVirtualAddressMap() runtime service.
> + * The barebox runtime isn't relocated and only an identity mapping
> + * is accepted.
> + *
> + * We intentionally don't announce support via the EFI_RT_PROPERTIES_TABLE,
> + * so only operating systems that ignore the table would end up here.
> + * If they configure a 1:1 mapping like NetBSD, we tolerate it and return
> + * success.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @memory_map_size:   size of the virtual map
> + * @descriptor_size:   size of an entry in the map
> + * @descriptor_version:        version of the map entries
> + * @virtmap:           virtual address mapping information
> + * Return:             status code
> + */
>  static efi_status_t EFIAPI efirt_set_virtual_address_map(size_t memory_map_size,
>                                                          size_t descriptor_size,
>                                                          uint32_t descriptor_version,
>                                                          struct efi_memory_desc *virtmap)
>  {
> -       return efirt_unimplemented();
> +       static bool efirt_virtmap_set;
> +
> +       EFI_ENTRY(L"");
> +
> +       if (efirt_virtmap_set)
> +               return EFI_EXIT(EFI_UNSUPPORTED);
> +
> +       if (descriptor_version != EFI_MEMORY_DESCRIPTOR_VERSION ||
> +           descriptor_size < sizeof(*virtmap))
> +               return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +       for (size_t off = 0; off < memory_map_size; off += descriptor_size) {
> +               struct efi_memory_desc *md = (void *)virtmap + off;
> +
> +               if ((md->attrs & EFI_MEMORY_RUNTIME) &&
> +                   efi_memory_desc_relocation_offset(md) != 0)
> +                       return EFI_EXIT(EFI_UNSUPPORTED);
> +       }
> +
> +       efirt_virtmap_set = true;
> +
> +       return EFI_EXIT(EFI_SUCCESS);
>  }
>  
> +/**
> + * efirt_convert_pointer() - convert pointer to virtual address
> + *
> + * This function implements the ConvertPointer() runtime service.
> + * Only identity mappings are accepted, so the pointer is left as-is.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @dbg:       EFI_OPTIONAL_PTR if the pointer may be NULL
> + * @address:   pointer to be converted
> + * Return:     status code
> + */
>  static efi_status_t EFIAPI efirt_convert_pointer(unsigned long dbg, void **address)
>  {
> -       return efirt_unimplemented();
> +       EFI_ENTRY(L"");
> +
> +       if (!address)
> +               return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +       if (!*address && !(dbg & EFI_OPTIONAL_PTR))
> +               return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +       return EFI_EXIT(EFI_SUCCESS);
>  }
>  
>  static efi_status_t EFIAPI
> diff --git a/include/efi/memory.h b/include/efi/memory.h
> index 74e714099021..336d2a730c1c 100644
> --- a/include/efi/memory.h
> +++ b/include/efi/memory.h
> @@ -91,4 +91,10 @@ struct efi_memory_desc {
>  
>  #define EFI_MEMORY_DESCRIPTOR_VERSION  1
>  
> +static inline ptrdiff_t
> +efi_memory_desc_relocation_offset(const struct efi_memory_desc *md)
> +{
> +       return efi_virt_to_phys(md->virt_start) - md->phys_start;
> +}
> +
>  #endif
> diff --git a/include/efi/services.h b/include/efi/services.h
> index 2d7167b25205..83dc56150a4e 100644
> --- a/include/efi/services.h
> +++ b/include/efi/services.h
> @@ -162,6 +162,8 @@ enum efi_reset_type {
>  #define EFI_RUNTIME_SERVICES_SIGNATURE ((u64)0x56524553544e5552ULL)
>  #define EFI_RUNTIME_SERVICES_REVISION  0x00010000
>  
> +#define EFI_OPTIONAL_PTR       0x00000001
> +
>  struct efi_capsule_header;
>  
>  struct efi_runtime_services {
> -- 
> 2.47.3
>

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEElFhU6KL81LF4wVq58sulx4+9g+EFAmrThZQACgkQ8sulx4+9
g+GDnQ/+K2nPv06Qew7pVxdXbQ6PLCzntg+NnbayP9TosZXWOaM2919qnlsHvCaS
Fng885Hf9Gwp+twIc0Vj20+jwakznQhL2nDb4/NBuwWis2NoTFcZKFo7dzLhAWAq
hFzwG5eLx5ZIzUiLgyaGefXOQVNroEu4xpEWGZX/70Jzo5NEm6QBAsj4WIqVFGnd
QHBWkLWUxsbhM3YHKF1Hd9vt1u0QynsqTQOQPinlsd4dMNsMCfa8GyG2gJ4KCait
9zbi905opZfQi4dVlqYyvlDg5qsMEg4UYX+tLx8MnLzUB7Gjo07zXtRU8x3mPDvT
dQb/4XtfdqUnG+LJzs/WUZe/dp2EmDeeS6m0o9RmVhztpUz3Y3m35Vvt0oHwJ5Wl
EkhU/PLK57NyTbh3KMA1CLQMyKefH0UGg26XV3TGXlR5sEtZKTgpGeYosgI+vBMM
kV0AGFWkr4xQ3dbNdpd9MwBh/js1BocuyNBSzoLNIvr7H5jgpP/N4ppkFViZT9Hd
Q9tZJCFuV+AF/S2K19k5lgoNu2Nhw/UJdAxJ4i4XNmSKgvTlW2YQF+FdOJFYI/jd
84aV3uQYYSsKYzzyf9pmcdunFO5iu13NbxpBFLUJin7Ih2XUKk3ci7U9wo70F5UM
uD8L1phwfC+aKFwMeWrxjyPJGw9d+7dGc/EPRx1Nb37kQiVIjqU=
=r+XL
-----END PGP SIGNATURE-----