On Tue, 2012-11-13 at 16:18 +0100, Laszlo Ersek wrote:
> On 11/12/12 21:55, Laszlo Ersek wrote:
> > On 10/29/12 01:01, Jordan Justen wrote:
>
> >> +
> >> +
> >> +STATIC
> >> EFI_STATUS
> >> -TryRunningQemuKernel (
> >> - VOID
> >> +TryRunningPeCoffImage (
> >> + IN VOID *SetupBuf,
> >> + IN UINTN SetupSize,
> >> + IN UINTN KernelSize
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> + UINTN PeImageSize;
> >> + VOID *PeImageData;
> >> + EFI_HANDLE ImageHandle;
> >> + EFI_HANDLE DevicePathHandle;
> >> + MEMMAP_IMAGE_DEVICE_PATH DevicePath;
> >> +
> >> + PeImageData = NULL;
> >> + PeImageSize = 0;
> >> + Status = EFI_SUCCESS;
> >> +
> >> + //
> >> + // The first part of the PE/COFF image was loaded from the 'kernel
> >> setup'
> >> + // area. QEMU will put the remaining portion in the 'kernel data' area.
> >> + //
> >> + // We combine these two items into a single buffer to retrieve the
> >> original
> >> + // PE/COFF image.
> >> + //
> >> + PeImageSize = SetupSize + KernelSize;
>
> Let's assume the sum won't ever overflow.
>
> >> + PeImageData = AllocatePages (EFI_SIZE_TO_PAGES (PeImageSize));
> >> + if (PeImageData == NULL) {
> >> + Status = EFI_OUT_OF_RESOURCES;
> >> + goto FreeAndReturn;
> >> + }
> >> + CopyMem (PeImageData, SetupBuf, SetupSize);
> >> +
> >> + DEBUG ((EFI_D_INFO, "Remaining PE/COFF image size: 0x%x\n", (UINT32)
> >> KernelSize));
> >> + DEBUG ((EFI_D_INFO, "Reading PE/COFF image ..."));
> >> + QemuFwCfgSelectItem (QemuFwCfgItemKernelData);
> >> + QemuFwCfgReadBytes (KernelSize, ((UINT8*) PeImageData) + SetupSize);
> >> + DEBUG ((EFI_D_INFO, " [done]\n"));
> >> +
> >> + //
> >> + // Create a device path for the image
> >> + //
> >> + CopyMem ((VOID*) &DevicePath,
> >> + (VOID*) &mMemmapImageDevicePathTemplate,
> >> + sizeof (DevicePath));
>
> Ah this makes me ask another question regarding
> MEMMAP_IMAGE_DEVICE_PATH: shouldn't it be packed?
Yes, you are correct. It should be packed.
> AFAIK a device path is
> traversed node-by-node using "EFI_DEVICE_PATH_PROTOCOL.Length" at the
> beginning of each ("derived") node. Without packing, a compiler is
> theoretically allowed to insert padding between the two fields of
> MEMMAP_IMAGE_DEVICE_PATH, throwing off the traversal.
>
>
> >> + DevicePath.MemMapDevPath.StartingAddress = (UINTN) PeImageData;
> >> + DevicePath.MemMapDevPath.EndingAddress =
> >> + DevicePath.MemMapDevPath.StartingAddress + PeImageSize;
>
> Is "EndingAddress" exclusive?
>
> - "DuetPkg/FvbRuntimeService/FWBlockService.c" suggests it is inclusive
> (ie. base + size - 1).
> - OTOH, "ArmPkg/Library/BdsLib/BdsFilePath.c" suggests it is exclusive
> (ie. size = ending - starting)
> - The field name itself suggests inclusive.
The UEFI spec seems vague, but
MdeModulePkg/Core/Dxe/FwVolBlock/FwVolBlock.c
also suggests inclusive, so I should probably go with that.
> >> + DevicePathHandle = NULL;
> >> + Status = gBS->InstallProtocolInterface (
> >> + &DevicePathHandle,
> >> + &gEfiDevicePathProtocolGuid,
> >> + EFI_NATIVE_INTERFACE,
> >> + (VOID*) &DevicePath
> >> + );
>
> Status check missing.
>
> BTW why do we need to install the protocol interace on DevicePath for
> the rest of the world?
The new UEFI shell required this to be present.
> >> +
> >> + //
> >> + // Load and start the PE/COFF image
> >> + //
> >> + ImageHandle = NULL;
> >> + Status = gBS->LoadImage (
> >> + FALSE,
> >> + gImageHandle,
> >> + (VOID*) &DevicePath,
> >> + PeImageData,
> >> + PeImageSize,
> >> + &ImageHandle
> >> + );
>
> Do we need both DevicePath and PeImageData/PeImageSize here? (Just
> trying to learn, the spec of LoadImage() is hard to interpret.)
For UEFI in general, I think either can be used. In this
situation a pointer+size is the most useful, but like I
mentioned above, I added the device path to allow the new
UEFI shell to run.
> >> + DEBUG ((EFI_D_INFO, "Loading PE/COFF image -> %r\n", Status));
> >> +
> >> + if (!EFI_ERROR (Status)) {
> >> + DEBUG ((EFI_D_INFO, "Starting PE/COFF image ...\n"));
> >> + Status = gBS->StartImage (ImageHandle, NULL, NULL);
> >> + DEBUG ((EFI_D_INFO, "PE/COFF image exited -> %r\n", Status));
> >> + }
>
> Again, I do not understand UnloadImage() spec: what happens if the image
> - has been loaded,
> - has been started,
> - does not have an Unload entry point?
>
> UnloadImage() returns EFI_UNSUPPORTED. That's OK, but what happens to
> the image when StartImage() returns? Is it automatically unloaded, or
> leaked forever?
The DXE Core will automatically unload applications regardless of
the return value, and it will unload drivers if an error value
is returned.
MdeModulePkg/Core/Dxe/Image/Image.c
> >> +
> >> + //
> >> + // Uninstall the device path for the image
> >> + //
> >> + Status = gBS->UninstallProtocolInterface (
> >> + DevicePathHandle,
> >> + &gEfiDevicePathProtocolGuid,
> >> + (VOID*) &DevicePath
> >> + );
>
> This overwrites the status returned by the image. May not matter in
> practice, but I think the intent is to propagate it.
Yes, I think you are right.
Thanks,
-Jordan
> >> + ASSERT_EFI_ERROR (Status);
> >> +
> >> +FreeAndReturn:
> >> + if (PeImageData != NULL) {
> >> + FreePages (PeImageData, EFI_SIZE_TO_PAGES (PeImageSize));
> >> + }
> >> +
> >> + return Status;
> >> +}
> >> +
> >> +
>
> >> +EFI_STATUS
> >> +TryRunningQemuKernel (
> >> + VOID
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> + UINTN SetupSize;
> >> + VOID *SetupBuf;
> >> + UINTN KernelSize;
> >> +
> >> + SetupBuf = NULL;
> >> + SetupSize = 0;
> >> +
> >> + if (!QemuFwCfgIsAvailable ()) {
> >> + return EFI_NOT_FOUND;
> >> + }
> >> +
> >> + QemuFwCfgSelectItem (QemuFwCfgItemKernelSize);
> >> + KernelSize = (UINTN) QemuFwCfgRead64 ();
> >> +
> >> + QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
> >> + SetupSize = (UINTN) QemuFwCfgRead64 ();
> >> +
> >> + if (KernelSize == 0 || SetupSize == 0) {
> >> + DEBUG ((EFI_D_INFO, "qemu -kernel was not used.\n"));
> >> + return EFI_NOT_FOUND;
> >> + }
> >> +
> >> + SetupBuf = LoadLinuxAllocateKernelSetupPages (EFI_SIZE_TO_PAGES
> >> (SetupSize));
> >> + if (SetupBuf == NULL) {
> >> + DEBUG ((EFI_D_ERROR, "Unable to allocate memory for kernel
> >> setup!\n"));
> >> + return EFI_OUT_OF_RESOURCES;
> >> + }
> >> +
> >> + DEBUG ((EFI_D_INFO, "Setup size: 0x%x\n", (UINT32) SetupSize));
> >> + DEBUG ((EFI_D_INFO, "Reading kernel setup image ..."));
> >> + QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupData);
> >> + QemuFwCfgReadBytes (SetupSize, SetupBuf);
> >> + DEBUG ((EFI_D_INFO, " [done]\n"));
> >> +
> >> + if (LooksLikeAPeCoffImage (SetupBuf, SetupSize)) {
> >> + Status = TryRunningPeCoffImage (SetupBuf, SetupSize, KernelSize);
> >> + } else {
> >> + Status = TryRunningKernelImage (SetupBuf, SetupSize, KernelSize);
> >> + }
> >> +
> >> + if (SetupBuf != NULL) {
> >> + FreePages (SetupBuf, EFI_SIZE_TO_PAGES (SetupSize));
> >> + }
>
> Sorry for not noticing this earlier: SetupBuf is never NULL here.
>
> >> +
> >> + return Status;
> >> +}
>
> No more comments.
>
> Thanks,
> Laszlo
------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
