[Xen-devel] [PATCH v4 0/3] Optionally call EFI SetVirtualAddressMap()

[Xen-devel] [PATCH v4 0/3] 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

Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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>

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?
Changes in v4:
 - reword commit messages, drop mentions of kexec
 - new patch (3)

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 (3):
  efi: remove old SetVirtualAddressMap() arrangement
  xen/efi: optionally call SetVirtualAddressMap()
  xen/efi: use directmap to access runtime services table

 xen/common/Kconfig       | 10 ++++++++-
 xen/common/efi/boot.c    | 52 +++++++++++++++++++++++------------------
 xen/common/efi/runtime.c | 19 +++------------
 3 files changed, 44 insertions(+), 37 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 v4 1/3] 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 v4 2/3] xen/efi: optionally call SetVirtualAddressMap()

[Marek Marczykowski-Górecki]

Some UEFI implementations are not happy about lack of
SetVirtualAddressMap() call. Likely abuse the address map change
notification to do things beyond the necessary ConvertPointer() calls.
Specifically, wihtout the SetVirtualAddressMap() call, 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.
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.
Changes in v4:
 - update commit message
 - adjust comment
 - rename config option to add EFI_ prefix
---
 xen/common/Kconfig    | 10 ++++++++++
 xen/common/efi/boot.c | 33 ++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 16829f6..549a7d5 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -88,6 +88,16 @@ config KEXEC
 
 	  If unsure, say Y.
 
+config EFI_SET_VIRTUAL_ADDRESS_MAP
+    bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y"
+    ---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.
+
+      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..9debc5b 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_EFI_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_EFI_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)
 {
@@ -1576,7 +1600,10 @@ void __init efi_init_memory(void)
         return;
     }
 
-    /* Set up 1:1 page tables to do runtime calls in "physical" mode. */
+    /*
+     * Set up 1:1 page tables for runtime calls. See SetVirtualAddressMap() in
+     * efi_exit_boot().
+     */
     efi_l4_pgtable = alloc_xen_pagetable();
     BUG_ON(!efi_l4_pgtable);
     clear_page(efi_l4_pgtable);
-- 
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 v4 3/3] xen/efi: use directmap to access runtime services table

[Marek Marczykowski-Górecki]

Do not require switching page tables to access (static) information in
the runtime services table itself, use directmap for this. This allows
exiting early from XEN_EFI_query_capsule_capabilities,
XEN_EFI_update_capsule and XEN_EFI_query_variable_info (in case of not
supported call) without all the impact of page table switch.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
New patch in v4. Can be applied independently of the other two.
Specifically can be defered beyond 4.13.
I'm also fine with dropping it, if adding directmap users is undesired.

Cc: Juergen Gross <jgross@suse.com>
---
 xen/common/efi/boot.c    |  1 +
 xen/common/efi/runtime.c | 19 ++++---------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 9debc5b..89b1c8a 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1122,6 +1122,7 @@ 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;
+    efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
     efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
     efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
 }
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index ab53ebc..22fd6c9 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -211,12 +211,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
         break;
     case XEN_FW_EFI_RT_VERSION:
     {
-        struct efi_rs_state state = efi_rs_enter();
-
-        if ( !state.cr3 )
-            return -EOPNOTSUPP;
         info->version = efi_rs->Hdr.Revision;
-        efi_rs_leave(&state);
         break;
     }
     case XEN_FW_EFI_CONFIG_TABLE:
@@ -618,12 +613,11 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
             break;
         }
 
+        if ( (efi_rs->Hdr.Revision >> 16) < 2 )
+            return -EOPNOTSUPP;
         state = efi_rs_enter();
-        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
-        {
-            efi_rs_leave(&state);
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
-        }
         status = efi_rs->QueryVariableInfo(
             op->u.query_variable_info.attr,
             &op->u.query_variable_info.max_store_size,
@@ -637,13 +631,8 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
         if ( op->misc )
             return -EINVAL;
 
-        state = efi_rs_enter();
-        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
-        {
-            efi_rs_leave(&state);
+        if ( (efi_rs->Hdr.Revision >> 16) < 2 )
             return -EOPNOTSUPP;
-        }
-        efi_rs_leave(&state);
         /* XXX fall through for now */
     default:
         return -ENOSYS;
-- 
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 v4 3/3] xen/efi: use directmap to access runtime services table

[Xia, Hongyan]

It is certainly nice to have less users of the direct map. My non-EFI
builds already work without the direct map now but once I start testing
EFI, it is nice to have one less thing to worry about.

How important and performance-critical is this? If we really want to
avoid switching the page table, we could reserve a virtual range and
map it to runtime services in Xen.

Hongyan

