diff mbox series

xen/arm: during efi boot, improve the check for usable memory

Message ID 20200106182620.19505-1-sstabellini@kernel.org (mailing list archive)
State Superseded
Headers show
Series xen/arm: during efi boot, improve the check for usable memory | expand

Commit Message

Stefano Stabellini Jan. 6, 2020, 6:26 p.m. UTC
When booting via EFI, the EFI memory map has information about memory
regions and their type. Improve the check for the type and attribute of
each memory region to figure out whether it is usable memory or not.
This patch brings the check on par with Linux and makes more memory
reusable as normal memory by Xen.

Reported-by: Roman Shaposhnik <roman@zededa.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
---
 xen/arch/arm/efi/efi-boot.h | 11 +++++++----
 xen/include/efi/efidef.h    |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

Comments

Julien Grall Jan. 6, 2020, 7:56 p.m. UTC | #1
Hi Stefano,

On 06/01/2020 18:26, Stefano Stabellini wrote:
> When booting via EFI, the EFI memory map has information about memory
> regions and their type. Improve the check for the type and attribute of
> each memory region to figure out whether it is usable memory or not.
> This patch brings the check on par with Linux and makes more memory
> reusable as normal memory by Xen.

Please mention the version of Linux used.

Furthermore, as I pointed out in the original thread, it may not be 
correct for Xen to use the exact same checks as Linux. More importantly 
any change in behavior should be explained in the commit message. For 
instance...

> 
> Reported-by: Roman Shaposhnik <roman@zededa.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Julien Grall <julien@xen.org>
> ---
>   xen/arch/arm/efi/efi-boot.h | 11 +++++++----
>   xen/include/efi/efidef.h    |  1 +
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index d7bf934077..5d1d526d17 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -149,10 +149,13 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
>   
>       for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
>       {
> -        if ( desc_ptr->Type == EfiConventionalMemory ||
> -             (!map_bs &&

... the behavior when the option /mapbs is given to the EFI stub will 
now change. Why such change?

> -              (desc_ptr->Type == EfiBootServicesCode ||
> -               desc_ptr->Type == EfiBootServicesData)) )
> +        if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
> +             (desc_ptr->Type == EfiConventionalMemory ||
> +              desc_ptr->Type == EfiLoaderCode ||
> +              desc_ptr->Type == EfiLoaderData ||
> +              desc_ptr->Type == EfiPersistentMemory ||

I am not entirely convince this is the right thing to do. From my 
understanding, a region marked as EfiPersistentMemory will keep the 
state of memory accross power cycle.

As the memory will be given to the Xen allocator directly, this means 
some domains may randomly have there data placed in the NVM. Wouldn't 
this be considered as a leak of data?

> +              desc_ptr->Type == EfiBootServicesCode ||
> +              desc_ptr->Type == EfiBootServicesData) )
>           {
>               if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
>               {
> diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
> index 86a7e111bf..f46207840f 100644
> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -147,6 +147,7 @@ typedef enum {
>       EfiMemoryMappedIO,
>       EfiMemoryMappedIOPortSpace,
>       EfiPalCode,
> +    EfiPersistentMemory,
>       EfiMaxMemoryType
>   } EFI_MEMORY_TYPE;
>   
> 

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index d7bf934077..5d1d526d17 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -149,10 +149,13 @@  static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
 
     for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
     {
-        if ( desc_ptr->Type == EfiConventionalMemory ||
-             (!map_bs &&
-              (desc_ptr->Type == EfiBootServicesCode ||
-               desc_ptr->Type == EfiBootServicesData)) )
+        if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
+             (desc_ptr->Type == EfiConventionalMemory ||
+              desc_ptr->Type == EfiLoaderCode ||
+              desc_ptr->Type == EfiLoaderData ||
+              desc_ptr->Type == EfiPersistentMemory ||
+              desc_ptr->Type == EfiBootServicesCode ||
+              desc_ptr->Type == EfiBootServicesData) )
         {
             if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
             {
diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
index 86a7e111bf..f46207840f 100644
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -147,6 +147,7 @@  typedef enum {
     EfiMemoryMappedIO,
     EfiMemoryMappedIOPortSpace,
     EfiPalCode,
+    EfiPersistentMemory,
     EfiMaxMemoryType
 } EFI_MEMORY_TYPE;