Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [Xen-devel] [PATCH v2 2/2] xen/efi: optionally call SetVirtualAddressMap()
Date: Sat, 12 Oct 2019 17:29:34 +0100
Message-ID: <272a9354-bcb4-50a4-a251-6a453221d6e3@citrix.com> (raw)
In-Reply-To: <bf29c0ca9c1622e980883f11030e21f013312d3e.1570890895.git-series.marmarek@invisiblethingslab.com>

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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-12 14:36 [Xen-devel] [PATCH v2 0/2] Optionally call EFI SetVirtualAddressMap() Marek Marczykowski-Górecki
2019-10-12 14:36 ` [Xen-devel] [PATCH v2 1/2] efi: remove old SetVirtualAddressMap() arrangement Marek Marczykowski-Górecki
2019-10-12 16:33   ` Andrew Cooper
2019-10-12 14:36 ` [Xen-devel] [PATCH v2 2/2] xen/efi: optionally call SetVirtualAddressMap() Marek Marczykowski-Górecki
2019-10-12 16:29   ` Andrew Cooper [this message]
2019-10-12 17:00     ` Marek Marczykowski-Górecki
2019-10-12 17:09       ` Marek Marczykowski-Górecki
2019-10-12 17:13       ` Andrew Cooper

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=272a9354-bcb4-50a4-a251-6a453221d6e3@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jbeulich@suse.com \
    --cc=julien.grall@arm.com \
    --cc=konrad.wilk@oracle.com \
    --cc=marmarek@invisiblethingslab.com \
    --cc=sstabellini@kernel.org \
    --cc=tim@xen.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git