[Xen-devel] [PATCH v3 0/2] Optionally call EFI SetVirtualAddressMap()

[Xen-devel] [PATCH v3 0/2] Optionally call EFI SetVirtualAddressMap()

[Marek Marczykowski-Górecki]

Workaround buggy UEFI accessing boot services memory after ExitBootServices().
Patches discussed here:
https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00701.html

In addition to the tests below, I've tested kexec on xen.efi with this option
enabled and it (still) works.

Test results on few laptops:

Thinkpad x230, firmware version 2.77:
 - without the patch: crashes on RS call (mapbs helps)
 - with patch: works
 - same with xen.efi and MB2

Librem 14 v1, firmware version (AMI) ARUD026 (06/18/2015):
 - without the patch: works
 - with the patch: works
 - same with xen.efi and MB2

Dell Latitude E6420, firmware version A21:
 this machine requires efi=attr=uc workaround
 - without the patch: dom0 hangs before sending any message to the console (even with earlyprintk=xen etc)
 - with the patch: crashes before dom0 prints anything: mm.c:896:d0v0 non-privileged attempt to map MMIO space 2c2c2c2c2c
 - same with xen.efi and MB2

Thinkpad W540:
 - without the patch: crashes on RS call (only efi=no-rs helps)
 - with patch: works
 - tested only with MB2

Thinkpad X1 Carbon gen5, firmware version 1.22 (2017-07-04):
 - without the patch: works
 - with patch: works
 - tested only xen.efi

Thinkpad P52, firmware version 1.25 (2018-04-15):
 - without the patch (MB2): hangs on RS call (mapbs helps)
 - without the patch (xen.efi): works(?!)
 - with the patch: works
 - tested with xen.efi and MB2

Changes in v2:
 - fix boot with xen.efi (efi_memmap at this point still needs to be accessed
   via physical address). TBH, I don't understand why previous version worked
   with MB2 - is directmap mapped at this point?

Cc: Juergen Gross <jgross@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
Cc: Jason Andryuk <jandryuk@gmail.com>

Marek Marczykowski-Górecki (2):
  efi: remove old SetVirtualAddressMap() arrangement
  xen/efi: optionally call SetVirtualAddressMap()

 xen/common/Kconfig    | 13 ++++++++++++-
 xen/common/efi/boot.c | 48 +++++++++++++++++++++++---------------------
 2 files changed, 39 insertions(+), 22 deletions(-)

base-commit: 7a4e6711114905b3cbbe48e81c3222361a7f3579
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/2] efi: remove old SetVirtualAddressMap() arrangement

[Marek Marczykowski-Górecki]

Remove unused (#ifdef-ed out) code. Reviving it in its current shape
won't fly because:
 - SetVirtualAddressMap() needs to be called with 1:1 mapping, which
   isn't the case at this time
 - it uses directmap, which may go away soon
 - it uses directmap, which is mapped with NX, breaking EfiRuntimeServicesCode

No functional change.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/common/efi/boot.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 7919378..cddf3de 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -29,9 +29,6 @@
 #undef __ASSEMBLY__
 #endif
 
-/* Using SetVirtualAddressMap() is incompatible with kexec: */
-#undef USE_SET_VIRTUAL_ADDRESS_MAP
-
 #define EFI_REVISION(major, minor) (((major) << 16) | (minor))
 
 #define SMBIOS3_TABLE_GUID \
@@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
 
     /* Adjust pointers into EFI. */
     efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
-    efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
-#endif
     efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
     efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
 }
@@ -1422,7 +1416,6 @@ static int __init parse_efi_param(const char *s)
 }
 custom_param("efi", parse_efi_param);
 
-#ifndef USE_SET_VIRTUAL_ADDRESS_MAP
 static __init void copy_mapping(unsigned long mfn, unsigned long end,
                                 bool (*is_valid)(unsigned long smfn,
                                                  unsigned long emfn))
@@ -1466,7 +1459,6 @@ static bool __init rt_range_valid(unsigned long smfn, unsigned long emfn)
 {
     return true;
 }
