comments in-line
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? 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.
>> + 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?
>> +
>> + //
>> + // 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.)
>> + 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?
>> +
>> + //
>> + // 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.
>> + 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
