[PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

Sergey Temerkhanov posted 1 patch 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200911144309.4559-1-s.temerkhanov@gmail.com
Maintainers: Jan Beulich <jbeulich@suse.com>
xen/common/efi/boot.c    | 27 ++++++++++++++++++++++++++-
xen/include/efi/efidef.h |  6 ++++++
2 files changed, 32 insertions(+), 1 deletion(-)

[PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

Posted by Sergey Temerkhanov 1 week ago
This helps overcome problems observed with some UEFI implementations
which don't set the Attributes field in memery descriptors properly

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 xen/common/efi/boot.c    | 27 ++++++++++++++++++++++++++-
 xen/include/efi/efidef.h |  6 ++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 5a520bf21d..811d8a0923 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1100,7 +1100,14 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
     {
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
 
-        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+        /*
+         * EfiRuntimeServicesCode and EfiRuntimeServicesData
+         * memory ranges 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;
@@ -1510,6 +1517,24 @@ void __init efi_init_memory(void)
                desc->PhysicalStart, desc->PhysicalStart + len - 1,
                desc->Type, desc->Attribute);
 
+        /*
+         * EfiRuntimeServicesCode and EfiRuntimeServicesData
+         * memory ranges are adjusted here. Any changes
+         * or adjustments must be kept in sync with efi_exit_boot()
+         */
+        if ( efi_enabled(EFI_RS) &&
+             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
+               (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) &&
+               (desc->Type == EfiRuntimeServicesCode ||
+                desc->Type == EfiRuntimeServicesData)) )
+        {
+            printk(XENLOG_WARNING
+                   "Setting EFI_RUNTIME memory attribute for area %013"
+                   PRIx64 "-%013" PRIx64 "\n",
+                   desc->PhysicalStart, desc->PhysicalStart + len - 1);
+            desc->Attribute |= EFI_MEMORY_RUNTIME;
+        }
+
         if ( (desc->Attribute & (EFI_MEMORY_WB | EFI_MEMORY_WT)) ||
              (efi_bs_revision >= EFI_REVISION(2, 5) &&
               (desc->Attribute & EFI_MEMORY_WP)) )
diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
index 86a7e111bf..3a35bfc8be 100644
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -158,6 +158,12 @@ typedef enum {
 #define EFI_MEMORY_UCE          0x0000000000000010  
 #define EFI_MEMORY_WP           0x0000000000001000
 
+#define EFI_MEMORY_CACHEABILITY_MASK  ( EFI_MEMORY_UC | \
+                                        EFI_MEMORY_WC | \
+                                        EFI_MEMORY_WT | \
+                                        EFI_MEMORY_WB | \
+                                        EFI_MEMORY_UCE )
+
 // physical memory protection on range 
 #define EFI_MEMORY_RP           0x0000000000002000
 #define EFI_MEMORY_XP           0x0000000000004000
-- 
2.26.2


Re: [PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

Posted by Jan Beulich 6 days ago
On 11.09.2020 16:43, Sergey Temerkhanov wrote:
> @@ -1510,6 +1517,24 @@ void __init efi_init_memory(void)
>                 desc->PhysicalStart, desc->PhysicalStart + len - 1,
>                 desc->Type, desc->Attribute);
>  
> +        /*
> +         * EfiRuntimeServicesCode and EfiRuntimeServicesData
> +         * memory ranges are adjusted here. Any changes
> +         * or adjustments must be kept in sync with efi_exit_boot()
> +         */
> +        if ( efi_enabled(EFI_RS) &&
> +             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +               (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) &&
> +               (desc->Type == EfiRuntimeServicesCode ||
> +                desc->Type == EfiRuntimeServicesData)) )
> +        {
> +            printk(XENLOG_WARNING
> +                   "Setting EFI_RUNTIME memory attribute for area %013"
> +                   PRIx64 "-%013" PRIx64 "\n",
> +                   desc->PhysicalStart, desc->PhysicalStart + len - 1);
> +            desc->Attribute |= EFI_MEMORY_RUNTIME;
> +        }

So you've moved from always checking for EFI_MEMORY_WP to not
checking it at all. Neither is the way to go imo. Similarly, ...

> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -158,6 +158,12 @@ typedef enum {
>  #define EFI_MEMORY_UCE          0x0000000000000010  
>  #define EFI_MEMORY_WP           0x0000000000001000
>  
> +#define EFI_MEMORY_CACHEABILITY_MASK  ( EFI_MEMORY_UC | \
> +                                        EFI_MEMORY_WC | \
> +                                        EFI_MEMORY_WT | \
> +                                        EFI_MEMORY_WB | \
> +                                        EFI_MEMORY_UCE )

... this now doesn't really cover what its name suggests. As
indicated before, without such a (questionable) #define having
appeared in the gnu-efi tree, I don't think we want it, at the
very least not in this imported header. But given that it
doesn't express what you want anyway, I can only repeat my
suggestion to drop this #define altogether.

In order to save further rounds, I would offer to finish this
patch to a shape that I'd feel comfortable with - if that's
okay with you.

Jan