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

From: Andrew Cooper <andrew.cooper3@citrix.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>,
	Tim Deegan <tim@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v2 2/2] xen/efi: optionally call SetVirtualAddressMap()
Date: Sat, 12 Oct 2019 18:13:28 +0100	[thread overview]
Message-ID: <251d8594-0d4f-fd4b-20c3-4f5edf3dc23f@citrix.com> (raw)
In-Reply-To: <20191012170040.GB28966@mail-itl>

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