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    | 19 ++++++++++++++++++-
 xen/include/efi/efidef.h |  3 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 5a520bf21d..05cfbf4de0 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1100,7 +1100,9 @@ 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 )
+        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
+              desc->Type == EfiRuntimeServicesCode  ||
+              desc->Type == EfiRuntimeServicesData )
             desc->VirtualStart = desc->PhysicalStart;
         else
             desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
@@ -1510,6 +1512,21 @@ void __init efi_init_memory(void)
                desc->PhysicalStart, desc->PhysicalStart + len - 1,
                desc->Type, desc->Attribute);
 
+        if (efi_enabled(EFI_RS) &&
+             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
+                (desc->Type == EfiRuntimeServicesCode ||
+                 desc->Type == EfiRuntimeServicesData))) {
+            printk(XENLOG_WARNING "Fixing memory attributes for area %013"
+                                   PRIx64 "-%013" PRIx64 "\n",
+                   desc->PhysicalStart, desc->PhysicalStart + len - 1);
+            desc->Attribute |= EFI_MEMORY_RUNTIME;
+            if ( !(desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) ) {
+                desc->Attribute |= (desc->Type == EfiRuntimeServicesCode) &&
+                                   (efi_bs_revision >= EFI_REVISION(2, 5)) ?
+                                        EFI_MEMORY_WP : EFI_MEMORY_UC;
+            }
+        }
+
         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..05170da2db 100644
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -158,6 +158,9 @@ typedef enum {
 #define EFI_MEMORY_UCE          0x0000000000000010  
 #define EFI_MEMORY_WP           0x0000000000001000
 
+#define EFI_MEMORY_CACHEABILITY_MASK \
+                                0x000000000000101F
+
 // physical memory protection on range 
 #define EFI_MEMORY_RP           0x0000000000002000
 #define EFI_MEMORY_XP           0x0000000000004000
-- 
2.26.2

On 04.09.2020 23:11, Sergey Temerkhanov wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1100,7 +1100,9 @@ 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 )
> +        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> +              desc->Type == EfiRuntimeServicesCode  ||

Nit: One too many blank at the start of the line and one too many
ahead of the ||. (Similar issues elsewhere.)

I think with the more involved logic this may also want a comment
indicating it needs keeping in sync with the other place (and a
similar comment should then go there as well).

> @@ -1510,6 +1512,21 @@ void __init efi_init_memory(void)
>                 desc->PhysicalStart, desc->PhysicalStart + len - 1,
>                 desc->Type, desc->Attribute);
>  
> +        if (efi_enabled(EFI_RS) &&
> +             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +                (desc->Type == EfiRuntimeServicesCode ||
> +                 desc->Type == EfiRuntimeServicesData))) {
> +            printk(XENLOG_WARNING "Fixing memory attributes for area %013"
> +                                   PRIx64 "-%013" PRIx64 "\n",
> +                   desc->PhysicalStart, desc->PhysicalStart + len - 1);

Please be more specific with the wording, e.g. "Adding runtime attribute
for area ...". Also please wrap the statement such that the format string
ends up all on one line, even if the line then exceeds the 80 cols limit.
For this you'll want to split between XENLOG_WARNING and the opening
quote.

> +            desc->Attribute |= EFI_MEMORY_RUNTIME;
> +            if ( !(desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) ) {
> +                desc->Attribute |= (desc->Type == EfiRuntimeServicesCode) &&
> +                                   (efi_bs_revision >= EFI_REVISION(2, 5)) ?
> +                                        EFI_MEMORY_WP : EFI_MEMORY_UC;

As said elsewhere already, please don't do this as a "proactive"
workaround.

Also please note that opening braces go on their own lines in our
coding style and there ought to be blanks immediately inside if()
and alike. See the surrounding code for reference.

> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -158,6 +158,9 @@ typedef enum {
>  #define EFI_MEMORY_UCE          0x0000000000000010  
>  #define EFI_MEMORY_WP           0x0000000000001000
>  
> +#define EFI_MEMORY_CACHEABILITY_MASK \
> +                                0x000000000000101F

Has such a #define appeared in the tree we've imported this from?
If not, it probably shouldn't go here. I'm also unconvinced
checking the bit EFI_MEMORY_WP occupies is, for the purpose here,
legitimate on versions prior to 2.5 - it's a reserved bit there,
and hence may have any [benign] meaning.

Finally, if this #define is to survive in the first place, please
don't use a literal number. Make its value an OR of the various
other constants instead, so it becomes self-documenting.

Jan