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

From: Jan Beulich <jbeulich@suse.com>
To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
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>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()
Date: Wed, 23 Oct 2019 17:37:33 +0200	[thread overview]
Message-ID: <24d8f989-92e6-d6a0-7c77-f02ae6a4ef54@suse.com> (raw)
In-Reply-To: <aedda92afd26caac474870d44504074d3b2ff6d0.1570918263.git-series.marmarek@invisiblethingslab.com>

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