[PATCH 0/4] misc EFI adjustments

[PATCH 0/4] misc EFI adjustments

[Jan Beulich]

1: x86/EFI: fix EFI_MEMORY_WP handling
2: EFI/early: add /mapbs to map EfiBootServices{Code,Data}
3: EFI: support default attributes to map Runtime service areas with none given
4: x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5 

Patches 2 and 3 are based on earlier work by Konrad.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/4] x86/EFI: fix EFI_MEMORY_WP handling

[Jan Beulich]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1195,7 +1195,7 @@ void __init efi_init_memory(void)
         }
 
         if ( desc->Attribute & EFI_MEMORY_WP )
-            prot &= _PAGE_RW;
+            prot &= ~_PAGE_RW;
         if ( desc->Attribute & EFI_MEMORY_XP )
             prot |= _PAGE_NX;
 




[-- Attachment #2: EFI-RS-write-protect.patch --]
[-- Type: text/plain, Size: 417 bytes --]

x86/EFI: fix EFI_MEMORY_WP handling

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1195,7 +1195,7 @@ void __init efi_init_memory(void)
         }
 
         if ( desc->Attribute & EFI_MEMORY_WP )
-            prot &= _PAGE_RW;
+            prot &= ~_PAGE_RW;
         if ( desc->Attribute & EFI_MEMORY_XP )
             prot |= _PAGE_NX;
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] x86/EFI: fix EFI_MEMORY_WP handling

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 630 bytes --]

On 09/06/15 14:52, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1195,7 +1195,7 @@ void __init efi_init_memory(void)
>          }
>  
>          if ( desc->Attribute & EFI_MEMORY_WP )
> -            prot &= _PAGE_RW;
> +            prot &= ~_PAGE_RW;
>          if ( desc->Attribute & EFI_MEMORY_XP )
>              prot |= _PAGE_NX;
>  
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 1551 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Jan Beulich]

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

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

To help on certain platforms to run.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -148,12 +148,16 @@ static void __init efi_arch_process_memo
 
         switch ( desc->Type )
         {
-        default:
-            type = E820_RESERVED;
-            break;
-        case EfiConventionalMemory:
         case EfiBootServicesCode:
         case EfiBootServicesData:
+            if ( map_bs )
+            {
+        default:
+                type = E820_RESERVED;
+                break;
+            }
+            /* fall through */
+        case EfiConventionalMemory:
             if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
                  len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
                 cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -84,6 +84,7 @@ static SIMPLE_TEXT_OUTPUT_INTERFACE *__i
 static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
 
 static UINT32 __initdata mdesc_ver;
+static bool_t __initdata map_bs;
 
 static struct file __initdata cfg;
 static struct file __initdata kernel;
@@ -753,6 +754,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
             {
                 if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
                     base_video = 1;
+                else if ( wstrcmp(ptr + 1, L"mapbs") == 0 )
+                    map_bs = 1;
                 else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
                     cfg_file_name = ptr + 5;
                 else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
@@ -762,6 +765,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
                 {
                     PrintStr(L"Xen EFI Loader options:\r\n");
                     PrintStr(L"-basevideo   retain current video mode\r\n");
+                    PrintStr(L"-mapbs       map EfiBootServices{Code,Data}\r\n");
                     PrintStr(L"-cfg=<file>  specify configuration file\r\n");
                     PrintStr(L"-help, -?    display this help\r\n");
                     blexit(NULL);
@@ -1158,7 +1162,8 @@ void __init efi_init_memory(void)
     } *extra, *extra_head = NULL;
 #endif
 
-    printk(XENLOG_INFO "EFI memory map:\n");
+    printk(XENLOG_INFO "EFI memory map:%s\n",
+           map_bs ? " (mapping BootServices)" : "");
     for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
     {
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
@@ -1171,7 +1176,11 @@ void __init efi_init_memory(void)
                desc->PhysicalStart, desc->PhysicalStart + len - 1,
                desc->Type, desc->Attribute);
 
-        if ( !efi_rs_enable || !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+        if ( !efi_rs_enable ||
+             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
+              (!map_bs ||
+               (desc->Type != EfiBootServicesCode &&
+                desc->Type != EfiBootServicesData))) )
             continue;
 
         desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
@@ -1257,7 +1266,10 @@ void __init efi_init_memory(void)
     {
         const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
 
-        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) &&
+        if ( ((desc->Attribute & EFI_MEMORY_RUNTIME) ||
+              (map_bs &&
+               (desc->Type == EfiBootServicesCode ||
+                desc->Type == EfiBootServicesData))) &&
              desc->VirtualStart != INVALID_VIRTUAL_ADDRESS &&
              desc->VirtualStart != desc->PhysicalStart )
             copy_mapping(PFN_DOWN(desc->PhysicalStart),




[-- Attachment #2: EFI-optionally-map-BS.patch --]
[-- Type: text/plain, Size: 3892 bytes --]

EFI/early: add /mapbs to map EfiBootServices{Code,Data}

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

To help on certain platforms to run.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -148,12 +148,16 @@ static void __init efi_arch_process_memo
 
         switch ( desc->Type )
         {
-        default:
-            type = E820_RESERVED;
-            break;
-        case EfiConventionalMemory:
         case EfiBootServicesCode:
         case EfiBootServicesData:
+            if ( map_bs )
+            {
+        default:
+                type = E820_RESERVED;
+                break;
+            }
+            /* fall through */
+        case EfiConventionalMemory:
             if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
                  len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
                 cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -84,6 +84,7 @@ static SIMPLE_TEXT_OUTPUT_INTERFACE *__i
 static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
 
 static UINT32 __initdata mdesc_ver;
+static bool_t __initdata map_bs;
 
 static struct file __initdata cfg;
 static struct file __initdata kernel;
@@ -753,6 +754,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
             {
                 if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
                     base_video = 1;
+                else if ( wstrcmp(ptr + 1, L"mapbs") == 0 )
+                    map_bs = 1;
                 else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
                     cfg_file_name = ptr + 5;
                 else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
@@ -762,6 +765,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
                 {
                     PrintStr(L"Xen EFI Loader options:\r\n");
                     PrintStr(L"-basevideo   retain current video mode\r\n");
+                    PrintStr(L"-mapbs       map EfiBootServices{Code,Data}\r\n");
                     PrintStr(L"-cfg=<file>  specify configuration file\r\n");
                     PrintStr(L"-help, -?    display this help\r\n");
                     blexit(NULL);
@@ -1158,7 +1162,8 @@ void __init efi_init_memory(void)
     } *extra, *extra_head = NULL;
 #endif
 
-    printk(XENLOG_INFO "EFI memory map:\n");
+    printk(XENLOG_INFO "EFI memory map:%s\n",
+           map_bs ? " (mapping BootServices)" : "");
     for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
     {
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
@@ -1171,7 +1176,11 @@ void __init efi_init_memory(void)
                desc->PhysicalStart, desc->PhysicalStart + len - 1,
                desc->Type, desc->Attribute);
 
-        if ( !efi_rs_enable || !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+        if ( !efi_rs_enable ||
+             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
+              (!map_bs ||
+               (desc->Type != EfiBootServicesCode &&
+                desc->Type != EfiBootServicesData))) )
             continue;
 
         desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
@@ -1257,7 +1266,10 @@ void __init efi_init_memory(void)
     {
         const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
 
-        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) &&
+        if ( ((desc->Attribute & EFI_MEMORY_RUNTIME) ||
+              (map_bs &&
+               (desc->Type == EfiBootServicesCode ||
+                desc->Type == EfiBootServicesData))) &&
              desc->VirtualStart != INVALID_VIRTUAL_ADDRESS &&
              desc->VirtualStart != desc->PhysicalStart )
             copy_mapping(PFN_DOWN(desc->PhysicalStart),

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4191 bytes --]

On 09/06/15 14:53, Jan Beulich wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> To help on certain platforms to run.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -148,12 +148,16 @@ static void __init efi_arch_process_memo
>  
>          switch ( desc->Type )
>          {
> -        default:
> -            type = E820_RESERVED;
> -            break;
> -        case EfiConventionalMemory:
>          case EfiBootServicesCode:
>          case EfiBootServicesData:
> +            if ( map_bs )

if ( !map_bs ) surely?  It is the else case which sets a type of E820_RAM.

~Andrew

> +            {
> +        default:
> +                type = E820_RESERVED;
> +                break;
> +            }
> +            /* fall through */
> +        case EfiConventionalMemory:
>              if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
>                   len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
>                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -84,6 +84,7 @@ static SIMPLE_TEXT_OUTPUT_INTERFACE *__i
>  static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
>  
>  static UINT32 __initdata mdesc_ver;
> +static bool_t __initdata map_bs;
>  
>  static struct file __initdata cfg;
>  static struct file __initdata kernel;
> @@ -753,6 +754,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>              {
>                  if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
>                      base_video = 1;
> +                else if ( wstrcmp(ptr + 1, L"mapbs") == 0 )
> +                    map_bs = 1;
>                  else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
>                      cfg_file_name = ptr + 5;
>                  else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
> @@ -762,6 +765,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>                  {
>                      PrintStr(L"Xen EFI Loader options:\r\n");
>                      PrintStr(L"-basevideo   retain current video mode\r\n");
> +                    PrintStr(L"-mapbs       map EfiBootServices{Code,Data}\r\n");
>                      PrintStr(L"-cfg=<file>  specify configuration file\r\n");
>                      PrintStr(L"-help, -?    display this help\r\n");
>                      blexit(NULL);
> @@ -1158,7 +1162,8 @@ void __init efi_init_memory(void)
>      } *extra, *extra_head = NULL;
>  #endif
>  
> -    printk(XENLOG_INFO "EFI memory map:\n");
> +    printk(XENLOG_INFO "EFI memory map:%s\n",
> +           map_bs ? " (mapping BootServices)" : "");
>      for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>      {
>          EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> @@ -1171,7 +1176,11 @@ void __init efi_init_memory(void)
>                 desc->PhysicalStart, desc->PhysicalStart + len - 1,
>                 desc->Type, desc->Attribute);
>  
> -        if ( !efi_rs_enable || !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> +        if ( !efi_rs_enable ||
> +             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +              (!map_bs ||
> +               (desc->Type != EfiBootServicesCode &&
> +                desc->Type != EfiBootServicesData))) )
>              continue;
>  
>          desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> @@ -1257,7 +1266,10 @@ void __init efi_init_memory(void)
>      {
>          const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>  
> -        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +        if ( ((desc->Attribute & EFI_MEMORY_RUNTIME) ||
> +              (map_bs &&
> +               (desc->Type == EfiBootServicesCode ||
> +                desc->Type == EfiBootServicesData))) &&
>               desc->VirtualStart != INVALID_VIRTUAL_ADDRESS &&
>               desc->VirtualStart != desc->PhysicalStart )
>              copy_mapping(PFN_DOWN(desc->PhysicalStart),
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5336 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Konrad Rzeszutek Wilk]

On Tue, Jun 09, 2015 at 03:03:18PM +0100, Andrew Cooper wrote:
> On 09/06/15 14:53, Jan Beulich wrote:
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > To help on certain platforms to run.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -148,12 +148,16 @@ static void __init efi_arch_process_memo
> >  
> >          switch ( desc->Type )
> >          {
> > -        default:
> > -            type = E820_RESERVED;
> > -            break;
> > -        case EfiConventionalMemory:
> >          case EfiBootServicesCode:
> >          case EfiBootServicesData:
> > +            if ( map_bs )
> 
> if ( !map_bs ) surely?  It is the else case which sets a type of E820_RAM.

We want the EfiBootServices{Code|Data} to be E820_RESERVED and this
sets it to that if map_bs is set.

If map_bs is not set then we treat it as E820_RAM.

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Andrew Cooper]

On 09/06/15 15:11, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 09, 2015 at 03:03:18PM +0100, Andrew Cooper wrote:
>> On 09/06/15 14:53, Jan Beulich wrote:
>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> To help on certain platforms to run.
>>>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -148,12 +148,16 @@ static void __init efi_arch_process_memo
>>>  
>>>          switch ( desc->Type )
>>>          {
>>> -        default:
>>> -            type = E820_RESERVED;
>>> -            break;
>>> -        case EfiConventionalMemory:
>>>          case EfiBootServicesCode:
>>>          case EfiBootServicesData:
>>> +            if ( map_bs )
>> if ( !map_bs ) surely?  It is the else case which sets a type of E820_RAM.
> We want the EfiBootServices{Code|Data} to be E820_RESERVED and this
> sets it to that if map_bs is set.
>
> If map_bs is not set then we treat it as E820_RAM.

Ah of course - sorry for the confusion.

~Andrew

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Jan Beulich]

>>> On 09.06.15 at 16:03, <andrew.cooper3@citrix.com> wrote:
> On 09/06/15 14:53, Jan Beulich wrote:
>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> To help on certain platforms to run.
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -148,12 +148,16 @@ static void __init efi_arch_process_memo
>>  
>>          switch ( desc->Type )
>>          {
>> -        default:
>> -            type = E820_RESERVED;
>> -            break;
>> -        case EfiConventionalMemory:
>>          case EfiBootServicesCode:
>>          case EfiBootServicesData:
>> +            if ( map_bs )
> 
> if ( !map_bs ) surely?  It is the else case which sets a type of E820_RAM.

No, we want it reserved when map_bs and treated as normal RAM
when !map_bs.

Jan

>> +            {
>> +        default:
>> +                type = E820_RESERVED;
>> +                break;
>> +            }
>> +            /* fall through */
>> +        case EfiConventionalMemory:
>>              if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
>>                   len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
>>                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Ian Campbell]

On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> To help on certain platforms to run.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

To be effective (or at least consistent) on ARM, would we also want to
change its efi_process_memory_map_bootinfo:
        if ( desc_ptr->Type == EfiConventionalMemory
             || desc_ptr->Type == EfiBootServicesCode
             || desc_ptr->Type == EfiBootServicesData )
to include a check on map_bs?

Ian.

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Jan Beulich]

>>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> 
>> To help on certain platforms to run.
>> 
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> To be effective (or at least consistent) on ARM, would we also want to
> change its efi_process_memory_map_bootinfo:
>         if ( desc_ptr->Type == EfiConventionalMemory
>              || desc_ptr->Type == EfiBootServicesCode
>              || desc_ptr->Type == EfiBootServicesData )
> to include a check on map_bs?

I'm not convinced, but I also don't know the history of why boot
services areas are being included here in the first place - Roy?
I.e. if the checks weren't there already, I'd agree that an addition
similar to the other ones would be needed here, but with the x86
side getting relaxed I don't see why you would want to tighten the
ARM side at the same time.

Jan

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Ian Campbell]

On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> 
> >> To help on certain platforms to run.
> >> 
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > To be effective (or at least consistent) on ARM, would we also want to
> > change its efi_process_memory_map_bootinfo:
> >         if ( desc_ptr->Type == EfiConventionalMemory
> >              || desc_ptr->Type == EfiBootServicesCode
> >              || desc_ptr->Type == EfiBootServicesData )
> > to include a check on map_bs?
> 
> I'm not convinced, but I also don't know the history of why boot
> services areas are being included here in the first place - Roy?
> I.e. if the checks weren't there already, I'd agree that an addition
> similar to the other ones would be needed here, but with the x86
> side getting relaxed I don't see why you would want to tighten the
> ARM side at the same time.

I read it backwards and thought this was currently excluding them like
x86 does.

Am I correct that the stricter x86 behaviour is per the spec, and this
new option is a workaround for non-compliant systems?

If so unless Roy knows of a reason why these should be mapped on ARM be
default (i.e. the ARM spec differs) I'd be inclined to suggesting the
default be stricter on ARM too for consistency.

Ian.

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Jan Beulich]

>>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
> On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
>> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
>> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
>> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> >> 
>> >> To help on certain platforms to run.
>> >> 
>> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> > 
>> > To be effective (or at least consistent) on ARM, would we also want to
>> > change its efi_process_memory_map_bootinfo:
>> >         if ( desc_ptr->Type == EfiConventionalMemory
>> >              || desc_ptr->Type == EfiBootServicesCode
>> >              || desc_ptr->Type == EfiBootServicesData )
>> > to include a check on map_bs?
>> 
>> I'm not convinced, but I also don't know the history of why boot
>> services areas are being included here in the first place - Roy?
>> I.e. if the checks weren't there already, I'd agree that an addition
>> similar to the other ones would be needed here, but with the x86
>> side getting relaxed I don't see why you would want to tighten the
>> ARM side at the same time.
> 
> I read it backwards and thought this was currently excluding them like
> x86 does.
> 
> Am I correct that the stricter x86 behaviour is per the spec, and this
> new option is a workaround for non-compliant systems?

Yes.

> If so unless Roy knows of a reason why these should be mapped on ARM be
> default (i.e. the ARM spec differs) I'd be inclined to suggesting the
> default be stricter on ARM too for consistency.

I agree, but would want this to be a separate patch then in any event.
I.e. I'm intending to commit the whole series shortly.

Jan

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Ian Campbell]

On Wed, 2015-06-10 at 10:37 +0100, Jan Beulich wrote:
> >>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
> > On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
> >> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
> >> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
> >> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> >> 
> >> >> To help on certain platforms to run.
> >> >> 
> >> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> > 
> >> > To be effective (or at least consistent) on ARM, would we also want to
> >> > change its efi_process_memory_map_bootinfo:
> >> >         if ( desc_ptr->Type == EfiConventionalMemory
> >> >              || desc_ptr->Type == EfiBootServicesCode
> >> >              || desc_ptr->Type == EfiBootServicesData )
> >> > to include a check on map_bs?
> >> 
> >> I'm not convinced, but I also don't know the history of why boot
> >> services areas are being included here in the first place - Roy?
> >> I.e. if the checks weren't there already, I'd agree that an addition
> >> similar to the other ones would be needed here, but with the x86
> >> side getting relaxed I don't see why you would want to tighten the
> >> ARM side at the same time.
> > 
> > I read it backwards and thought this was currently excluding them like
> > x86 does.
> > 
> > Am I correct that the stricter x86 behaviour is per the spec, and this
> > new option is a workaround for non-compliant systems?
> 
> Yes.
> 
> > If so unless Roy knows of a reason why these should be mapped on ARM be
> > default (i.e. the ARM spec differs) I'd be inclined to suggesting the
> > default be stricter on ARM too for consistency.
> 
> I agree, but would want this to be a separate patch then in any event.
> I.e. I'm intending to commit the whole series shortly.

Sure, lets wait and see what Roy says

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Ian Campbell]

On Wed, 2015-06-10 at 11:00 +0100, Ian Campbell wrote:
> On Wed, 2015-06-10 at 10:37 +0100, Jan Beulich wrote:
> > >>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
> > > On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
> > >> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
> > >> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
> > >> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >> >> 
> > >> >> To help on certain platforms to run.
> > >> >> 
> > >> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >> > 
> > >> > To be effective (or at least consistent) on ARM, would we also want to
> > >> > change its efi_process_memory_map_bootinfo:
> > >> >         if ( desc_ptr->Type == EfiConventionalMemory
> > >> >              || desc_ptr->Type == EfiBootServicesCode
> > >> >              || desc_ptr->Type == EfiBootServicesData )
> > >> > to include a check on map_bs?
> > >> 
> > >> I'm not convinced, but I also don't know the history of why boot
> > >> services areas are being included here in the first place - Roy?
> > >> I.e. if the checks weren't there already, I'd agree that an addition
> > >> similar to the other ones would be needed here, but with the x86
> > >> side getting relaxed I don't see why you would want to tighten the
> > >> ARM side at the same time.
> > > 
> > > I read it backwards and thought this was currently excluding them like
> > > x86 does.
> > > 
> > > Am I correct that the stricter x86 behaviour is per the spec, and this
> > > new option is a workaround for non-compliant systems?
> > 
> > Yes.
> > 
> > > If so unless Roy knows of a reason why these should be mapped on ARM be
> > > default (i.e. the ARM spec differs) I'd be inclined to suggesting the
> > > default be stricter on ARM too for consistency.
> > 
> > I agree, but would want this to be a separate patch then in any event.
> > I.e. I'm intending to commit the whole series shortly.
> 
> Sure, lets wait and see what Roy says

... before doing anything more than committing the whole series I mean.
Sorry for being vague!

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Roy Franz]

On Wed, Jun 10, 2015 at 2:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
>> On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
>>> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
>>> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
>>> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> >>
>>> >> To help on certain platforms to run.
>>> >>
>>> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> >
>>> > To be effective (or at least consistent) on ARM, would we also want to
>>> > change its efi_process_memory_map_bootinfo:
>>> >         if ( desc_ptr->Type == EfiConventionalMemory
>>> >              || desc_ptr->Type == EfiBootServicesCode
>>> >              || desc_ptr->Type == EfiBootServicesData )
>>> > to include a check on map_bs?
>>>
>>> I'm not convinced, but I also don't know the history of why boot
>>> services areas are being included here in the first place - Roy?
>>> I.e. if the checks weren't there already, I'd agree that an addition
>>> similar to the other ones would be needed here, but with the x86
>>> side getting relaxed I don't see why you would want to tighten the
>>> ARM side at the same time.

The boot services code/data is "memory available for general use", just
like EfiConventionalMemory after ExitBootServices() is called.  Since the memory
map being created here is going to be used after ExitBootServices() is called,
I think this matches the UEFI specification. This matches x86 behavior before
the patch (modulo some address range checks used to set cfg.addr.)

>>
>> I read it backwards and thought this was currently excluding them like
>> x86 does.
>>
>> Am I correct that the stricter x86 behaviour is per the spec, and this
>> new option is a workaround for non-compliant systems?
>
> Yes.
>
>> If so unless Roy knows of a reason why these should be mapped on ARM be
>> default (i.e. the ARM spec differs) I'd be inclined to suggesting the
>> default be stricter on ARM too for consistency.
>
> I agree, but would want this to be a separate patch then in any event.
> I.e. I'm intending to commit the whole series shortly.
>
> Jan
>

The open question regarding the Arm code is whether we want/need this workaround
for Arm as well, right?  I don't see a reason why firmware bugs
regarding memory allocation
types would be x86 specific, so we could see firmware broken the same
way on arm platforms.
Is !map_bs the default?

I think "reserve_bs" would be a better name, as I think of it as being
'mapped' when it is added
as normal memory to the memory map.  I find the terminology in the
patch a bit generic/confusing.


Roy

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Andrew Cooper]

On 10/06/15 18:22, Roy Franz wrote:
>
>>> I read it backwards and thought this was currently excluding them like
>>> x86 does.
>>>
>>> Am I correct that the stricter x86 behaviour is per the spec, and this
>>> new option is a workaround for non-compliant systems?
>> Yes.
>>
>>> If so unless Roy knows of a reason why these should be mapped on ARM be
>>> default (i.e. the ARM spec differs) I'd be inclined to suggesting the
>>> default be stricter on ARM too for consistency.
>> I agree, but would want this to be a separate patch then in any event.
>> I.e. I'm intending to commit the whole series shortly.
>>
>> Jan
>>
> The open question regarding the Arm code is whether we want/need this workaround
> for Arm as well, right?  I don't see a reason why firmware bugs
> regarding memory allocation
> types would be x86 specific, so we could see firmware broken the same
> way on arm platforms.

It would be nice to hope that the arm side was all coded to spec. 
However, the realist in me would expect to see the same kinds of
mistakes on any architecture.

> Is !map_bs the default?
>
> I think "reserve_bs" would be a better name, as I think of it as being
> 'mapped' when it is added
> as normal memory to the memory map.  I find the terminology in the
> patch a bit generic/confusing.

Technically, it is "map boot services code/data for runtime services",
as it is a workaround for firmware which doesn't correctly avoid using
__init/__initdata at runtime.

I don't agree that "reserve_bs" is any better, but can't think of a 3rd
alternative which would be better than either.

~Andrew

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Konrad Rzeszutek Wilk]

On Wed, Jun 10, 2015 at 07:12:07PM +0100, Andrew Cooper wrote:
> On 10/06/15 18:22, Roy Franz wrote:
> >
> >>> I read it backwards and thought this was currently excluding them like
> >>> x86 does.
> >>>
> >>> Am I correct that the stricter x86 behaviour is per the spec, and this
> >>> new option is a workaround for non-compliant systems?
> >> Yes.
> >>
> >>> If so unless Roy knows of a reason why these should be mapped on ARM be
> >>> default (i.e. the ARM spec differs) I'd be inclined to suggesting the
> >>> default be stricter on ARM too for consistency.
> >> I agree, but would want this to be a separate patch then in any event.
> >> I.e. I'm intending to commit the whole series shortly.
> >>
> >> Jan
> >>
> > The open question regarding the Arm code is whether we want/need this workaround
> > for Arm as well, right?  I don't see a reason why firmware bugs
> > regarding memory allocation
> > types would be x86 specific, so we could see firmware broken the same
> > way on arm platforms.
> 
> It would be nice to hope that the arm side was all coded to spec. 
> However, the realist in me would expect to see the same kinds of
> mistakes on any architecture.
> 
> > Is !map_bs the default?
> >
> > I think "reserve_bs" would be a better name, as I think of it as being
> > 'mapped' when it is added
> > as normal memory to the memory map.  I find the terminology in the
> > patch a bit generic/confusing.
> 
> Technically, it is "map boot services code/data for runtime services",
> as it is a workaround for firmware which doesn't correctly avoid using
> __init/__initdata at runtime.
> 
> I don't agree that "reserve_bs" is any better, but can't think of a 3rd
> alternative which would be better than either.
> 

fix_bs ?

<chuckles>
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Andrew Cooper]

On 10/06/15 20:48, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 10, 2015 at 07:12:07PM +0100, Andrew Cooper wrote:
>> On 10/06/15 18:22, Roy Franz wrote:
>>>>> I read it backwards and thought this was currently excluding them like
>>>>> x86 does.
>>>>>
>>>>> Am I correct that the stricter x86 behaviour is per the spec, and this
>>>>> new option is a workaround for non-compliant systems?
>>>> Yes.
>>>>
>>>>> If so unless Roy knows of a reason why these should be mapped on ARM be
>>>>> default (i.e. the ARM spec differs) I'd be inclined to suggesting the
>>>>> default be stricter on ARM too for consistency.
>>>> I agree, but would want this to be a separate patch then in any event.
>>>> I.e. I'm intending to commit the whole series shortly.
>>>>
>>>> Jan
>>>>
>>> The open question regarding the Arm code is whether we want/need this workaround
>>> for Arm as well, right?  I don't see a reason why firmware bugs
>>> regarding memory allocation
>>> types would be x86 specific, so we could see firmware broken the same
>>> way on arm platforms.
>> It would be nice to hope that the arm side was all coded to spec. 
>> However, the realist in me would expect to see the same kinds of
>> mistakes on any architecture.
>>
>>> Is !map_bs the default?
>>>
>>> I think "reserve_bs" would be a better name, as I think of it as being
>>> 'mapped' when it is added
>>> as normal memory to the memory map.  I find the terminology in the
>>> patch a bit generic/confusing.
>> Technically, it is "map boot services code/data for runtime services",
>> as it is a workaround for firmware which doesn't correctly avoid using
>> __init/__initdata at runtime.
>>
>> I don't agree that "reserve_bs" is any better, but can't think of a 3rd
>> alternative which would be better than either.
>>
> fix_bs ?
>
> <chuckles>

:)

~Andrew

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Roy Franz]

On Wed, Jun 10, 2015 at 11:12 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 10/06/15 18:22, Roy Franz wrote:
>>
>>>> I read it backwards and thought this was currently excluding them like
>>>> x86 does.
>>>>
>>>> Am I correct that the stricter x86 behaviour is per the spec, and this
>>>> new option is a workaround for non-compliant systems?
>>> Yes.
>>>
>>>> If so unless Roy knows of a reason why these should be mapped on ARM be
>>>> default (i.e. the ARM spec differs) I'd be inclined to suggesting the
>>>> default be stricter on ARM too for consistency.
>>> I agree, but would want this to be a separate patch then in any event.
>>> I.e. I'm intending to commit the whole series shortly.
>>>
>>> Jan
>>>
>> The open question regarding the Arm code is whether we want/need this workaround
>> for Arm as well, right?  I don't see a reason why firmware bugs
>> regarding memory allocation
>> types would be x86 specific, so we could see firmware broken the same
>> way on arm platforms.
>
> It would be nice to hope that the arm side was all coded to spec.
> However, the realist in me would expect to see the same kinds of
> mistakes on any architecture.
>
>> Is !map_bs the default?
>>
>> I think "reserve_bs" would be a better name, as I think of it as being
>> 'mapped' when it is added
>> as normal memory to the memory map.  I find the terminology in the
>> patch a bit generic/confusing.
>
> Technically, it is "map boot services code/data for runtime services",
> as it is a workaround for firmware which doesn't correctly avoid using
> __init/__initdata at runtime.
>
> I don't agree that "reserve_bs" is any better, but can't think of a 3rd
> alternative which would be better than either.

OK, makes sense.  I was focusing on it getting marked as E820_reserved,
and missed the later mapping with runtime services.  I agree that reserve_bs
is not better.  I won't bikeshed any further :)

>
> ~Andrew

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Jan Beulich]

>>> On 10.06.15 at 19:22, <roy.franz@linaro.org> wrote:
> On Wed, Jun 10, 2015 at 2:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
>>> On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
>>>> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
>>>> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
>>>> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> >>
>>>> >> To help on certain platforms to run.
>>>> >>
>>>> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> >
>>>> > To be effective (or at least consistent) on ARM, would we also want to
>>>> > change its efi_process_memory_map_bootinfo:
>>>> >         if ( desc_ptr->Type == EfiConventionalMemory
>>>> >              || desc_ptr->Type == EfiBootServicesCode
>>>> >              || desc_ptr->Type == EfiBootServicesData )
>>>> > to include a check on map_bs?
>>>>
>>>> I'm not convinced, but I also don't know the history of why boot
>>>> services areas are being included here in the first place - Roy?
>>>> I.e. if the checks weren't there already, I'd agree that an addition
>>>> similar to the other ones would be needed here, but with the x86
>>>> side getting relaxed I don't see why you would want to tighten the
>>>> ARM side at the same time.
> 
> The boot services code/data is "memory available for general use", just
> like EfiConventionalMemory after ExitBootServices() is called.  Since the 
> memory
> map being created here is going to be used after ExitBootServices() is 
> called,
> I think this matches the UEFI specification. This matches x86 behavior 
> before
> the patch (modulo some address range checks used to set cfg.addr.)

Ouch - looks like I read this the wrong way round, and Ian was right
in wanting a map_bs check added here:

EFI/ARM: don't treat EfiBootServices{Code,Data} as normal RAM under /mapbs

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -131,9 +131,10 @@ static EFI_STATUS __init efi_process_mem
 
     for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
     {
-        if ( desc_ptr->Type == EfiConventionalMemory
-             || desc_ptr->Type == EfiBootServicesCode
-             || desc_ptr->Type == EfiBootServicesData )
+        if ( desc_ptr->Type == EfiConventionalMemory ||
+             (!map_bs &&
+              (desc_ptr->Type == EfiBootServicesCode ||
+               desc_ptr->Type == EfiBootServicesData)) )
         {
             if ( i >= NR_MEM_BANKS )
             {

Jan

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Ian Campbell]

On Thu, 2015-06-11 at 07:34 +0100, Jan Beulich wrote:
> >>> On 10.06.15 at 19:22, <roy.franz@linaro.org> wrote:
> > On Wed, Jun 10, 2015 at 2:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
> >>> On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
> >>>> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
> >>>> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
> >>>> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>> >>
> >>>> >> To help on certain platforms to run.
> >>>> >>
> >>>> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> >
> >>>> > To be effective (or at least consistent) on ARM, would we also want to
> >>>> > change its efi_process_memory_map_bootinfo:
> >>>> >         if ( desc_ptr->Type == EfiConventionalMemory
> >>>> >              || desc_ptr->Type == EfiBootServicesCode
> >>>> >              || desc_ptr->Type == EfiBootServicesData )
> >>>> > to include a check on map_bs?
> >>>>
> >>>> I'm not convinced, but I also don't know the history of why boot
> >>>> services areas are being included here in the first place - Roy?
> >>>> I.e. if the checks weren't there already, I'd agree that an addition
> >>>> similar to the other ones would be needed here, but with the x86
> >>>> side getting relaxed I don't see why you would want to tighten the
> >>>> ARM side at the same time.
> > 
> > The boot services code/data is "memory available for general use", just
> > like EfiConventionalMemory after ExitBootServices() is called.  Since the 
> > memory
> > map being created here is going to be used after ExitBootServices() is 
> > called,
> > I think this matches the UEFI specification. This matches x86 behavior 
> > before
> > the patch (modulo some address range checks used to set cfg.addr.)
> 
> Ouch - looks like I read this the wrong way round, and Ian was right
> in wanting a map_bs check added here:

I should have the courage of my convictions!

> EFI/ARM: don't treat EfiBootServices{Code,Data} as normal RAM under /mapbs
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -131,9 +131,10 @@ static EFI_STATUS __init efi_process_mem
>  
>      for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
>      {
> -        if ( desc_ptr->Type == EfiConventionalMemory
> -             || desc_ptr->Type == EfiBootServicesCode
> -             || desc_ptr->Type == EfiBootServicesData )
> +        if ( desc_ptr->Type == EfiConventionalMemory ||
> +             (!map_bs &&
> +              (desc_ptr->Type == EfiBootServicesCode ||
> +               desc_ptr->Type == EfiBootServicesData)) )
>          {
>              if ( i >= NR_MEM_BANKS )
>              {
> 
> Jan
> 

Re: [PATCH 2/4] EFI/early: add /mapbs to map EfiBootServices{Code, Data}

[Roy Franz]

On Wed, Jun 10, 2015 at 11:34 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.06.15 at 19:22, <roy.franz@linaro.org> wrote:
>> On Wed, Jun 10, 2015 at 2:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 10.06.15 at 11:26, <ian.campbell@citrix.com> wrote:
>>>> On Wed, 2015-06-10 at 10:15 +0100, Jan Beulich wrote:
>>>>> >>> On 10.06.15 at 10:56, <ian.campbell@citrix.com> wrote:
>>>>> > On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
>>>>> >> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>>> >>
>>>>> >> To help on certain platforms to run.
>>>>> >>
>>>>> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> >
>>>>> > To be effective (or at least consistent) on ARM, would we also want to
>>>>> > change its efi_process_memory_map_bootinfo:
>>>>> >         if ( desc_ptr->Type == EfiConventionalMemory
>>>>> >              || desc_ptr->Type == EfiBootServicesCode
>>>>> >              || desc_ptr->Type == EfiBootServicesData )
>>>>> > to include a check on map_bs?
>>>>>
>>>>> I'm not convinced, but I also don't know the history of why boot
>>>>> services areas are being included here in the first place - Roy?
>>>>> I.e. if the checks weren't there already, I'd agree that an addition
>>>>> similar to the other ones would be needed here, but with the x86
>>>>> side getting relaxed I don't see why you would want to tighten the
>>>>> ARM side at the same time.
>>
>> The boot services code/data is "memory available for general use", just
>> like EfiConventionalMemory after ExitBootServices() is called.  Since the
>> memory
>> map being created here is going to be used after ExitBootServices() is
>> called,
>> I think this matches the UEFI specification. This matches x86 behavior
>> before
>> the patch (modulo some address range checks used to set cfg.addr.)
>
> Ouch - looks like I read this the wrong way round, and Ian was right
> in wanting a map_bs check added here:
>
> EFI/ARM: don't treat EfiBootServices{Code,Data} as normal RAM under /mapbs
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -131,9 +131,10 @@ static EFI_STATUS __init efi_process_mem
>
>      for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
>      {
> -        if ( desc_ptr->Type == EfiConventionalMemory
> -             || desc_ptr->Type == EfiBootServicesCode
> -             || desc_ptr->Type == EfiBootServicesData )
> +        if ( desc_ptr->Type == EfiConventionalMemory ||
> +             (!map_bs &&
> +              (desc_ptr->Type == EfiBootServicesCode ||
> +               desc_ptr->Type == EfiBootServicesData)) )
>          {
>              if ( i >= NR_MEM_BANKS )
>              {
>
> Jan
>

Reviewed-by: Roy Franz <roy.franz@linaro.org>

[PATCH 3/4] EFI: support default attributes to map Runtime service areas with none given

[Jan Beulich]

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

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

For example on Dell machines we see:

(XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
(XEN) Unknown cachability for MFNs 0xfed18-0xfed19

Let's allow them to be mapped as UC.

We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -617,12 +617,24 @@ Either force retrieval of monitor EDID i
 disable it (edid=no). This option should not normally be required
 except for debugging purposes.
 
-### efi-rs
-> `= <boolean>`
+### efi
+> `= List of [ rs | attr ]`
+
+All options are of boolean kind and can be prefixed with `no-` to
+effect the inverse meaning.
+
+> `rs`
 
 > Default: `true`
 
-Force or disable use of EFI runtime services.
+>> Force or disable use of EFI runtime services.
+
+> `attr=uc`
+
+> Default: `off`
+
+>> Allows mapping of RuntimeServices which have no cachability attribute
+>> set as UC.
 
 ### extra\_guest\_irqs
 > `= [<domU number>][,<dom0 number>]`
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1100,7 +1100,31 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
 #ifndef CONFIG_ARM /* TODO - runtime service support */
 
 static bool_t __initdata efi_rs_enable = 1;
-boolean_param("efi-rs", efi_rs_enable);
+static bool_t __initdata efi_map_uc;
+
+static void __init parse_efi_param(char *s)
+{
+    char *ss;
+
+    do {
+        bool_t val = !!strncmp(s, "no-", 3);
+
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        if ( !strcmp(s, "rs") )
+            efi_rs_enable = val;
+        else if ( !strcmp(s, "attr=uc") )
+            efi_map_uc = val;
+
+        s = ss + 1;
+    } while ( ss );
+}
+custom_param("efi", parse_efi_param);
 
 #ifndef USE_SET_VIRTUAL_ADDRESS_MAP
 static __init void copy_mapping(unsigned long mfn, unsigned long end,
@@ -1198,9 +1222,11 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         else
         {
-            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx\n",
-                   smfn, emfn - 1);
-            continue;
+            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
+                   smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
+            if ( !efi_map_uc )
+                continue;
+            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         }
 
         if ( desc->Attribute & EFI_MEMORY_WP )




[-- Attachment #2: EFI-default-map-attr.patch --]
[-- Type: text/plain, Size: 2819 bytes --]

EFI: support default attributes to map Runtime service areas with none given

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

For example on Dell machines we see:

(XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
(XEN) Unknown cachability for MFNs 0xfed18-0xfed19

Let's allow them to be mapped as UC.

We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -617,12 +617,24 @@ Either force retrieval of monitor EDID i
 disable it (edid=no). This option should not normally be required
 except for debugging purposes.
 
-### efi-rs
-> `= <boolean>`
+### efi
+> `= List of [ rs | attr ]`
+
+All options are of boolean kind and can be prefixed with `no-` to
+effect the inverse meaning.
+
+> `rs`
 
 > Default: `true`
 
-Force or disable use of EFI runtime services.
+>> Force or disable use of EFI runtime services.
+
+> `attr=uc`
+
+> Default: `off`
+
+>> Allows mapping of RuntimeServices which have no cachability attribute
+>> set as UC.
 
 ### extra\_guest\_irqs
 > `= [<domU number>][,<dom0 number>]`
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1100,7 +1100,31 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
 #ifndef CONFIG_ARM /* TODO - runtime service support */
 
 static bool_t __initdata efi_rs_enable = 1;
-boolean_param("efi-rs", efi_rs_enable);
+static bool_t __initdata efi_map_uc;
+
+static void __init parse_efi_param(char *s)
+{
+    char *ss;
+
+    do {
+        bool_t val = !!strncmp(s, "no-", 3);
+
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        if ( !strcmp(s, "rs") )
+            efi_rs_enable = val;
+        else if ( !strcmp(s, "attr=uc") )
+            efi_map_uc = val;
+
+        s = ss + 1;
+    } while ( ss );
+}
+custom_param("efi", parse_efi_param);
 
 #ifndef USE_SET_VIRTUAL_ADDRESS_MAP
 static __init void copy_mapping(unsigned long mfn, unsigned long end,
@@ -1198,9 +1222,11 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         else
         {
-            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx\n",
-                   smfn, emfn - 1);
-            continue;
+            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
+                   smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
+            if ( !efi_map_uc )
+                continue;
+            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         }
 
         if ( desc->Attribute & EFI_MEMORY_WP )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] EFI: support default attributes to map Runtime service areas with none given

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3155 bytes --]

On 09/06/15 14:53, Jan Beulich wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> For example on Dell machines we see:
>
> (XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
> (XEN) Unknown cachability for MFNs 0xfed18-0xfed19
>
> Let's allow them to be mapped as UC.
>
> We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -617,12 +617,24 @@ Either force retrieval of monitor EDID i
>  disable it (edid=no). This option should not normally be required
>  except for debugging purposes.
>  
> -### efi-rs
> -> `= <boolean>`
> +### efi
> +> `= List of [ rs | attr ]`
> +
> +All options are of boolean kind and can be prefixed with `no-` to
> +effect the inverse meaning.
> +
> +> `rs`
>  
>  > Default: `true`
>  
> -Force or disable use of EFI runtime services.
> +>> Force or disable use of EFI runtime services.
> +
> +> `attr=uc`
> +
> +> Default: `off`

`false` would be more consistent with the rest of the rest of the document.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +
> +>> Allows mapping of RuntimeServices which have no cachability attribute
> +>> set as UC.
>  
>  ### extra\_guest\_irqs
>  > `= [<domU number>][,<dom0 number>]`
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1100,7 +1100,31 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>  #ifndef CONFIG_ARM /* TODO - runtime service support */
>  
>  static bool_t __initdata efi_rs_enable = 1;
> -boolean_param("efi-rs", efi_rs_enable);
> +static bool_t __initdata efi_map_uc;
> +
> +static void __init parse_efi_param(char *s)
> +{
> +    char *ss;
> +
> +    do {
> +        bool_t val = !!strncmp(s, "no-", 3);
> +
> +        if ( !val )
> +            s += 3;
> +
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        if ( !strcmp(s, "rs") )
> +            efi_rs_enable = val;
> +        else if ( !strcmp(s, "attr=uc") )
> +            efi_map_uc = val;
> +
> +        s = ss + 1;
> +    } while ( ss );
> +}
> +custom_param("efi", parse_efi_param);
>  
>  #ifndef USE_SET_VIRTUAL_ADDRESS_MAP
>  static __init void copy_mapping(unsigned long mfn, unsigned long end,
> @@ -1198,9 +1222,11 @@ void __init efi_init_memory(void)
>              prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
>          else
>          {
> -            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx\n",
> -                   smfn, emfn - 1);
> -            continue;
> +            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
> +                   smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
> +            if ( !efi_map_uc )
> +                continue;
> +            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
>          }
>  
>          if ( desc->Attribute & EFI_MEMORY_WP )
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 4145 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] EFI: support default attributes to map Runtime service areas with none given

[Jan Beulich]

>>> On 09.06.15 at 16:08, <andrew.cooper3@citrix.com> wrote:
> On 09/06/15 14:53, Jan Beulich wrote:
>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> For example on Dell machines we see:
>>
>> (XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
>> (XEN) Unknown cachability for MFNs 0xfed18-0xfed19
>>
>> Let's allow them to be mapped as UC.
>>
>> We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -617,12 +617,24 @@ Either force retrieval of monitor EDID i
>>  disable it (edid=no). This option should not normally be required
>>  except for debugging purposes.
>>  
>> -### efi-rs
>> -> `= <boolean>`
>> +### efi
>> +> `= List of [ rs | attr ]`
>> +
>> +All options are of boolean kind and can be prefixed with `no-` to
>> +effect the inverse meaning.
>> +
>> +> `rs`
>>  
>>  > Default: `true`
>>  
>> -Force or disable use of EFI runtime services.
>> +>> Force or disable use of EFI runtime services.
>> +
>> +> `attr=uc`
>> +
>> +> Default: `off`
> 
> `false` would be more consistent with the rest of the rest of the document.

That's how Konrad had it originally, and I intentionally changed it,
as consistency with the rest of the doc seemed less important to
me than for this to make sense (and "off" to me makes more sense
than "false" here).

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Please clarify whether this stands even without the above changed.

Jan

Re: [PATCH 3/4] EFI: support default attributes to map Runtime service areas with none given

[Andrew Cooper]

On 09/06/15 16:26, Jan Beulich wrote:
>>>> On 09.06.15 at 16:08, <andrew.cooper3@citrix.com> wrote:
>> On 09/06/15 14:53, Jan Beulich wrote:
>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> For example on Dell machines we see:
>>>
>>> (XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
>>> (XEN) Unknown cachability for MFNs 0xfed18-0xfed19
>>>
>>> Let's allow them to be mapped as UC.
>>>
>>> We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.
>>>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -617,12 +617,24 @@ Either force retrieval of monitor EDID i
>>>  disable it (edid=no). This option should not normally be required
>>>  except for debugging purposes.
>>>  
>>> -### efi-rs
>>> -> `= <boolean>`
>>> +### efi
>>> +> `= List of [ rs | attr ]`
>>> +
>>> +All options are of boolean kind and can be prefixed with `no-` to
>>> +effect the inverse meaning.
>>> +
>>> +> `rs`
>>>  
>>>  > Default: `true`
>>>  
>>> -Force or disable use of EFI runtime services.
>>> +>> Force or disable use of EFI runtime services.
>>> +
>>> +> `attr=uc`
>>> +
>>> +> Default: `off`
>> `false` would be more consistent with the rest of the rest of the document.
> That's how Konrad had it originally, and I intentionally changed it,
> as consistency with the rest of the doc seemed less important to
> me than for this to make sense (and "off" to me makes more sense
> than "false" here).
>
>> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Please clarify whether this stands even without the above changed.

It stands.  I don't wish documentation quibbling to get in the way of
otherwise-good patches.

~Andrew

Re: [PATCH 3/4] EFI: support default attributes to map Runtime service areas with none given

[Ian Campbell]

On Tue, 2015-06-09 at 14:53 +0100, Jan Beulich wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> For example on Dell machines we see:
> 
> (XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
> (XEN) Unknown cachability for MFNs 0xfed18-0xfed19
> 
> Let's allow them to be mapped as UC.
> 
> We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For ARM: Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 4/4] x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

[Jan Beulich]

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

That flag now means cachability rather than protection, and a new flag
EFI_MEMORY_RO got added in its place.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -32,6 +32,8 @@
 /* Using SetVirtualAddressMap() is incompatible with kexec: */
 #undef USE_SET_VIRTUAL_ADDRESS_MAP
 
+#define EFI_REVISION(major, minor) (((major) << 16) | (minor))
+
 #define SMBIOS3_TABLE_GUID \
   { 0xf2fd1544, 0x9794, 0x4a2c, {0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94} }
 #define SHIM_LOCK_PROTOCOL_GUID \
@@ -78,6 +80,7 @@ static int set_color(u32 mask, int bpp, 
 static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
 
 static const EFI_BOOT_SERVICES *__initdata efi_bs;
+static UINT32 __initdata efi_bs_revision;
 static EFI_HANDLE __initdata efi_ih;
 
 static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
@@ -713,6 +716,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
 
     efi_ih = ImageHandle;
     efi_bs = SystemTable->BootServices;
+    efi_bs_revision = efi_bs->Hdr.Revision;
     efi_rs = SystemTable->RuntimeServices;
     efi_ct = SystemTable->ConfigurationTable;
     efi_num_ct = SystemTable->NumberOfTableEntries;
@@ -1220,6 +1224,9 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PAT | MAP_SMALL_PAGES;
         else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+        else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
+                  (desc->Attribute & EFI_MEMORY_WP) )
+            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
         else
         {
             printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
@@ -1229,7 +1236,8 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         }
 
-        if ( desc->Attribute & EFI_MEMORY_WP )
+        if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
+                                ? EFI_MEMORY_WP : EFI_MEMORY_RO) )
             prot &= ~_PAGE_RW;
         if ( desc->Attribute & EFI_MEMORY_XP )
             prot |= _PAGE_NX;
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -156,11 +156,15 @@ typedef enum {
 #define EFI_MEMORY_WT           0x0000000000000004
 #define EFI_MEMORY_WB           0x0000000000000008
 #define EFI_MEMORY_UCE          0x0000000000000010  
+#define EFI_MEMORY_WP           0x0000000000001000
 
 // physical memory protection on range 
-#define EFI_MEMORY_WP           0x0000000000001000
 #define EFI_MEMORY_RP           0x0000000000002000
 #define EFI_MEMORY_XP           0x0000000000004000
+#define EFI_MEMORY_RO           0x0000000000020000
+
+#define EFI_MEMORY_NV           0x0000000000008000
+#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000
 
 // range requires a runtime mapping
 #define EFI_MEMORY_RUNTIME      0x8000000000000000




[-- Attachment #2: EFI-memory-attr-v25.patch --]
[-- Type: text/plain, Size: 3034 bytes --]

x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

That flag now means cachability rather than protection, and a new flag
EFI_MEMORY_RO got added in its place.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -32,6 +32,8 @@
 /* Using SetVirtualAddressMap() is incompatible with kexec: */
 #undef USE_SET_VIRTUAL_ADDRESS_MAP
 
+#define EFI_REVISION(major, minor) (((major) << 16) | (minor))
+
 #define SMBIOS3_TABLE_GUID \
   { 0xf2fd1544, 0x9794, 0x4a2c, {0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94} }
 #define SHIM_LOCK_PROTOCOL_GUID \
@@ -78,6 +80,7 @@ static int set_color(u32 mask, int bpp, 
 static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
 
 static const EFI_BOOT_SERVICES *__initdata efi_bs;
+static UINT32 __initdata efi_bs_revision;
 static EFI_HANDLE __initdata efi_ih;
 
 static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
@@ -713,6 +716,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
 
     efi_ih = ImageHandle;
     efi_bs = SystemTable->BootServices;
+    efi_bs_revision = efi_bs->Hdr.Revision;
     efi_rs = SystemTable->RuntimeServices;
     efi_ct = SystemTable->ConfigurationTable;
     efi_num_ct = SystemTable->NumberOfTableEntries;
@@ -1220,6 +1224,9 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PAT | MAP_SMALL_PAGES;
         else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+        else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
+                  (desc->Attribute & EFI_MEMORY_WP) )
+            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
         else
         {
             printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
@@ -1229,7 +1236,8 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         }
 
-        if ( desc->Attribute & EFI_MEMORY_WP )
+        if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
+                                ? EFI_MEMORY_WP : EFI_MEMORY_RO) )
             prot &= ~_PAGE_RW;
         if ( desc->Attribute & EFI_MEMORY_XP )
             prot |= _PAGE_NX;
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -156,11 +156,15 @@ typedef enum {
 #define EFI_MEMORY_WT           0x0000000000000004
 #define EFI_MEMORY_WB           0x0000000000000008
 #define EFI_MEMORY_UCE          0x0000000000000010  
+#define EFI_MEMORY_WP           0x0000000000001000
 
 // physical memory protection on range 
-#define EFI_MEMORY_WP           0x0000000000001000
 #define EFI_MEMORY_RP           0x0000000000002000
 #define EFI_MEMORY_XP           0x0000000000004000
+#define EFI_MEMORY_RO           0x0000000000020000
+
+#define EFI_MEMORY_NV           0x0000000000008000
+#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000
 
 // range requires a runtime mapping
 #define EFI_MEMORY_RUNTIME      0x8000000000000000

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 4/4] x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

[Konrad Rzeszutek Wilk]

. ..snip..
> @@ -1220,6 +1224,9 @@ void __init efi_init_memory(void)
>              prot |= _PAGE_PAT | MAP_SMALL_PAGES;
>          else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
>              prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> +        else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
> +                  (desc->Attribute & EFI_MEMORY_WP) )
> +            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
>          else
>          {
>              printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
> @@ -1229,7 +1236,8 @@ void __init efi_init_memory(void)
>              prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
>          }
>  
> -        if ( desc->Attribute & EFI_MEMORY_WP )
> +        if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
> +                                ? EFI_MEMORY_WP : EFI_MEMORY_RO) )
>              prot &= ~_PAGE_RW;
>          if ( desc->Attribute & EFI_MEMORY_XP )
>              prot |= _PAGE_NX;
> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -156,11 +156,15 @@ typedef enum {
>  #define EFI_MEMORY_WT           0x0000000000000004
>  #define EFI_MEMORY_WB           0x0000000000000008
>  #define EFI_MEMORY_UCE          0x0000000000000010  
> +#define EFI_MEMORY_WP           0x0000000000001000
>  
>  // physical memory protection on range 
> -#define EFI_MEMORY_WP           0x0000000000001000

You delete EFI_MEMORY_WP it here, but you use it in 'efi_init_memory'?


>  #define EFI_MEMORY_RP           0x0000000000002000
>  #define EFI_MEMORY_XP           0x0000000000004000
> +#define EFI_MEMORY_RO           0x0000000000020000
> +
> +#define EFI_MEMORY_NV           0x0000000000008000
> +#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000

Those two are new (NV, MORE_RELIABLE), should they be part
of a different patch? Or if they are part of the new spec
just mention in the commit that you are adding it in for
future use?
>  
>  // range requires a runtime mapping
>  #define EFI_MEMORY_RUNTIME      0x8000000000000000

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 4/4] x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

[Konrad Rzeszutek Wilk]

On Tue, Jun 09, 2015 at 10:08:56AM -0400, Konrad Rzeszutek Wilk wrote:
> .. ..snip..
> > @@ -1220,6 +1224,9 @@ void __init efi_init_memory(void)
> >              prot |= _PAGE_PAT | MAP_SMALL_PAGES;
> >          else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
> >              prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> > +        else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
> > +                  (desc->Attribute & EFI_MEMORY_WP) )
> > +            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
> >          else
> >          {
> >              printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
> > @@ -1229,7 +1236,8 @@ void __init efi_init_memory(void)
> >              prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> >          }
> >  
> > -        if ( desc->Attribute & EFI_MEMORY_WP )
> > +        if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
> > +                                ? EFI_MEMORY_WP : EFI_MEMORY_RO) )
> >              prot &= ~_PAGE_RW;
> >          if ( desc->Attribute & EFI_MEMORY_XP )
> >              prot |= _PAGE_NX;
> > --- a/xen/include/efi/efidef.h
> > +++ b/xen/include/efi/efidef.h
> > @@ -156,11 +156,15 @@ typedef enum {
> >  #define EFI_MEMORY_WT           0x0000000000000004
> >  #define EFI_MEMORY_WB           0x0000000000000008
> >  #define EFI_MEMORY_UCE          0x0000000000000010  
> > +#define EFI_MEMORY_WP           0x0000000000001000
> >  
> >  // physical memory protection on range 
> > -#define EFI_MEMORY_WP           0x0000000000001000
> 
> You delete EFI_MEMORY_WP it here, but you use it in 'efi_init_memory'?

<blushes> Please ignore that - I overlooked the movement
of the define up.

Re: [PATCH 4/4] x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

[Jan Beulich]

>>> On 09.06.15 at 16:08, <konrad.wilk@oracle.com> wrote:
>> @@ -1220,6 +1224,9 @@ void __init efi_init_memory(void)
>>  #define EFI_MEMORY_RP           0x0000000000002000
>>  #define EFI_MEMORY_XP           0x0000000000004000
>> +#define EFI_MEMORY_RO           0x0000000000020000
>> +
>> +#define EFI_MEMORY_NV           0x0000000000008000
>> +#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000
> 
> Those two are new (NV, MORE_RELIABLE), should they be part
> of a different patch? Or if they are part of the new spec
> just mention in the commit that you are adding it in for
> future use?

Added

"Along with EFI_MEMORY_RO also add the two other new EFI_MEMORY_*
 definitions, even if we don't need them right away."

Jan

Re: [PATCH 4/4] x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

[Konrad Rzeszutek Wilk]

On Tue, Jun 09, 2015 at 04:28:50PM +0100, Jan Beulich wrote:
> >>> On 09.06.15 at 16:08, <konrad.wilk@oracle.com> wrote:
> >> @@ -1220,6 +1224,9 @@ void __init efi_init_memory(void)
> >>  #define EFI_MEMORY_RP           0x0000000000002000
> >>  #define EFI_MEMORY_XP           0x0000000000004000
> >> +#define EFI_MEMORY_RO           0x0000000000020000
> >> +
> >> +#define EFI_MEMORY_NV           0x0000000000008000
> >> +#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000
> > 
> > Those two are new (NV, MORE_RELIABLE), should they be part
> > of a different patch? Or if they are part of the new spec
> > just mention in the commit that you are adding it in for
> > future use?
> 
> Added
> 
> "Along with EFI_MEMORY_RO also add the two other new EFI_MEMORY_*
>  definitions, even if we don't need them right away."

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Jan
> 

Re: [PATCH 4/4] x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5

[Andrew Cooper]

On 09/06/15 14:54, Jan Beulich wrote:
> That flag now means cachability rather than protection, and a new flag
> EFI_MEMORY_RO got added in its place.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This matches my reading of the 2.5 spec.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 0/4] misc EFI adjustments

[Konrad Rzeszutek Wilk]

On Tue, Jun 09, 2015 at 02:49:45PM +0100, Jan Beulich wrote:
> 1: x86/EFI: fix EFI_MEMORY_WP handling
> 2: EFI/early: add /mapbs to map EfiBootServices{Code,Data}
> 3: EFI: support default attributes to map Runtime service areas with none given
> 4: x86/EFI: adjust EFI_MEMORY_WP handling for spec version 2.5 
> 
> Patches 2 and 3 are based on earlier work by Konrad.

Thank you for retouching them!
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>