-#endif
 
 #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
@@ -1474,13 +1466,11 @@ static bool __init rt_range_valid(unsigned long smfn, unsigned long emfn)
 void __init efi_init_memory(void)
 {
     unsigned int i;
-#ifndef USE_SET_VIRTUAL_ADDRESS_MAP
     struct rt_extra {
         struct rt_extra *next;
         unsigned long smfn, emfn;
         unsigned int prot;
     } *extra, *extra_head = NULL;
-#endif
 
     free_ebmalloc_unused_mem();
 
@@ -1563,7 +1553,6 @@ void __init efi_init_memory(void)
                 printk(XENLOG_ERR "Could not map MFNs %#lx-%#lx\n",
                        smfn, emfn - 1);
         }
-#ifndef USE_SET_VIRTUAL_ADDRESS_MAP
         else if ( !((desc->PhysicalStart + len - 1) >> (VADDR_BITS - 1)) &&
                   (extra = xmalloc(struct rt_extra)) != NULL )
         {
@@ -1574,12 +1563,8 @@ void __init efi_init_memory(void)
             extra_head = extra;
             desc->VirtualStart = desc->PhysicalStart;
         }
-#endif
         else
         {
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
-            /* XXX allocate e.g. down from FIXADDR_START */
-#endif
             printk(XENLOG_ERR "No mapping for MFNs %#lx-%#lx\n",
                    smfn, emfn - 1);
         }
@@ -1591,10 +1576,6 @@ void __init efi_init_memory(void)
         return;
     }
 
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
-    efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
-                                 mdesc_ver, efi_memmap);
-#else
     /* Set up 1:1 page tables to do runtime calls in "physical" mode. */
     efi_l4_pgtable = alloc_xen_pagetable();
     BUG_ON(!efi_l4_pgtable);
@@ -1680,6 +1661,5 @@ void __init efi_init_memory(void)
     for ( i = l4_table_offset(HYPERVISOR_VIRT_START);
           i < l4_table_offset(DIRECTMAP_VIRT_END); ++i )
         efi_l4_pgtable[i] = idle_pg_table[i];
-#endif
 }
 #endif
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Marek Marczykowski-Górecki]

Some UEFI implementations are not happy about running in 1:1 addressing,
but really virtual address space. Specifically, some access
EfiBootServices{Code,Data}, or even totally unmapped areas. Example
crash of GetVariable() call on Thinkpad W540:

    Xen call trace:
       [<0000000000000080>] 0000000000000080
       [<8c2b0398e0000daa>] 8c2b0398e0000daa

    Pagetable walk from ffffffff858483a1:
       L4[0x1ff] = 0000000000000000 ffffffffffffffff

    ****************************************
    Panic on CPU 0:
    FATAL PAGE FAULT
    [error_code=0002]
    Faulting linear address: ffffffff858483a1
    ****************************************

Fix this by calling SetVirtualAddressMap() runtime service, giving it
1:1 map for areas marked as needed during runtime. The address space in
which EFI runtime services are called is unchanged, but UEFI view of it
may be.
SetVirtualAddressMap() can be called only once, so it might make
future kexec EFI plumbing more complex or incompatible with this option.
Since it's fairly late in Xen 4.13 development cycle, disable it
by default and hide behind EXPERT.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
 - call  SetVirtualAddressMap() before adjusting efi pointers; especially
   efi_memmap at this point still needs to use physical address, not a
   directmap one
Changes in v3:
 - clarify impact (or rather: lack of it) on kexec, drop !KEXEC
   dependency.
---
 xen/common/Kconfig    | 13 +++++++++++++
 xen/common/efi/boot.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 16829f6..4ad4534 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -88,6 +88,19 @@ config KEXEC
 
 	  If unsure, say Y.
 
