On Tue, 30 May 2023 at 08:32, Ni, Ray <ray...@intel.com> wrote:
>
> I didn't review the existing logic carefully. Several comments:
> 1. Assignments of several fields are skipped when executing in place. Do they
> matter?
> a). Image->NumberOfPages
> b). Image->ImageBasePage
>
These are used when freeing the image again, so NumberOfPages should
remain 0x0 in this case, so that unloading the image does not free any
pages.
> 2. PeCoffLoaderRelocateImage() is called even for XIP case. But I don't think
> it's expected that relocation really happens. Even if the fixed data is the
> same as the original data stored in MMIO device, MMIO writing might cause
> unexpected behavior.
>
PeCoffLoaderRelocateImage() currently calculates the adjustment
offset, and does nothing to the image if it is 0x0.
> 3. CoreFreePages() is called when image is not loaded successfully. Is it
> expected for XIP case?
>
Yes, but Image->NumberOfPages will be 0x0.
>
> > -----Original Message-----
> > From: Ard Biesheuvel <a...@kernel.org>
> > Sent: Monday, May 29, 2023 6:17 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <a...@kernel.org>; Ni, Ray <ray...@intel.com>; Yao,
> > Jiewen
> > <jiewen....@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Taylor Beebe
> > <t...@taylorbeebe.com>; Oliver Smith-Denny <o...@smith-denny.com>; Bi,
> > Dandan
> > <dandan...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Kinney,
> > Michael D <michael.d.kin...@intel.com>; Leif Lindholm
> > <quic_llind...@quicinc.com>; Michael Kubacki <mikub...@linux.microsoft.com>
> > Subject: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in
> > place if possible
> >
> > In the image loader, check whether an image has already been relocated
> > to the address from which it is being loaded. This is not something that
> > can happen by accident, and so we can assume that this means that the
> > image was intended to be executed in place.
> >
> > This removes a redundant copy of the image contents, and also permits
> > the image to be mapped with restricted permissions even before the CPU
> > arch protocol has been dispatched.
> >
> > Signed-off-by: Ard Biesheuvel <a...@kernel.org>
> > ---
> > MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 3dfab4829b3ca17f..621637e869daf62d 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -573,7 +573,7 @@ STATIC
> > EFI_STATUS
> >
> > CoreLoadPeImage (
> >
> > IN BOOLEAN BootPolicy,
> >
> > - IN VOID *Pe32Handle,
> >
> > + IN IMAGE_FILE_HANDLE *Pe32Handle,
> >
> > IN LOADED_IMAGE_PRIVATE_DATA *Image,
> >
> > IN UINT32 Attribute
> >
> > )
> >
> > @@ -630,10 +630,16 @@ CoreLoadPeImage (
> > return EFI_UNSUPPORTED;
> >
> > }
> >
> >
> >
> > + //
> >
> > + // Check whether the loaded image can be executed in place
> >
> > + //
> >
> > + if (Image->ImageContext.ImageAddress ==
> > (PHYSICAL_ADDRESS)(UINTN)Pe32Handle->Source) {
> >
> > + goto ExecuteInPlace;
> >
> > + }
> >
> > +
> >
> > //
> >
> > // Allocate Destination Buffer as caller did not pass it in
> >
> > //
> >
> > -
> >
> > if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
> >
> > Size = (UINTN)Image->ImageContext.ImageSize + Image-
> > >ImageContext.SectionAlignment;
> >
> > } else {
> >
> > @@ -704,6 +710,7 @@ CoreLoadPeImage (
> > //
> >
> > // Load the image from the file into the allocated memory
> >
> > //
> >
> > +ExecuteInPlace:
> >
> > Status = PeCoffLoaderLoadImage (&Image->ImageContext);
> >
> > if (EFI_ERROR (Status)) {
> >
> > goto Done;
> >
> > --
> > 2.39.2
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105428): https://edk2.groups.io/g/devel/message/105428
Mute This Topic: https://groups.io/mt/99197140/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
