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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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: signature.asc
Description: PGP signature

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