Re: [edk2-devel] Conflicting virtual addresses causing Runtime Services issues

["Ard Biesheuvel" <ardb@kernel.org>]

On Fri, 12 Mar 2021 at 07:00, Jon Nettleton <jon@solid-run.com> wrote:
>
> On Fri, Mar 12, 2021 at 4:02 AM Jon Nettleton via groups.io
> <jon=solid-run.com@groups.io> wrote:
> >
> > On Thu, Mar 11, 2021 at 11:39 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote:
> > > >
> > > > Adding Ard and Leif, comments below:
> > > >
...
> > > Thanks for inviting me to this party!
> > >
> > > So the tl;dr here is that some points get converted twice, which
> > > usually is not a problem because the virtual address resulting from
> > > the conversion is rarely mistaken for a physical address living in a
> > > EFI_MEMORY_RUNTIME region.
> > >
> > > So I agree with Laszlo's assertion that the consumer of a protocol has
> > > no business updating its protocol pointers, so this should definitely
> > > be fixed in the core VariableRuntime driver. However, given the
> > > typical nature of the variable stack, i.e., a platform specfic NOR
> > > flash driver combined with the generic FTW and variable drivers, doing
> > > so would likely break many out of tree platforms where the NOR flash
> > > driver does not bother to update its pointers at all.
> >
> > Not ideal, but we could add a flag to Runtime Services and let the platform
> > specify if it is remapping the FVB.  This would allow us to not break legacy
> > drivers, but still easily let properly designed platforms bypass this
> > behaviour.  As time progressed this could be used to for a deprecation
> > warning, until it became the default handling of FVB pointer conversion.
> >
> Something like the attached patch possibly?

Frankly, I don't think adding a PCD that does one or the other is the
right way to go here.

Instead, what we might do is:
- record the original values of the pointers at ExitBootServices() time
- don't touch the original values but the copies during SetVirtualAddressMap()
- in each exposed RuntimeService(), do a runtime check whether the
addresses in the protocol struct are equal to the converted copies,
and if not, update them.

This functionality should be enabled by default, but can be opted out
of by a platform if we dedicate a PCD to it that defaults to enabled.
At some point, we could add some kind of deprecation warning if the
PCD is enabled, and have a path forward to actually retiring it
completely.