+config SET_VIRTUAL_ADDRESS_MAP
+    bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y"
+    default n
+    ---help---
+      Call EFI SetVirtualAddressMap() runtime service to setup memory map for
+      further runtime services. According to UEFI spec, it isn't strictly
+      necessary, but many UEFI implementations misbehave when this call is
+      missing. On the other hand, this call can be made only once, which might
+      be incompatible with future EFI support in Xen's kexec. Kexec ability
+      in the current form is not affected.
+
+      If unsure, say N.
+
 config XENOPROF
 	def_bool y
 	prompt "Xen Oprofile Support" if EXPERT = "y"
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index cddf3de..6eaabd4 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1056,11 +1056,17 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
         efi_arch_video_init(gop, info_size, mode_info);
 }
 
+#define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
+                                 (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
+
 static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     EFI_STATUS status;
     UINTN info_size = 0, map_key;
     bool retry;
+#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP
+    unsigned int i;
+#endif
 
     efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
                          &efi_mdesc_size, &mdesc_ver);
@@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Cannot exit boot services", status);
 
+#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP
+    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
+    {
+        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
+
+        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+            desc->VirtualStart = desc->PhysicalStart;
+        else
+            desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
+    }
+    status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
+                                          mdesc_ver, efi_memmap);
+    if ( status != EFI_SUCCESS )
+    {
+        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n",
+               status);
+        __clear_bit(EFI_RS, &efi_flags);
+    }
+#endif
+
     /* Adjust pointers into EFI. */
     efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
     efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
@@ -1460,8 +1486,6 @@ static bool __init rt_range_valid(unsigned long smfn, unsigned long emfn)
     return true;
 }
 
-#define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
-                                 (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
 
 void __init efi_init_memory(void)
 {
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 0/2] Optionally call EFI SetVirtualAddressMap()

[Jason Andryuk]

On Sat, Oct 12, 2019 at 6:11 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> Workaround buggy UEFI accessing boot services memory after ExitBootServices().
> Patches discussed here:
> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00701.html
>
> In addition to the tests below, I've tested kexec on xen.efi with this option
> enabled and it (still) works.
>
> Test results on few laptops:
>
> Thinkpad x230, firmware version 2.77:
>  - without the patch: crashes on RS call (mapbs helps)
>  - with patch: works
>  - same with xen.efi and MB2
>
> Librem 14 v1, firmware version (AMI) ARUD026 (06/18/2015):
>  - without the patch: works
>  - with the patch: works
>  - same with xen.efi and MB2
>
> Dell Latitude E6420, firmware version A21:
>  this machine requires efi=attr=uc workaround
>  - without the patch: dom0 hangs before sending any message to the console (even with earlyprintk=xen etc)
>  - with the patch: crashes before dom0 prints anything: mm.c:896:d0v0 non-privileged attempt to map MMIO space 2c2c2c2c2c
>  - same with xen.efi and MB2
>
> Thinkpad W540:
>  - without the patch: crashes on RS call (only efi=no-rs helps)
>  - with patch: works
>  - tested only with MB2
>
> Thinkpad X1 Carbon gen5, firmware version 1.22 (2017-07-04):
>  - without the patch: works
>  - with patch: works
>  - tested only xen.efi
>
> Thinkpad P52, firmware version 1.25 (2018-04-15):
>  - without the patch (MB2): hangs on RS call (mapbs helps)
>  - without the patch (xen.efi): works(?!)
>  - with the patch: works
>  - tested with xen.efi and MB2
>
> Changes in v2:
>  - fix boot with xen.efi (efi_memmap at this point still needs to be accessed
>    via physical address). TBH, I don't understand why previous version worked
>    with MB2 - is directmap mapped at this point?

v1 failed to boot for me.

For v3:
Dell Latitude 5580, firmware 1.16.0
 - without the patch: works
 - with patch: works
 - tested only xen.efi

Tested-by: Jason Andryuk <jandryuk@gmail.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Andrew Cooper]

