Re: [PATCH v3 3/4] Add a new hypercall to get the ESRT

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Apr 28, 2022 at 08:47:49AM +0200, Jan Beulich wrote:
> On 27.04.2022 21:08, Demi Marie Obenour wrote:
> > On Wed, Apr 27, 2022 at 10:56:34AM +0200, Jan Beulich wrote:
> >> On 19.04.2022 17:49, Demi Marie Obenour wrote:
> >>> This hypercall can be used to get the ESRT from the hypervisor.  It
> >>> returning successfully also indicates that Xen has reserved the ESRT and
> >>> it can safely be parsed by dom0.
> >>
> >> I'm not convinced of the need, and I view such an addition as inconsistent
> >> with the original intentions. The pointer comes from the config table,
> >> which Dom0 already has access to. All a Dom0 kernel may need to know in
> >> addition is whether the range was properly reserved. This could be achieved
> >> by splitting the EFI memory map entry in patch 2, instead of only splitting
> >> the E820 derivation, as then XEN_FW_EFI_MEM_INFO can be used to find out
> >> the range's type. Another way to find out would be for Dom0 to attempt to
> >> map this area as MMIO, after first checking that no part of the range is in
> >> its own memory allocation. This 2nd approach may, however, not really be
> >> suitable for PVH Dom0, I think.
> > 
> > On further thought, I think the hypercall approach is actually better
> > than reserving the ESRT.  I really do not want XEN_FW_EFI_MEM_INFO to
> > return anything other than the actual firmware-provided memory
> > information, and the current approach seems to require more and more
> > special-casing of the ESRT, not to mention potentially wasting memory
> > and splitting a potentially large memory region into two smaller ones.
> > By copying the entire ESRT into memory owned by Xen, the logic becomes
> > significantly simpler on both the Xen and dom0 sides.
> 
> I actually did consider the option of making a private copy when you did
> send the initial version of this, but I'm not convinced this simplifies
> things from a kernel perspective: They'd now need to discover the table
> by some entirely different means. In Linux at least such divergence
> "just for Xen" hasn't been liked in the past.
> 
> There's also the question of how to propagate the information across
> kexec. But I guess that question exists even outside of Xen, with the
> area living in memory which the OS is expected to recycle.

Indeed it does.  A simple rule might be, “Only trust the ESRT if it is
in memory of type EfiRuntimeServicesData.”  That is easy to achieve by
monkeypatching the config table as you suggested below.

I *am* worried that the config table might be mapped read-only on some
systems, in which case the overwrite would cause a fatal page fault.  Is
there a way for Xen to check for this?  It could also be undefined
behavior to modify it.

> > Is using ebmalloc() to allocate a copy of the ESRT a reasonable option?
> 
> I'd suggest to try hard to avoid ebmalloc(). It ought to be possible to
> make the copy before ExitBootServices(), via normal EFI allocation. If
> replacing a pointer in the config table was okay(ish), this could even
> be utilized to overcome the kexec problem.

What type should I use for the allocation?  EfiLoaderData looks like the
most consistent choice, but I am not sure if memory so allocated remains
valid when Xen hands off to the OS, so EfiRuntimeServicesData might be a
better choice.  To avoid memory leaks from repeated kexec(), this could
be made conditional on the ESRT not being in memory of type
EfiRuntimeServicesData to begin with.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature