comments in-line
On 10/29/12 01:01, Jordan Justen wrote:
> [Note: This patch depends on my previous QEMU -kernel patch series.]
>
> If QEMU's -kernel is used with a UEFI executable, then it will
> be launched as a standard UEFI image, and not treated as a
> Linux kernel image.
>
> So, for example, even though we build the older EFI shell into
> OVMF by default, you can use a command such at this to launch
> OVMF and run the new UEFI shell:
> OvmfPkg/build.sh qemu -kernel ShellBinPkg/UefiShell/X64/Shell.efi
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen <[email protected]>
> ---
> OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 3 +-
> OvmfPkg/Library/PlatformBdsLib/QemuKernel.c | 275
> ++++++++++++++++++---
> 2 files changed, 238 insertions(+), 40 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
> b/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
> index 7f7f473..a293b30 100644
> --- a/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
> +++ b/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
> @@ -63,4 +63,5 @@
>
> [Protocols]
> gEfiDecompressProtocolGuid
> -
> + gEfiDevicePathProtocolGuid
> + gEfiLoadedImageProtocolGuid
> diff --git a/OvmfPkg/Library/PlatformBdsLib/QemuKernel.c
> b/OvmfPkg/Library/PlatformBdsLib/QemuKernel.c
> index fa8bcbc..09684a0 100644
> --- a/OvmfPkg/Library/PlatformBdsLib/QemuKernel.c
> +++ b/OvmfPkg/Library/PlatformBdsLib/QemuKernel.c
> @@ -13,32 +13,205 @@
>
> #include <Uefi.h>
>
> +#include <IndustryStandard/PeImage.h>
> +
> #include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> #include <Library/LoadLinuxLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/QemuFwCfgLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
>
> +typedef struct {
> + MEMMAP_DEVICE_PATH MemMapDevPath;
> + EFI_DEVICE_PATH_PROTOCOL EndDevPath;
> +} MEMMAP_IMAGE_DEVICE_PATH;
> +
> +MEMMAP_IMAGE_DEVICE_PATH mMemmapImageDevicePathTemplate = {
> + {
> + {
> + HARDWARE_DEVICE_PATH,
> + HW_MEMMAP_DP,
> + {
> + (UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
> + (UINT8)(sizeof (MEMMAP_DEVICE_PATH) >> 8)
> + }
> + },
> + EfiBootServicesData,
> + (EFI_PHYSICAL_ADDRESS) 0,
> + (EFI_PHYSICAL_ADDRESS) 0,
> + },
> + {
> + END_DEVICE_PATH_TYPE,
> + END_ENTIRE_DEVICE_PATH_SUBTYPE,
> + {
> + END_DEVICE_PATH_LENGTH,
> + 0
It doesn't matter much, but for readability I'd suggest
"END_DEVICE_PATH_LENGTH >> 8".
> + }
> + }
> +};
> +
> +/**
> + Determines if a buffer looks like a PE/COFF image.
> +
> + @param Buffer The buffer containing a potential PE/COFF image.
> + @param Size The size of the buffer
[in] qualifiers missing, but they may not be that important
> +
> + @retval TRUE The buffer appears to be a PE/COFF image
> + @retval FALSE The buffer does not appear to be a PE/COFF image
> +
> +**/
> +BOOLEAN
> +LooksLikeAPeCoffImage (
> + IN VOID *Buffer,
> + IN UINTN Size
> + )
> +{
> + EFI_IMAGE_DOS_HEADER *DosHdr;
> + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
> +
> + ASSERT (Buffer != NULL);
extraneous whitespace
> +
> + DosHdr = (EFI_IMAGE_DOS_HEADER *)Buffer;
I'm not familiar with the headers you're checking here and I'll have to
look them up, but I think we should make sure
Size >= sizeof (EFI_IMAGE_DOS_HEADER)
(also for the other headers) before converting the pointer.
> + if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
> + //
> + // DOS image header is present, so read the PE header after the DOS
> image header.
> + //
> + Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN) Buffer + (UINTN)
> ((DosHdr->e_lfanew) & 0x0ffff));
(a) why not just 0xffff?
(b) the expression relies on the following, thus I'd suggest checking it
explicitly:
Size >= ((DosHdr->e_lfanew & 0xffff) +
sizeof (EFI_IMAGE_NT_HEADERS32))
(The addition on the right side can't overflow on either supported
platform.)
> + } else {
> + //
> + // DOS image header is not present, so PE header is at the image base.
> + //
> + Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *) Buffer;
Size >= sizeof (EFI_IMAGE_NT_HEADERS32)
> + }
> +
> + //
> + // Does the signature show a PE32 or TE image?
> + //
> + if ((Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) ||
> + (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)) {
After the additional checks suggested above, dereferencing Hdr.Pe32
should be OK. However EFI_TE_IMAGE_HEADER could be bigger than
EFI_IMAGE_NT_HEADERS32 (it seems to be the case in fact), for which case
we have to redo the same checks.
> + return TRUE;
> + } else {
> + return FALSE;
> + }
> +}
I suggest rewriting the function like this:
BOOLEAN
LooksLikeAPeCoffImage (
IN VOID *Buffer,
IN UINTN Size
)
{
EFI_IMAGE_DOS_HEADER *DosHdr;
UINTN Offset;
EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
ASSERT (Buffer != NULL);
if (Size < sizeof (EFI_IMAGE_DOS_HEADER)) {
return FALSE;
}
DosHdr = (EFI_IMAGE_DOS_HEADER *) Buffer;
//
// If DOS image header is present, search for the PE header after the DOS
// image.
//
Offset = (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) ?
(DosHdr->e_lfanew) & 0xffff :
0;
//
// The following check is for a sufficient (not necessary) condition, but in
// practice it should never refuse a valid image.
// The addition on the RHS never overflows.
//
if (Size < Offset + sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION)) {
return FALSE;
}
Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *) ((UINT8 *) Buffer + Offset);
//
// Does the signature show a PE32 or TE image?
//
if ((Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) ||
(Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)) {
return TRUE;
} else {
return FALSE;
}
}
Review to be continued -- apologies for the inconvenience.
Thanks,
Laszlo
> +
> +
> +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;
> + 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));
> + DevicePath.MemMapDevPath.StartingAddress = (UINTN) PeImageData;
> + DevicePath.MemMapDevPath.EndingAddress =
> + DevicePath.MemMapDevPath.StartingAddress + PeImageSize;
> + DevicePathHandle = NULL;
> + Status = gBS->InstallProtocolInterface (
> + &DevicePathHandle,
> + &gEfiDevicePathProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + (VOID*) &DevicePath
> + );
> +
> + //
> + // Load and start the PE/COFF image
> + //
> + ImageHandle = NULL;
> + Status = gBS->LoadImage (
> + FALSE,
> + gImageHandle,
> + (VOID*) &DevicePath,
> + PeImageData,
> + PeImageSize,
> + &ImageHandle
> + );
> + 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));
> + }
> +
> + //
> + // Uninstall the device path for the image
> + //
> + Status = gBS->UninstallProtocolInterface (
> + DevicePathHandle,
> + &gEfiDevicePathProtocolGuid,
> + (VOID*) &DevicePath
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> +FreeAndReturn:
> + if (PeImageData != NULL) {
> + FreePages (PeImageData, EFI_SIZE_TO_PAGES (PeImageSize));
> + }
> +
> + return Status;
> +}
> +
> +
> +STATIC
> +EFI_STATUS
> +TryRunningKernelImage (
> + IN VOID *SetupBuf,
> + IN UINTN SetupSize,
> + IN UINTN KernelSize
> )
> {
> EFI_STATUS Status;
> - UINTN KernelSize;
> - UINTN KernelInitialSize;
> - VOID *KernelBuf;
> - UINTN SetupSize;
> - VOID *SetupBuf;
> UINTN CommandLineSize;
> CHAR8 *CommandLine;
> + UINTN KernelInitialSize;
> + VOID *KernelBuf;
> UINTN InitrdSize;
> VOID* InitrdData;
>
> - SetupBuf = NULL;
> - SetupSize = 0;
> KernelBuf = NULL;
> KernelInitialSize = 0;
> CommandLine = NULL;
> @@ -46,33 +219,6 @@ TryRunningQemuKernel (
> InitrdData = NULL;
> InitrdSize = 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"));
> -
> Status = LoadLinuxCheckKernelSetup (SetupBuf, SetupSize);
> if (EFI_ERROR (Status)) {
> goto FreeAndReturn;
> @@ -141,9 +287,6 @@ TryRunningQemuKernel (
> Status = LoadLinux (KernelBuf, SetupBuf);
>
> FreeAndReturn:
> - if (SetupBuf != NULL) {
> - FreePages (SetupBuf, EFI_SIZE_TO_PAGES (SetupSize));
> - }
> if (KernelBuf != NULL) {
> FreePages (KernelBuf, EFI_SIZE_TO_PAGES (KernelInitialSize));
> }
> @@ -157,3 +300,57 @@ FreeAndReturn:
> 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));
> + }
> +
> + return Status;
> +}
> +
------------------------------------------------------------------------------
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