On 12/10/2019 23:11, Marek Marczykowski-Górecki wrote:
> Some UEFI implementations are not happy about running in 1:1 addressing,
> but really virtual address space. Specifically, some access
> EfiBootServices{Code,Data}, or even totally unmapped areas. Example
> crash of GetVariable() call on Thinkpad W540:
>
>     Xen call trace:
>        [<0000000000000080>] 0000000000000080
>        [<8c2b0398e0000daa>] 8c2b0398e0000daa
>
>     Pagetable walk from ffffffff858483a1:
>        L4[0x1ff] = 0000000000000000 ffffffffffffffff
>
>     ****************************************
>     Panic on CPU 0:
>     FATAL PAGE FAULT
>     [error_code=0002]
>     Faulting linear address: ffffffff858483a1
>     ****************************************
>
> Fix this by calling SetVirtualAddressMap() runtime service, giving it
> 1:1 map for areas marked as needed during runtime. The address space in
> which EFI runtime services are called is unchanged, but UEFI view of it
> may be.
> SetVirtualAddressMap() can be called only once, so it might make
> future kexec EFI plumbing more complex or incompatible with this option.

There is no such thing as "future incompatibilities".  There is "one
more piece of plumbing someone in the future needs to do with passing
EFI details".

Given how the rest of this looks, I'd just drop all references to
Kexec.  I think what is here is misleading at best.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] efi: remove old SetVirtualAddressMap() arrangement

[Jan Beulich]

On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> @@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>  
>      /* Adjust pointers into EFI. */
>      efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
> -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
> -    efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
> -#endif

This doesn't get re-instated in any way by patch 2. How come you
get away without? In any event this is perhaps the best example
of why I personally think it would have been better to change
things in place, rather than remove everything first. But for
some of the other change the question also arises of why they
don't need re-instating in one form or another.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] efi: remove old SetVirtualAddressMap() arrangement

[Jan Beulich]

On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> @@ -1591,10 +1576,6 @@ void __init efi_init_memory(void)
>          return;
>      }
>  
> -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
> -    efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
> -                                 mdesc_ver, efi_memmap);
> -#else
>      /* Set up 1:1 page tables to do runtime calls in "physical" mode. */

This comment, btw, also wants either adjusting or removing.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] efi: remove old SetVirtualAddressMap() arrangement

[Marek Marczykowski-Górecki]

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

On Wed, Oct 23, 2019 at 05:15:42PM +0200, Jan Beulich wrote:
> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> > @@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> >  
> >      /* Adjust pointers into EFI. */
> >      efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
> > -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
> > -    efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
> > -#endif
> 
> This doesn't get re-instated in any way by patch 2. 

This commit remove dead code.

> How come you
> get away without? 

The second patch doesn't just fix what was under #ifdef
USE_SET_VIRTUAL_ADDRESS_MAP. It does a completely different approach to
using SetVirtualAddressMap. See below.

On Wed, Oct 23, 2019 at 05:26:48PM +0200, Jan Beulich wrote:
> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> > @@ -1591,10 +1576,6 @@ void __init efi_init_memory(void)
> >          return;
> >      }
> >  
> > -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
> > -    efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
> > -                                 mdesc_ver, efi_memmap);
> > -#else
> >      /* Set up 1:1 page tables to do runtime calls in "physical" mode. */
> 
> This comment, btw, also wants either adjusting or removing.

No, it still setup 1:1 page tables for the runtime calls, exactly as it
was before. This is also why I don't need to adjust efi_rs. The only
difference is now (with patch 2) we tell UEFI about it. But the actual
address space layout for the runtime calls is exactly as it was before.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Jan Beulich]

On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> Some UEFI implementations are not happy about running in 1:1 addressing,
> but really virtual address space.

I have to admit that I find this misleading. There's no true "physical
mode" on x86-64 anyway. What I assume happens is that people abuse the
address map change notification to do things beyond the necessary
ConvertPointer(() calls.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -88,6 +88,19 @@ config KEXEC
>  
>  	  If unsure, say Y.
>  
> +config SET_VIRTUAL_ADDRESS_MAP

I'm of the strong opinion that this wants to have an EFI_ prefix.

> +    bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y"
> +    default n

I don't think you need this line.

> @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>      if ( EFI_ERROR(status) )
>          PrintErrMesg(L"Cannot exit boot services", status);
>  
> +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP
> +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> +    {
> +        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> +
> +        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +            desc->VirtualStart = desc->PhysicalStart;
> +        else
> +            desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> +    }
> +    status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
> +                                          mdesc_ver, efi_memmap);
> +    if ( status != EFI_SUCCESS )
> +    {
> +        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n",
> +               status);
> +        __clear_bit(EFI_RS, &efi_flags);
> +    }
> +#endif

