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

[Xen-devel] [PATCH v2 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

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 v2 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 mapped with 1:1 mapping, which
   isn't the case at this time
 - it uses directmap, which is going 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>
---
 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 v2 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, hence it's incompatible
with kexec. For this reason, make it an optional feature, depending on
!KEXEC. And to not inflate number of supported configurations, hide it
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
---
 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..fe98f8a 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
+    depends on !KEXEC
+    ---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 makes
+      it incompatible with kexec (kexec-ing this Xen from other Xen or Linux).
+
+      If unsuser, 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 v2 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Andrew Cooper]

On 12/10/2019 15:36, Marek Marczykowski-Górecki wrote:
> SetVirtualAddressMap() can be called only once,

True.

> hence it's incompatible with kexec.

Most certainly not.

Linux unconditionally enters virtual mode, citing a huge slew of EFI
firmware bugs, and is perfectly capable of kexec-ing on the resulting
systems.

This is how Xen should behave as well, and I suspect it will have a
marked improvement on our ability to actually boot on EFI systems.


Now - it may be true that Xen is missing some piece of plumbing to allow
kexec in virtual mode to work, and that is a fine reason to leave a note
in the text of an EXPERT option noting what what is/isn't expected to
work (and what may or may not have been tested).

> For this reason, make it an optional feature, depending on
> !KEXEC.

This presupposes (at Xen's build time) that a kexec'd kernel is going to
want/need to use runtime services.  I'm not convinced this is
universally true, or a reasonable restriction to make, as kexec is the
action of last resort to try and get something useful out.  (However,
given the 4.13 timeline, and that this is off-by-default, lets not waste
time arguing, so it can stay as it is.)

> And to not inflate number of supported configurations, hide it
> behind EXPERT.

"number of supported configurations" isn't a relevant argument.  We will
have as few or as many as are appropriate to present to user, given a
baseline competency of "able to at read and comprehend the descriptions
given".

A valid reason for putting this behind EXPERT is because it is an
interim bit of duct tape, trying to work around other breakages in Xen,
and its late in the 4.13 dev cycle, and use of this option might cause
other things to explode in weird and wonderful ways.

> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 16829f6..fe98f8a 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
> +    depends on !KEXEC
> +    ---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 makes
> +      it incompatible with kexec (kexec-ing this Xen from other Xen or Linux).
> +
> +      If unsuser, say N.

"unsure".

> +
>  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);

Use this example...

>  
> +#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);

... here.  printk() isn't set up, and won't go anywhere useful.

With this, and a bit of rewording of the commit message and Kconfig
text, I think this is fine for inclusion into 4.13.  It is off by
default, so will not interfere with existing configuration, and it
clearly improves the status quo on others.

~Andrew

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

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

[Andrew Cooper]

On 12/10/2019 15:36, 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 mapped with 1:1 mapping, which
>    isn't the case at this time
>  - it uses directmap, which is going away soon
>  - it uses directmap, which is mapped with NX, breaking EfiRuntimeServicesCode

I think the second point is far more important than the first.  After
all, there is no guarantee at this point that the directmap is going to
disappear unilaterally - that is simply one option at the moment.

>
> No functional change.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

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

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

[Marek Marczykowski-Górecki]

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

On Sat, Oct 12, 2019 at 05:29:34PM +0100, Andrew Cooper wrote:
> On 12/10/2019 15:36, Marek Marczykowski-Górecki wrote:
> > SetVirtualAddressMap() can be called only once,
> 
> True.
> 
> > hence it's incompatible with kexec.
> 
> Most certainly not.
> 
> Linux unconditionally enters virtual mode, citing a huge slew of EFI
> firmware bugs, and is perfectly capable of kexec-ing on the resulting
> systems.
> 
> This is how Xen should behave as well, and I suspect it will have a
> marked improvement on our ability to actually boot on EFI systems.
> 
> 
> Now - it may be true that Xen is missing some piece of plumbing to allow
> kexec in virtual mode to work, and that is a fine reason to leave a note
> in the text of an EXPERT option noting what what is/isn't expected to
> work (and what may or may not have been tested).
> 
> > For this reason, make it an optional feature, depending on
> > !KEXEC.
> 
> This presupposes (at Xen's build time) that a kexec'd kernel is going to
> want/need to use runtime services.  I'm not convinced this is
> universally true,

In fact, as it turned out in the discussion, right now it definitely
can't, as it doesn't get runtime services table (or efi system table or
any other info required for this). So, it looks like it should read "it
might be incompatible with some future Xen implementation of kexec".

> or a reasonable restriction to make, as kexec is the
> action of last resort to try and get something useful out.  (However,
> given the 4.13 timeline, and that this is off-by-default, lets not waste
> time arguing, so it can stay as it is.)
> 
> > And to not inflate number of supported configurations, hide it
> > behind EXPERT.
> 
> "number of supported configurations" isn't a relevant argument.  We will
> have as few or as many as are appropriate to present to user, given a
> baseline competency of "able to at read and comprehend the descriptions
> given".
> 
> A valid reason for putting this behind EXPERT is because it is an
> interim bit of duct tape, trying to work around other breakages in Xen,

Rather in UEFI...

> and its late in the 4.13 dev cycle, and use of this option might cause
> other things to explode in weird and wonderful ways.
> 
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > index 16829f6..fe98f8a 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
> > +    depends on !KEXEC
> > +    ---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 makes
> > +      it incompatible with kexec (kexec-ing this Xen from other Xen or Linux).
> > +
> > +      If unsuser, say N.
> 
> "unsure".
> 
> > +
> >  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);
> 
> Use this example...
> 
> >  
> > +#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);
> 
> ... here.  printk() isn't set up, and won't go anywhere useful.

I can't. It's after ExitBootServices(). Isn't it going to land in
console ring, to be printed later?

> With this, and a bit of rewording of the commit message and Kconfig
> text, I think this is fine for inclusion into 4.13.  It is off by
> default, so will not interfere with existing configuration, and it
> clearly improves the status quo on others.
> 
> ~Andrew

-- 
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 v2 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Marek Marczykowski-Górecki]

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

On Sat, Oct 12, 2019 at 07:00:43PM +0200, Marek Marczykowski-Górecki wrote:
> On Sat, Oct 12, 2019 at 05:29:34PM +0100, Andrew Cooper wrote:
> > On 12/10/2019 15:36, Marek Marczykowski-Górecki wrote:
> > > SetVirtualAddressMap() can be called only once,
> > 
> > True.
> > 
> > > hence it's incompatible with kexec.
> > 
> > Most certainly not.
> > 
> > Linux unconditionally enters virtual mode, citing a huge slew of EFI
> > firmware bugs, and is perfectly capable of kexec-ing on the resulting
> > systems.
> > 
> > This is how Xen should behave as well, and I suspect it will have a
> > marked improvement on our ability to actually boot on EFI systems.
> > 
> > 
> > Now - it may be true that Xen is missing some piece of plumbing to allow
> > kexec in virtual mode to work, and that is a fine reason to leave a note
> > in the text of an EXPERT option noting what what is/isn't expected to
> > work (and what may or may not have been tested).
> > 
> > > For this reason, make it an optional feature, depending on
> > > !KEXEC.
> > 
> > This presupposes (at Xen's build time) that a kexec'd kernel is going to
> > want/need to use runtime services.  I'm not convinced this is
> > universally true,
> 
> In fact, as it turned out in the discussion, right now it definitely
> can't, as it doesn't get runtime services table (or efi system table or
> any other info required for this). So, it looks like it should read "it
> might be incompatible with some future Xen implementation of kexec".

Specifically, dependency on !KEXEC isn't needed right now.

-- 
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 v2 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Andrew Cooper]

On 12/10/2019 18:00, Marek Marczykowski-Górecki wrote:
>>> For this reason, make it an optional feature, depending on
>>> !KEXEC.
>> This presupposes (at Xen's build time) that a kexec'd kernel is going to
>> want/need to use runtime services.  I'm not convinced this is
>> universally true,
> In fact, as it turned out in the discussion, right now it definitely
> can't, as it doesn't get runtime services table (or efi system table or
> any other info required for this).

On, in which case definitely drop the dependency.  Kexec works fine (and
is tested thoroughly by XenServer) with current Xen when EFI booted, and
the call to SetVirtualAddressMap() won't make any difference in the
kexec environment.

Whomever does the plumbing for EFI details in the future can also pass
the virtual map (like Linux already does) and everything will continue
to work fine.

>
>>> And to not inflate number of supported configurations, hide it
>>> behind EXPERT.
>> "number of supported configurations" isn't a relevant argument.  We will
>> have as few or as many as are appropriate to present to user, given a
>> baseline competency of "able to at read and comprehend the descriptions
>> given".
>>
>> A valid reason for putting this behind EXPERT is because it is an
>> interim bit of duct tape, trying to work around other breakages in Xen,
> Rather in UEFI...

I suppose, yes.

>>>  
>>> +#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);
>> ... here.  printk() isn't set up, and won't go anywhere useful.
> I can't. It's after ExitBootServices(). Isn't it going to land in
> console ring, to be printed later?

Urgh.  Yes - you're right.  It will sit in the console buffer and appear
at the top of `xl dmesg`.

Leave it as-is.

~Andrew

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