On 10/20/12 02:58, Laszlo Ersek wrote:
>
> On 10/20/12 00:38, Jordan Justen wrote:
Last time I forgot to propose to move the ExitBootServices() call
LoadLinux
SetupLinuxBootParams
SetupLinuxMemmap
ExitBootServices <-- from here
<to here>
DisableInterrupts
SetLinuxDescriptorTables
JumpToKernel
(without any functional changes), but considering it again, the current
place is probably the right one. Once we saved the memory map, we should
not be able to do anything that changes it (like allocate memory), and
thus they ExitBootServices should not be a separate act.
But I think it would warrant some documentation for
SetupLinuxBootParams/SetupLinuxMemmap.
>
>> +/**
>> + Loads and boots UEFI Linux.
>> +
>> + @param[in] Kernel - The main kernel image
>> + @param[in] KernelSetup - The kernel setup image
>> + @param[in] CommandLine - The kernel command line
CommandLine is not a parameter here, and KernelSetup is IN OUT.
>> +
>> + @retval EFI_NOT_FOUND - The Linux kernel was not found
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +LoadLinux (
>> + IN VOID *Kernel,
>> + IN VOID *KernelSetup
>> + )
>> +{
>> + struct boot_params *Bp;
>> +
>> + Bp = (struct boot_params *) KernelSetup;
>> +
>> + if (Bp->hdr.version < 0x205) {
>> + //
>> + // We only support relocatable kernels
>> + //
>> + return EFI_UNSUPPORTED;
>> + }
>> +
>> + InitLinuxDescriptorTables ();
>> +
>> + SetupLinuxBootParams (Kernel, (struct boot_params*) KernelSetup);
>> +
>> + DEBUG ((EFI_D_INFO, "Jumping to kernel\n"));
>> + DisableInterrupts ();
>> + SetLinuxDescriptorTables ();
>> + JumpToKernel (Kernel, (VOID*) KernelSetup);
>> +
>> + return EFI_SUCCESS;
>> +}
I think EFI_SUCCESS is not appropriate here. The documentation in the
header says EFI_NOT_FOUND, but I doubt if we reach the final return
(after JumpToKernel) we can do anything else useful. What are your
thoughts about ASSERT(FALSE) + return EFI_NOT_FOUND?
>> +
>> diff --git a/OvmfPkg/Library/LoadLinuxLib/LinuxGdt.c
>> b/OvmfPkg/Library/LoadLinuxLib/LinuxGdt.c
>> new file mode 100644
>> index 0000000..653adcf
>> --- /dev/null
>> +++ b/OvmfPkg/Library/LoadLinuxLib/LinuxGdt.c
>> @@ -0,0 +1,200 @@
>> +/** @file
>> + Initialize GDT for Linux.
>> +
>> + Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
>> + This program and the accompanying materials
>> + are licensed and made available under the terms and conditions of the BSD
>> License
>> + which accompanies this distribution. The full text of the license may be
>> found at
>> + http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#include "LoadLinuxLib.h"
>> +
>> +
>> +//
>> +// Local structure definitions
>> +//
>> +
>> +#pragma pack (1)
>> +
>> +//
>> +// Global Descriptor Entry structures
>> +//
>> +
>> +typedef struct _GDT_ENTRY {
>> + UINT16 Limit15_0;
>> + UINT16 Base15_0;
>> + UINT8 Base23_16;
>> + UINT8 Type;
>> + UINT8 Limit19_16_and_flags;
>> + UINT8 Base31_24;
>> +} GDT_ENTRY;
>> +
>> +typedef
>> +struct _GDT_ENTRIES {
>> + GDT_ENTRY Null;
>> + GDT_ENTRY Null2;
>> + GDT_ENTRY Linear;
>> + GDT_ENTRY LinearCode;
>> + GDT_ENTRY TaskSegment;
>> + GDT_ENTRY Spare4;
>> + GDT_ENTRY Spare5;
>> +} GDT_ENTRIES;
>> +
>> +#pragma pack ()
>> +
>> +#define NULL_SEL OFFSET_OF (GDT_ENTRIES, Null)
>> +#define LINEAR_SEL OFFSET_OF (GDT_ENTRIES, Linear)
>> +#define LINEAR_CODE_SEL OFFSET_OF (GDT_ENTRIES, LinearCode)
>> +#define SYS_DATA_SEL OFFSET_OF (GDT_ENTRIES, SysData)
>> +#define SYS_CODE_SEL OFFSET_OF (GDT_ENTRIES, SysCode)
>> +#define LINEAR_CODE64_SEL OFFSET_OF (GDT_ENTRIES, LinearCode64)
These three fields don't exist in the structure. (Hence the macros are
probably unused.)
>> +#define SPARE4_SEL OFFSET_OF (GDT_ENTRIES, Spare4)
>> +#define SPARE5_SEL OFFSET_OF (GDT_ENTRIES, Spare5)
>> +
>> +#if defined (MDE_CPU_IA32)
>> +#define CPU_CODE_SEL LINEAR_CODE_SEL
>> +#define CPU_DATA_SEL LINEAR_SEL
>> +#elif defined (MDE_CPU_X64)
>> +#define CPU_CODE_SEL LINEAR_CODE64_SEL
>> +#define CPU_DATA_SEL LINEAR_SEL
>> +#else
>> +#error CPU type not supported for CPU GDT initialization!
>> +#endif
"#define CPU_DATA_SEL LINEAR_SEL" could be moved down as a common
definition.
>> +
>> +STATIC GDT_ENTRIES *mGdt = NULL;
>> +
>> +//
>> +// Global descriptor table (GDT) Template
>> +//
>> +STATIC GDT_ENTRIES GdtTemplate = {
>> + //
>> + // NULL_SEL
>> + //
>> + {
>> + 0x0, // limit 15:0
>> + 0x0, // base 15:0
>> + 0x0, // base 23:16
>> + 0x0, // type
>> + 0x0, // limit 19:16, flags
>> + 0x0, // base 31:24
>> + },
>> + //
>> + // NULL_SEL
>> + //
>> + {
>> + 0x0, // limit 15:0
>> + 0x0, // base 15:0
>> + 0x0, // base 23:16
>> + 0x0, // type
>> + 0x0, // limit 19:16, flags
>> + 0x0, // base 31:24
>> + },
Null2, but there's no macro for it.
>> + //
>> + // LINEAR_CODE_SEL
>> + //
>> + {
>> + 0x0FFFF, // limit 0xFFFFF
>> + 0x0, // base 0
>> + 0x0,
>> + 0x09A, // present, ring 0, data, expand-up, writable
>> + 0x0CF, // page-granular, 32-bit
>> + 0x0,
>> + },
>> + //
>> + // LINEAR_SEL
>> + //
>> + {
>> + 0x0FFFF, // limit 0xFFFFF
>> + 0x0, // base 0
>> + 0x0,
>> + 0x092, // present, ring 0, data, expand-up, writable
>> + 0x0CF, // page-granular, 32-bit
>> + 0x0,
>> + },
The macros are in reverse order, Linear comes before LinearCode. Also
the Type values are different (0x9A != 0x92), but the comment is the same.
>> + //
>> + // TASK_SEL
>> + //
>> + {
>> + 0x0, // limit 0
>> + 0x0, // base 0
>> + 0x0,
>> + 0x089, // ?
>> + 0x080, // ?
>> + 0x0,
>> + },
TaskSegment indeed, but TASK_SEL is not defined.
>> + //
>> + // SPARE4_SEL
>> + //
>> + {
>> + 0x0, // limit 0
>> + 0x0, // base 0
>> + 0x0,
>> + 0x0, // present, ring 0, data, expand-up, writable
>> + 0x0, // page-granular, 32-bit
>> + 0x0,
>> + },
>> + //
>> + // SPARE5_SEL
>> + //
>> + {
>> + 0x0, // limit 0
>> + 0x0, // base 0
>> + 0x0,
>> + 0x0, // present, ring 0, data, expand-up, writable
>> + 0x0, // page-granular, 32-bit
>> + 0x0,
>> + },
>> +};
Type == 0x00; I think the comments are inaccurate.
Plus I'm not sure about Limit19_16_and_flags; is 0x0 "page-granular,
32-bit" the same way as 0x0CF?
>> +
>> +/**
>> + Initialize Global Descriptor Table.
>> +
>> +**/
>> +VOID
>> +InitLinuxDescriptorTables (
>> + VOID
>> + )
>> +{
>> + //
>> + // Allocate Runtime Data for the GDT
>> + //
>> + mGdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
>> + ASSERT (mGdt != NULL);
>> + mGdt = ALIGN_POINTER (mGdt, 8);
I'm not sure, but can we exploit that AllocateRuntimePool() is a wrapper
around gBS->AllocatePool() here? If so, then the spec says "All
allocations are eight-byte aligned."
Hm, MODULE_TYPE is BASE, so I guess we might use the library in earlier
phases (where AllocateRuntimePool() might work differently for what I
know). OK, it's safest like this.
Anyway I think adding 7 instead of 8 to sizeof (GdtTemplate) should
suffice; ALIGN_POINTER (..., 8) shouldn't move the pointer by more than
7 bytes.
>> +
>> + //
>> + // Initialize all GDT entries
>> + //
>> + CopyMem (mGdt, &GdtTemplate, sizeof (GdtTemplate));
What speaks against "*mGdt = GdtTemplate"?
>> +
>> +}
>> +
>> +/**
>> + Initialize Global Descriptor Table.
>> +
>> +**/
>> +VOID
>> +SetLinuxDescriptorTables (
>> + VOID
>> + )
>> +{
>> + IA32_DESCRIPTOR GdtPtr;
>> + IA32_DESCRIPTOR IdtPtr;
>> +
>> + //
>> + // Write GDT register
>> + //
>> + GdtPtr.Base = (UINT32)(UINTN)(VOID*) mGdt;
>> + GdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
According to the Intel "LGDT" manual, "The source operand specifies a
6-byte memory location that contains the base address (a linear address)
and the limit (size of table in bytes) of the global descriptor table
(GDT)". I'm not sure but I suspect the -1 is not needed.
Additionally, is EfiRuntimeServicesData always allocated below 4 GB?
(Sorry for the lame question.)
... Apologies for the mostly irrelevant points!
Laszlo
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_sfd2d_oct
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