This new placement undermines (or at least complicates afaict) the
original intention to allow picking virtual addresses which don't
match the directmap. I can accept this as an intended tradeoff (as
you validly mention in the other patch we don't honor the 1:1 map
requirement at the time of the call with its original placement),
but it should be mentioned in one of the two patch descriptions.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Marek Marczykowski-Górecki]

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

On Wed, Oct 23, 2019 at 05:37:33PM +0200, Jan Beulich wrote:
> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> > Some UEFI implementations are not happy about running in 1:1 addressing,
> > but really virtual address space.
> 
> I have to admit that I find this misleading. There's no true "physical
> mode" on x86-64 anyway. What I assume happens is that people abuse the
> address map change notification to do things beyond the necessary
> ConvertPointer(() calls.

That would indeed match the behaviour. I'll add it to commit message.

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -88,6 +88,19 @@ config KEXEC
> >  
> >  	  If unsure, say Y.
> >  
> > +config SET_VIRTUAL_ADDRESS_MAP
> 
> I'm of the strong opinion that this wants to have an EFI_ prefix.

Ok.

> > +    bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y"
> > +    default n
> 
> I don't think you need this line.
> 
> > @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> >      if ( EFI_ERROR(status) )
> >          PrintErrMesg(L"Cannot exit boot services", status);
> >  
> > +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP
> > +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> > +    {
> > +        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> > +
> > +        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> > +            desc->VirtualStart = desc->PhysicalStart;
> > +        else
> > +            desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> > +    }
> > +    status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
> > +                                          mdesc_ver, efi_memmap);
> > +    if ( status != EFI_SUCCESS )
> > +    {
> > +        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n",
> > +               status);
> > +        __clear_bit(EFI_RS, &efi_flags);
> > +    }
> > +#endif
> 
> This new placement undermines (or at least complicates afaict) the
> original intention to allow picking virtual addresses which don't
> match the directmap.

If I read it right, the original intention was to specifically use
directmap, not some other virtual addresses. Which is flawed, because
directmap is mapped with NX, so at least EfiRuntimeServicesCode will
break. This means, even when using directmap, Xen would need to switch
page tables for the runtime call time to allow executing that code.

There is of course an option to rewrite it completely differently,
mapping EFI runtime regions somewhere else (not 1:1 and not re-use
directmap). But I don't think it worth the effort, and also is definitely
too complex this far in 4.13 release cycle.

> I can accept this as an intended tradeoff (as
> you validly mention in the other patch we don't honor the 1:1 map
> requirement at the time of the call with its original placement),
> but it should be mentioned in one of the two patch descriptions.

This is one of the reason why I've decided to split this change into two
parts - remove the old one and add the new one. It is really not "fixing
the old approach", but doing this very differently. I think this patch
description referencing old, never working and never even enabled
approach would be misleading at least. The patch removing the old
approach do list reasons why it was broken.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] efi: remove old SetVirtualAddressMap() arrangement

[Jan Beulich]

On 23.10.2019 17:36, Marek Marczykowski-Górecki wrote:
> On Wed, Oct 23, 2019 at 05:15:42PM +0200, Jan Beulich wrote:
>> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
>>> @@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>>>  
>>>      /* Adjust pointers into EFI. */
>>>      efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
>>> -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
>>> -    efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
>>> -#endif
>>
>> This doesn't get re-instated in any way by patch 2. 
> 
> This commit remove dead code.
> 
>> How come you
>> get away without? 
> 
> The second patch doesn't just fix what was under #ifdef
> USE_SET_VIRTUAL_ADDRESS_MAP. It does a completely different approach to
> using SetVirtualAddressMap. See below.
> 
> On Wed, Oct 23, 2019 at 05:26:48PM +0200, Jan Beulich wrote:
>> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
>>> @@ -1591,10 +1576,6 @@ void __init efi_init_memory(void)
>>>          return;
>>>      }
>>>  
>>> -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
>>> -    efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
>>> -                                 mdesc_ver, efi_memmap);
>>> -#else
>>>      /* Set up 1:1 page tables to do runtime calls in "physical" mode. */
>>
>> This comment, btw, also wants either adjusting or removing.
> 
> No, it still setup 1:1 page tables for the runtime calls, exactly as it
> was before.

But the "physical" is no longer correct.

> This is also why I don't need to adjust efi_rs.

Well, you may not _need_ to with the current code structure, but I
wonder if you better would. In fact I wonder whether the #ifdef
around the line further up shouldn't have been removed already
(and hence that's what you want to do): Take the processing of
XEN_EFI_query_variable_info - it could do the version check
outside of the efi_rs_{enter,exit}() region if efi_rs was properly
relocated. Right now it's a requirement to make all accesses to
efi_rs within such regions.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Jan Beulich]

On 23.10.2019 18:07, Marek Marczykowski-Górecki wrote:
> On Wed, Oct 23, 2019 at 05:37:33PM +0200, Jan Beulich wrote:
>> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
>>> @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>>>      if ( EFI_ERROR(status) )
>>>          PrintErrMesg(L"Cannot exit boot services", status);
>>>  
>>> +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP
>>> +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>>> +    {
>>> +        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>>> +
>>> +        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
>>> +            desc->VirtualStart = desc->PhysicalStart;
>>> +        else
>>> +            desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
>>> +    }
>>> +    status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
>>> +                                          mdesc_ver, efi_memmap);
>>> +    if ( status != EFI_SUCCESS )
>>> +    {
>>> +        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n",
>>> +               status);
>>> +        __clear_bit(EFI_RS, &efi_flags);
>>> +    }
>>> +#endif
>>
>> This new placement undermines (or at least complicates afaict) the
>> original intention to allow picking virtual addresses which don't
>> match the directmap.
> 
> If I read it right, the original intention was to specifically use
> directmap, not some other virtual addresses. Which is flawed, because
> directmap is mapped with NX, so at least EfiRuntimeServicesCode will
> break. This means, even when using directmap, Xen would need to switch
> page tables for the runtime call time to allow executing that code.

Just FYI: The NX-ifying post-dates the EFI work by several years.

> There is of course an option to rewrite it completely differently,
> mapping EFI runtime regions somewhere else (not 1:1 and not re-use
> directmap). But I don't think it worth the effort, and also is definitely
> too complex this far in 4.13 release cycle.

Especially on this last point - fully agree.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] efi: remove old SetVirtualAddressMap() arrangement

[Marek Marczykowski-Górecki]

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

On Wed, Oct 23, 2019 at 06:10:55PM +0200, Jan Beulich wrote:
> On 23.10.2019 17:36, Marek Marczykowski-Górecki wrote:
> > No, it still setup 1:1 page tables for the runtime calls, exactly as it
> > was before.
> 
> But the "physical" is no longer correct.

Ok, I'll add it to the other patch (as it is where UEFI is informed it
isn't "physical").

> > This is also why I don't need to adjust efi_rs.
> 
> Well, you may not _need_ to with the current code structure, but I
> wonder if you better would. In fact I wonder whether the #ifdef
> around the line further up shouldn't have been removed already
> (and hence that's what you want to do): Take the processing of
> XEN_EFI_query_variable_info - it could do the version check
> outside of the efi_rs_{enter,exit}() region if efi_rs was properly
> relocated. Right now it's a requirement to make all accesses to
> efi_rs within such regions.

Right, this could be a further improvement. But given the conditional
nature of this patch series for 4.13, I'm not sure if it should be done
here. I can post it as a separate patch and let you/Juergen decide
whether commit it to 4.13 or next.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel