On Mon, 2012-10-22 at 10:45 +0100, Matt Fleming wrote:
> On Sun, 2012-10-21 at 15:44 -0700, Jordan Justen wrote:
> > This code is based on efilinux's bzimage support.
> > git://git.kernel.org/pub/scm/boot/efilinux/efilinux.git
> >
> > Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
> > Cc: Matt Fleming <matt.flem...@intel.com>
> > ---
> > OvmfPkg/Library/LoadLinuxLib/Ia32/JumpToKernel.S | 29 ++
> > OvmfPkg/Library/LoadLinuxLib/Ia32/JumpToKernel.asm | 35 ++
> > OvmfPkg/Library/LoadLinuxLib/Linux.c | 487
> > ++++++++++++++++++++
> > OvmfPkg/Library/LoadLinuxLib/LinuxGdt.c | 200 ++++++++
> > OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.h | 56 +++
> > OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf | 50 ++
> > OvmfPkg/Library/LoadLinuxLib/X64/JumpToKernel.S | 30 ++
> > OvmfPkg/Library/LoadLinuxLib/X64/JumpToKernel.asm | 34 ++
> > OvmfPkg/OvmfPkgIa32.dsc | 1 +
> > OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> > OvmfPkg/OvmfPkgX64.dsc | 1 +
> > 11 files changed, 924 insertions(+)
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/Ia32/JumpToKernel.S
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/Ia32/JumpToKernel.asm
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/Linux.c
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/LinuxGdt.c
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.h
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/X64/JumpToKernel.S
> > create mode 100644 OvmfPkg/Library/LoadLinuxLib/X64/JumpToKernel.asm
>
> [...]
>
> > +VOID*
> > +EFIAPI
> > +LoadLinuxAllocateKernelSetupPages (
> > + IN UINTN Pages
> > + )
> > +{
> > + EFI_STATUS Status;
> > + EFI_PHYSICAL_ADDRESS Address;
> > +
> > + Address = 0xa0000;
> > + Status = gBS->AllocatePages (
> > + AllocateMaxAddress,
> > + EfiLoaderData,
> > + Pages,
> > + &Address
> > + );
> > + if (EFI_ERROR (Status)) {
> > + return NULL;
> > + }
> > +
> > + return (VOID*)(UINTN) Address;
> > +}
>
> It looks like you're using this function to allocate memory for the
> "struct boot_params" that you pass to the kernel image. This memory
> doesn't need to be allocated under 0xa0000, and in fact, it would
> probably be better if it wasn't since there's very little memory there
> anyway.
>
> But the memory you allocate for "struct boot_params" does need to be
> below the 1GB mark, which is what efilinux commit 8947b2 that you
> highlighted in the covering letter does - that commit doesn't affect the
> memory allocation for the initrd (see comments below for initrd
> allocation restrictions).
Whoops. Thanks for pointing this out.
> > +VOID*
> > +EFIAPI
> > +LoadLinuxAllocateInitrdPages (
> > + IN UINTN Pages
> > + )
> > +{
> > + EFI_STATUS Status;
> > + EFI_PHYSICAL_ADDRESS Address;
> > +
> > + Address = BASE_1GB;
> > + Status = gBS->AllocatePages (
> > + AllocateMaxAddress,
> > + EfiLoaderData,
> > + Pages,
> > + &Address
> > + );
> > + if (!EFI_ERROR (Status)) {
> > + return (VOID*)(UINTN) Address;
> > + } else {
> > + return NULL;
> > + }
> > +}
>
> What you should actually be using to restrict the allocation of the
> initrd is Bp->hdr.ramdisk_max, which may be much higher than 1GB. The
> initrd is accessed pretty late in the boot procedure, way after "proper"
> pagetables have been installed that lift the 1GB restriction.
>
> > +
> > +STATIC
> > +VOID
> > +SetupLinuxMemmap (
> > + IN OUT struct boot_params *Bp
> > + )
> > +{
> > + EFI_STATUS Status;
> > + UINT8 TmpMemoryMap[1];
> > + UINTN MapKey;
> > + UINTN DescriptorSize;
> > + UINT32 DescriptorVersion;
> > + UINTN MemoryMapSize;
> > + EFI_MEMORY_DESCRIPTOR *MemoryMap;
> > + EFI_MEMORY_DESCRIPTOR *MemoryMapPtr;
> > + UINTN Index;
> > + struct efi_info *Efi;
> > + struct e820_entry *LastE820;
>
> [...]
>
> > + LastE820 = &Bp->e820_map[0];
>
> [...]
>
> > +
> > + if ((LastE820 != NULL) &&
> > + (LastE820->type == (UINT32) E820Type) &&
> > + (MemoryMap->PhysicalStart == LastEndAddr)) {
> > + LastE820->size += EFI_PAGES_TO_SIZE (MemoryMap->NumberOfPages);
>
> Is the (LastE820 != NULL) expression ever false?
I think it is set just below whenever a new E820 entry is modified.
> > + } else {
> > + if (E820EntryCount >= (sizeof (Bp->e820_map) / sizeof
> > (Bp->e820_map[0]))) {
> > + break;
> > + }
> > + E820->type = (UINT32) E820Type;
> > + E820->addr = MemoryMap->PhysicalStart;
> > + E820->size = EFI_PAGES_TO_SIZE (MemoryMap->NumberOfPages);
> > + LastE820 = E820;
> > + LastEndAddr = E820->addr + E820->size;
> > + E820++;
> > + E820EntryCount++;
> > + }
> > +
> > + //
> > + // Get next item
> > + //
> > + MemoryMap = (EFI_MEMORY_DESCRIPTOR *)((UINTN)MemoryMap +
> > DescriptorSize);
> > + }
> > + Bp->e820_entries = (UINT8) E820EntryCount;
>
> I think Laszlo has already mentioned that this code doesn't handle the
> case where, even after merging adjacent entries of the same time, we
> have more than 128 entries.
I took Laszlo's comment to mean I wasn't checking if I overflowed
the e820 entries. At least that should be handled in the
'if (...) break' code just above.
> The efilinux code doesn't either but such
> machines do exist. It's something that's been on my TODO list for a
> while. You need to stuff the extra entries in a setup_data entry with
> type SETUP_E280_EXT.
I think I'll wait for you to add this, and then port it over. :)
> > +EFI_STATUS
> > +EFIAPI
> > +LoadLinuxSetInitrd (
> > + IN OUT VOID *KernelSetup,
> > + IN VOID *Initrd,
> > + IN UINTN InitrdSize
> > + )
> > +{
> > + struct boot_params *Bp;
> > +
> > + RETURN_STATUS_ON_ERROR (BasicKernelSetupCheck (KernelSetup));
> > +
> > + Bp = (struct boot_params*) KernelSetup;
> > +
> > + Bp->hdr.ramdisk_start = (UINT32)(UINTN) Initrd;
> > + Bp->hdr.ramdisk_len = (UINT32) InitrdSize;
> > + Bp->hdr.ramdisk_max = Bp->hdr.ramdisk_start + Bp->hdr.ramdisk_len;
>
> 'ramdisk_max' is a read-only field. It tells you the maximum address at
> which you can allocate memory to hold the initrd. The kernel doesn't
> read it.
Got it.
> > +STATIC
> > +EFI_STATUS
> > +SetupLinuxBootParams (
> > + IN VOID *Kernel,
> > + IN OUT struct boot_params *Bp
> > + )
> > +{
> > + Bp->alt_mem_k = SIZE_32KB;
>
> This is a bit of legacy code and shouldn't be needed (it shouldn't be in
> efilinux either).
Will it possibly allow some older kernels to boot? Or, should I
just toss it out?
Thanks for the review Matt!
-Jordan
------------------------------------------------------------------------------
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
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
