Re: [PATCH] Call SetVirtualAddressMap() by default

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

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. The first
link points at thread, where initial message contains list of systems
affected. If that is not enough, I can point you also dig up (at least)
tens of Qubes users reporting similar issues in the past. I'm sure
Andrew could do the same about XenServer (which is why "mapbs"
workaround exists).

> 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.

> 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.

> 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.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature