comments in-line
On 10/26/12 19:35, Jordan Justen wrote:
> +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;
> + struct e820_entry *E820;
> + UINTN E820EntryCount;
> + EFI_PHYSICAL_ADDRESS LastEndAddr;
> +
> + //
> + // Get System MemoryMapSize
> + //
> + MemoryMapSize = sizeof (TmpMemoryMap);
> + Status = gBS->GetMemoryMap (
> + &MemoryMapSize,
> + (EFI_MEMORY_DESCRIPTOR *)TmpMemoryMap,
> + &MapKey,
> + &DescriptorSize,
> + &DescriptorVersion
> + );
> + ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> + //
> + // Enlarge space here, because we will allocate pool now.
> + //
> + MemoryMapSize += EFI_PAGE_SIZE;
> + MemoryMap = AllocatePool (MemoryMapSize);
> + ASSERT (MemoryMap != NULL);
> +
> + //
> + // Get System MemoryMap
> + //
> + Status = gBS->GetMemoryMap (
> + &MemoryMapSize,
> + MemoryMap,
> + &MapKey,
> + &DescriptorSize,
> + &DescriptorVersion
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + LastE820 = &Bp->e820_map[0];
LastE820 here should be set to NULL...
> + E820 = &Bp->e820_map[0];
> + E820EntryCount = 0;
> + LastEndAddr = 0;
> + MemoryMapPtr = MemoryMap;
> + for (Index = 0; Index < (MemoryMapSize / DescriptorSize); Index++) {
> + UINTN E820Type = 0;
> +
> + if (MemoryMap->NumberOfPages == 0) {
> + continue;
> + }
> +
> + switch(MemoryMap->Type) {
> + case EfiReservedMemoryType:
> + case EfiRuntimeServicesCode:
> + case EfiRuntimeServicesData:
> + case EfiMemoryMappedIO:
> + case EfiMemoryMappedIOPortSpace:
> + case EfiPalCode:
> + E820Type = E820_RESERVED;
> + break;
> +
> + case EfiUnusableMemory:
> + E820Type = E820_UNUSABLE;
> + break;
> +
> + case EfiACPIReclaimMemory:
> + E820Type = E820_ACPI;
> + break;
> +
> + case EfiLoaderCode:
> + case EfiLoaderData:
> + case EfiBootServicesCode:
> + case EfiBootServicesData:
> + case EfiConventionalMemory:
> + E820Type = E820_RAM;
> + break;
> +
> + case EfiACPIMemoryNVS:
> + E820Type = E820_NVS;
> + break;
> +
> + default:
> + DEBUG ((
> + EFI_D_ERROR,
> + "Invalid EFI memory descriptor type (0x%x)!\n",
> + MemoryMap->Type
> + ));
> + continue;
> + }
> +
> + if ((LastE820 != NULL) &&
> + (LastE820->type == (UINT32) E820Type) &&
... because here "LastE820->type" should not even be investigated unless
we have added at least one entry. (Currently, the contents of *LastE820
on the first occasion we reach this code is indeterminate, it is
something we copied from the host into the kernel setup buffer via fw_cfg.)
(I think Matt mentioned something like this last time -- apologies for
not reviewing this function more thoroughly in the v2 posting!)
> + (MemoryMap->PhysicalStart == LastEndAddr)) {
> + LastE820->size += EFI_PAGES_TO_SIZE (MemoryMap->NumberOfPages);
In this block "LastEndAddr" should be increased too, by the same number
of bytes.
> + } 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;
> +
> + Efi = &Bp->efi_info;
> + Efi->efi_systab = (UINT32)(UINTN) gST;
> + Efi->efi_memdesc_size = (UINT32) DescriptorSize;
> + Efi->efi_memdesc_version = DescriptorVersion;
> + Efi->efi_memmap = (UINT32)(UINTN) MemoryMapPtr;
> + Efi->efi_memmap_size = (UINT32) MemoryMapSize;
> +#ifdef MDE_CPU_IA32
> + Efi->efi_loader_signature = SIGNATURE_32 ('E', 'L', '3', '2');
> +#else
> + Efi->efi_systab_hi = ((UINT64)(UINTN) gST) >> 32;
> + Efi->efi_memmap_hi = ((UINT64)(UINTN) MemoryMapPtr) >> 32;
> + Efi->efi_loader_signature = SIGNATURE_32 ('E', 'L', '6', '4');
> +#endif
> +
> + gBS->ExitBootServices (gImageHandle, MapKey);
> +}
No more comments.
*If* Matt finds no problems, I propose to commit this series, and fix
the above in a small follow-up patch. On that condition:
Reviewed-by: Laszlo Ersek <[email protected]>
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
