Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 19 Apr 2019 at 20:23, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> Let's see if we can remove the ESRT access from those
> paths.  That would be the better fix.
>

I am not that familiar with this code, but it seems that the only
reason we access the ESRT at runtime is to check whether a capsule is
a nested FMP capsule, where the outer GUID is cross referenced against
the ESRT before checking whether the inner GUID is the FMP guid.

Could we relax this check? FMP capsule can only be dispatched across a
reboot anyway, and so the runtime capsule handling could accept any
capsule that has an inner FMP guid.

If not, it means we do need to read the ESRT at runtime, in which case
my patch is the simplest solution. We would have to clarify the spec
though.


> > -----Original Message-----
> > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > Biesheuvel
> > Sent: Friday, April 19, 2019 11:11 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; ming.huang@linaro.org; Wu,
> > Hao A <hao.a.wu@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH resend]
> > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > RtServicesData memory
> >
> > On Fri, 19 Apr 2019 at 20:08, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Ard,
> > >
> > > Where is the use of ESRT at runtime?
> > >
> >
> > DxeCapsuleLibFmp is invoked at runtime by the
> > UpdateCapsule() and
> > QueryCapsuleCapabilities() code.
> >
> > The Canonical FWTS crashes on this runtime service on
> > some ARM and x86
> > systems due to this issue, hence the patch.
> >
> >
> > > The SetVa conversion of the ESRT table may be an old
> > artifact
> > > that we should consider removing.
> > >
> >
> > No, it is definitely live code.
> >
> >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io
> > > > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > > > Biesheuvel
> > > > Sent: Friday, April 19, 2019 10:54 AM
> > > > To: edk2-devel-groups-io <devel@edk2.groups.io>;
> > > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > > Cc: ming.huang@linaro.org; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH resend]
> > > > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > > > RtServicesData memory
> > > >
> > > > On Fri, 19 Apr 2019 at 19:42, Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org> wrote:
> > > > >
> > > > > On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
> > > > > <michael.d.kinney@intel.com> wrote:
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > The UEFI Specification Section 23.3 says:
> > > > > >
> > > > > >         "The ESRT shall be stored in memory of
> > type
> > > > EfiBootServicesData."
> > > > > >
> > > > > > If an RT driver needs ESRT info after
> > > > ExitBootServices(), then the
> > > > > > RT driver should collect that information
> > before
> > > > ExitBootServices().
> > > > > >
> > > > >
> > > > > But that means that before EBS(), we will have to
> > > > make a copy of the
> > > > > ESRT into runtime services data, and we will end
> > up
> > > > using twice as
> > > > > much memory - one copy in boot services data for
> > the
> > > > OS, and one copy
> > > > > in runtime services data for the firmware itself.
> > I
> > > > understand the
> > > > > reasoning behind preferring boot over runtime, so
> > > > that the OS is not
> > > > > burdened with it if it does not care. But it this
> > > > case, the RT
> > > > > services data will be allocated regardless, and
> > so we
> > > > end up using
> > > > > less memory if we expose that same copy to the
> > OS.
> > > > >
> > > >
> > > > Actually, DxeCapsuleLibVirtualAddressChangeEvent()
> > in
> > > > DxeCapsuleLibFmp
> > > > updates the ESRT pointer to virtual, so this code
> > is
> > > > clearly based on
> > > > the assumption that the ESRT resides in memory that
> > is
> > > > covered by a
> > > > runtime VA mapping.
> > > >
> > > > Could we instead clarify the UEFI spec to something
> > > > like
> > > >
> > > > "The ESRT shall be stored in memory of type
> > > > EfiBootServicesData unless
> > > > the runtime services implementations themselves
> > need to
> > > > access it at
> > > > OS runtime, in which case it may be stored in
> > > > EfiRuntimeServicesData."
> > > >
> > > >
> > >
> >
> > 
>