On Thu, 2019-10-24 at 05:45 +0200, Marek Marczykowski-Górecki wrote:
> Do not require switching page tables to access (static) information
> in
> the runtime services table itself, use directmap for this. This
> allows
> exiting early from XEN_EFI_query_capsule_capabilities,
> XEN_EFI_update_capsule and XEN_EFI_query_variable_info (in case of
> not
> supported call) without all the impact of page table switch.
> 
> Signed-off-by: Marek Marczykowski-Górecki <
> marmarek@invisiblethingslab.com>
> ---
> New patch in v4. Can be applied independently of the other two.
> Specifically can be defered beyond 4.13.
> I'm also fine with dropping it, if adding directmap users is
> undesired.
> 
> Cc: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/efi/boot.c    |  1 +
>  xen/common/efi/runtime.c | 19 ++++---------------
>  2 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 9debc5b..89b1c8a 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1122,6 +1122,7 @@ 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;
> +    efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
>      efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
>      efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
>  }
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index ab53ebc..22fd6c9 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -211,12 +211,7 @@ int efi_get_info(uint32_t idx, union
> xenpf_efi_info *info)
>          break;
>      case XEN_FW_EFI_RT_VERSION:
>      {
> -        struct efi_rs_state state = efi_rs_enter();
> -
> -        if ( !state.cr3 )
> -            return -EOPNOTSUPP;
>          info->version = efi_rs->Hdr.Revision;
> -        efi_rs_leave(&state);
>          break;
>      }
>      case XEN_FW_EFI_CONFIG_TABLE:
> @@ -618,12 +613,11 @@ int efi_runtime_call(struct
> xenpf_efi_runtime_call *op)
>              break;
>          }
>  
> +        if ( (efi_rs->Hdr.Revision >> 16) < 2 )
> +            return -EOPNOTSUPP;
>          state = efi_rs_enter();
> -        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
> -        {
> -            efi_rs_leave(&state);
> +        if ( !state.cr3 )
>              return -EOPNOTSUPP;
> -        }
>          status = efi_rs->QueryVariableInfo(
>              op->u.query_variable_info.attr,
>              &op->u.query_variable_info.max_store_size,
> @@ -637,13 +631,8 @@ int efi_runtime_call(struct
> xenpf_efi_runtime_call *op)
>          if ( op->misc )
>              return -EINVAL;
>  
> -        state = efi_rs_enter();
> -        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
> -        {
> -            efi_rs_leave(&state);
> +        if ( (efi_rs->Hdr.Revision >> 16) < 2 )
>              return -EOPNOTSUPP;
> -        }
> -        efi_rs_leave(&state);
>          /* XXX fall through for now */
>      default:
>          return -ENOSYS;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 3/3] xen/efi: use directmap to access runtime services table

[marmarek]

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

On Thu, Oct 24, 2019 at 01:11:22PM +0000, Xia, Hongyan wrote:
> It is certainly nice to have less users of the direct map. My non-EFI
> builds already work without the direct map now but once I start testing
> EFI, it is nice to have one less thing to worry about.

Note this is just yet another EFI info that's included there. Others
are: efi_ct, efi_memmap, efi_fw_vendor. So, if you'd like to get rid of
directmap, you'll need to handle the others too in some way. Doing that
for 3 or 4 tables shouldn't make significant difference.

> How important and performance-critical is this? If we really want to
> avoid switching the page table, we could reserve a virtual range and
> map it to runtime services in Xen.

Honestly I don't think that's very critical. The biggest improvement is
for XEN_FW_EFI_RT_VERSION, where you avoid switching page tables at all.
In other cases, you avoid that for too old UEFIs only. Anyway, I think
none of it is on a hot path.
This is an optimization suggested by Jan, which is nice to have, but
definitely isn't the only possible option.

-- 
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 v4 3/3] xen/efi: use directmap to access runtime services table

[Jan Beulich]

On 24.10.2019 05:45, Marek Marczykowski-Górecki wrote:
> Do not require switching page tables to access (static) information in
> the runtime services table itself, use directmap for this. This allows
> exiting early from XEN_EFI_query_capsule_capabilities,
> XEN_EFI_update_capsule and XEN_EFI_query_variable_info (in case of not
> supported call) without all the impact of page table switch.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

As said I would have preferred this to be patch 1 of the series.

Jan

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

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

[Jan Beulich]

On 24.10.2019 05:45, Marek Marczykowski-Górecki wrote:
> 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>

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

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

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

[Jan Beulich]

On 24.10.2019 05:45, Marek Marczykowski-Górecki wrote:
> Some UEFI implementations are not happy about lack of
> SetVirtualAddressMap() call. Likely abuse the address map change
> notification to do things beyond the necessary ConvertPointer() calls.
> Specifically, wihtout the SetVirtualAddressMap() call, 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.
> 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>

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

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

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

[Jürgen Groß]

On 24.10.19 05:45, Marek Marczykowski-Górecki 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
> 
> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> 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>
> 
> 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?
> Changes in v4:
>   - reword commit messages, drop mentions of kexec
>   - new patch (3)
> 
> 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 (3):
>    efi: remove old SetVirtualAddressMap() arrangement
>    xen/efi: optionally call SetVirtualAddressMap()
>    xen/efi: use directmap to access runtime services table
> 
>   xen/common/Kconfig       | 10 ++++++++-
>   xen/common/efi/boot.c    | 52 +++++++++++++++++++++++------------------
>   xen/common/efi/runtime.c | 19 +++------------
>   3 files changed, 44 insertions(+), 37 deletions(-)
> 
> base-commit: 7a4e6711114905b3cbbe48e81c3222361a7f3579
> 

For the series:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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