efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
higher priority than the type of the range. To avoid accessing memory at
runtime which was re-used for other purposes, make
efi_arch_process_memory_map() follow suit. While on x86 in theory the
same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried
to do so, bypassing Xen's memory management), hence that type's handling
can be left alone.

Fixes: bf6501a62e80 ("x86-64: EFI boot code")
Fixes: facac0af87ef ("x86-64: EFI runtime code")
Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm
Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm
---
v2: Amend description.

--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -183,13 +183,15 @@ static EFI_STATUS __init efi_process_mem
 
     for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
     {
-        if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
-             (desc_ptr->Type == EfiConventionalMemory ||
-              desc_ptr->Type == EfiLoaderCode ||
-              desc_ptr->Type == EfiLoaderData ||
-              (!map_bs &&
-               (desc_ptr->Type == EfiBootServicesCode ||
-                desc_ptr->Type == EfiBootServicesData))) )
+        if ( desc_ptr->Attribute & EFI_MEMORY_RUNTIME )
+            /* nothing */;
+        else if ( (desc_ptr->Attribute & EFI_MEMORY_WB) &&
+                  (desc_ptr->Type == EfiConventionalMemory ||
+                   desc_ptr->Type == EfiLoaderCode ||
+                   desc_ptr->Type == EfiLoaderData ||
+                   (!map_bs &&
+                    (desc_ptr->Type == EfiBootServicesCode ||
+                     desc_ptr->Type == EfiBootServicesData))) )
         {
             if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
             {
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -185,7 +185,9 @@ static void __init efi_arch_process_memo
             /* fall through */
         case EfiLoaderCode:
         case EfiLoaderData:
-            if ( desc->Attribute & EFI_MEMORY_WB )
+            if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+                type = E820_RESERVED;
+            else if ( desc->Attribute & EFI_MEMORY_WB )
                 type = E820_RAM;
             else
         case EfiUnusableMemory:

On Thu, Oct 06, 2022 at 10:40:56AM +0200, Jan Beulich wrote:
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried
> to do so, bypassing Xen's memory management), hence that type's handling

Strictly speaking I don't think dom0 needs to bypass Xen's memory
management, just overwriting the page would be bad enough for runtime
services to not work correctly I would think.

> can be left alone.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm
> Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

On 06.10.2022 10:53, Roger Pau Monné wrote:
> On Thu, Oct 06, 2022 at 10:40:56AM +0200, Jan Beulich wrote:
>> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
>> higher priority than the type of the range. To avoid accessing memory at
>> runtime which was re-used for other purposes, make
>> efi_arch_process_memory_map() follow suit. While on x86 in theory the
>> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
>> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried
>> to do so, bypassing Xen's memory management), hence that type's handling
> 
> Strictly speaking I don't think dom0 needs to bypass Xen's memory
> management, just overwriting the page would be bad enough for runtime
> services to not work correctly I would think.

Then how about:

"While on x86 in theory the same would apply to EfiACPIReclaimMemory, we don't
 actually "reclaim" or clobber E820_ACPI memory there (and it would be a bug if
 the Dom0 kernel tried to reclaim the range, bypassing Xen's memory management,
 plus it would be at least bogus if it clobbered that space), hence that type's
 handling can be left alone."

I didn't think the clobbering aspect needed pointing out, as the same applies
to all other memory which Dom0 is able to access beyond its actual allocation.

>> can be left alone.
>>
>> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
>> Fixes: facac0af87ef ("x86-64: EFI runtime code")
>> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm
>> Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan

On Thu, Oct 06, 2022 at 10:58:43AM +0200, Jan Beulich wrote:
> On 06.10.2022 10:53, Roger Pau Monné wrote:
> > On Thu, Oct 06, 2022 at 10:40:56AM +0200, Jan Beulich wrote:
> >> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> >> higher priority than the type of the range. To avoid accessing memory at
> >> runtime which was re-used for other purposes, make
> >> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> >> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> >> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried
> >> to do so, bypassing Xen's memory management), hence that type's handling
> > 
> > Strictly speaking I don't think dom0 needs to bypass Xen's memory
> > management, just overwriting the page would be bad enough for runtime
> > services to not work correctly I would think.
> 
> Then how about:
> 
> "While on x86 in theory the same would apply to EfiACPIReclaimMemory, we don't
>  actually "reclaim" or clobber E820_ACPI memory there (and it would be a bug if
>  the Dom0 kernel tried to reclaim the range, bypassing Xen's memory management,
>  plus it would be at least bogus if it clobbered that space), hence that type's
>  handling can be left alone."
> 
> I didn't think the clobbering aspect needed pointing out, as the same applies
> to all other memory which Dom0 is able to access beyond its actual allocation.

I think it makes it clear that just clobbering it from dom0 could
cause issues to runtime services.  I guess it can be extrapolated that
clobbering is also bad if reclaiming is.

Thanks, Roger.

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Subject: [PATCH v2][4.17] EFI: don't convert memory marked for runtime use
> to ordinary RAM
> 
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried
> to do so, bypassing Xen's memory management), hence that type's handling
> can be left alone.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm
> Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> ---
> v2: Amend description.