Re: [PATCH] Call SetVirtualAddressMap() by default

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.02.2023 14:56, Marek Marczykowski-Górecki wrote:
> On Thu, Feb 23, 2023 at 02:21:11PM +0100, Jan Beulich wrote:
>> On 23.02.2023 14:08, Marek Marczykowski-Górecki wrote:
>>> On Thu, Feb 23, 2023 at 11:16:28AM +0100, Jan Beulich wrote:
>>>> On 22.02.2023 20:14, Demi Marie Obenour wrote:
>>>>> To quote Andrew Cooper:
>>>>>
>>>>>> I know we've had this argument before, but not calling
>>>>>> SetVirtualAddressMap() isn't a viable option.  It's a prerequisite to
>>>>>> function on literally millions of devices
>>>>>
>>>>> Qubes OS has been shipping EFI_SET_VIRTUAL_ADDRESS_MAP for years, and I
>>>>> believe OpenXT and EVE ship it as well. Mark EFI_SET_VIRTUAL_ADDRESS_MAP
>>>>> as SUPPORTED and enable it by default.
>>>>
>>>> This is insufficient justification. The main reason why we don't call
>>>> it is because it can be called only once. Any entity taking over later
>>>> (e.g. via kexec) can't do anything anymore about the virtual address
>>>> associations once set. Hence what's needed to justify a change like
>>>> this is an explanation of why this restriction is not really an issue
>>>> to anyone in any case.
>>>
>>> AFAIR from the discussion about the original patch, kexec under Xen does
>>> not preserve runtime services working anyway, so this limitation is more
>>> about some possible kexec implementation in the future, not actually
>>> breaking something right now. And since Linux calls
>>> SetVirtualAddressMap() _and_ supports kexec just fine, it's definitely
>>> possible to design this future kexec extension to work after
>>> SetVirtualAddressMap() too.
>>>
>>> Relevant parts of that older discussion:
>>> - 
>>> https://lore.kernel.org/all/272a9354-bcb4-50a4-a251-6a453221d6e3@xxxxxxxxxx/T/#u
>>> - https://lore.kernel.org/all/20191009235725.GT8065@mail-itl/T/#u
>>
>> Well, there are various statements there without further reference. I'm
>> having a hard time seeing how a full-fledged Linux could do well without
>> runtime services, or without being able to set the virtual address map
>> to its liking. If they can, then a question would be why they need to
>> set the virtual address map in the first place (yes, there is this
>> supposed "firmware bugs" argument, which unfortunately I lack any proof
>> of; at the very least I'm unaware of bug reports against Xen boiling
>> down to us not making this call). 
> 
> The second link points at a thread of one of such bug reports.

Hmm, yes, digging through the about two dozen mails, I can see there is a
connection to (not) calling SetVirtualAddressMap() there.

>> Plus maybe they can merely because old
>> and new OS are similar enough in their (virtual) memory layout? IOW
>> kexec-ing to Linux for crash dumping purposes is just one (important)
>> example of the functionality that needs retaining. 
> 
> It works just fine with Xen calling SetVirtualAddressMap().
> SetVirtualAddressMap() is relevant only for using runtime services, and
> you don't need them for crash dumps. In fact, runtime services are not
> accessible to post-kexec Linux anyway, so this call doesn't change
> anything.
> Additionally, given most stuff works
> just fine with efi=no-rs proves it isn't severe limitation, if it really
> would need to be there - but as Andrew noted, given Linux example, it
> doesn't really need to be the case - it may simply require a bit more
> thinking when adding runtime services capability past kexec.

All of what you say here is what I had meant to cover by adding the
"(important)", which initially I didn't have. 

>> Once we get better
>> PVH Dom0 support, maybe other Dom0 OSes surface with entirely different
>> needs.
> 
> I find this claim rather weird. Runtime services are a thing that Xen
> needs to call, not some domain. And Xen has control over its memory
> layout.
> 
> _If_ PVH dom0 would really turn out to be incompatible with
> SetVirtualAddressMap() call by Xen (which I highly doubt), then some
> alternative for that case can be made. But that's only speculation.

The remark wasn't about Dom0 itself wanting runtime services access:
Dom0 isn't going to be provided such, at least not to the physical EFI's.
If such was needed for PVH Dom0, we'd need to wire it to virtual firmware
hooks (which in turn may or may not be viewed as against some of the
ideas of PVH).

The remark was instead meant to point out that such an alternative OS
may want to invoke another (native) instance of itself for e.g. crash
dumping purposes.

>> As said back then - part of the reason why in the original
>> implementation I've avoided making this call is because of the fear of
>> closing possible roads we may need to take in the future.
> 
> Yet, not calling SetVirtualAddressMap() leads to actual issues _right
> now_, not in some hypothetical undefined future.

That's the way you, Andrew, and others like to put it. My view at this
is that it's not the lack of the call, but the improper implementation
of firmware which leads to an apparent need for this call. Like for all
other firmware bug workarounds, I'm happy to accept any proposals for
workarounds, as long as such workarounds either don't impact spec-
compliant systems, or as long as they're off by default. In the case
here this would mean that I'm likely to approve a change which converts
the EFI_SET_VIRTUAL_ADDRESS_MAP Kconfig to a command line option (as
long as it looks technically correct, of course), improving flexibility
from the present build-time-only control. (Qubes and other distros
would then be free to change the default of that option from "false" to
"true", and that would be a pretty easy to maintain local change.)

However, part of the reason why originally this wasn't (even meant to
be) controllable via Kconfig (or any pre-Kconfig means) was that I
don't think the handling of the ->VirtualStart fields is correct for
this case in efi_init_memory(): The final layout simply cannot be done
there (after SetVirtualAddressMap() was already called). Since I don't
think laying out virtual space can be moved ahead, this would mean
moving the call to SetVirtualAddressMap() past this layout phase. Yet
then we may again end up calling runtime services functions (time
ones in particular, but note that I didn't check the order of things,
so "may" was deliberate) ahead of calling SetVirtualAddressMap().

Plus, once you look at the layout code, you'll find a number of cases
which simply end in an error message.

To confess - I was quite happy back at the time that I could use the
kexec concern as a justification for not needing to solve these issues
right away.

These issues would, however, need sorting (by code changes or verbal
justification) no matter whether the Kconfig default was to change
(or even just the EXPERT dependency to go away), or whether the
conversion to a command line option was the way to go.

Jan