From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.10116.1603371550165757605 for ; Thu, 22 Oct 2020 05:59:10 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 68B59101E; Thu, 22 Oct 2020 05:59:09 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 82F4B3F66E; Thu, 22 Oct 2020 05:59:07 -0700 (PDT) Subject: =?UTF-8?B?UmU6IOWbnuWkjTogW2VkazItZGV2ZWxdIFtQQVRDSCAxLzNdIE1kZU1vZHVsZVBrZy9BY3BpVGFibGVEeGU6IHVzZSBwb29sIGFsbG9jYXRpb25zIHdoZW4gcG9zc2libGU=?= To: gaoliming , 'Laszlo Ersek' , devel@edk2.groups.io Cc: 'Dandan Bi' , 'Jian J Wang' , 'Hao A Wu' , 'Sami Mujawar' , 'Leif Lindholm' References: <20201016154923.21260-1-ard.biesheuvel@arm.com> <20201016154923.21260-2-ard.biesheuvel@arm.com> <009401d6a817$53da54e0$fb8efea0$@byosoft.com.cn> From: "Ard Biesheuvel" Message-ID: Date: Thu, 22 Oct 2020 14:55:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <009401d6a817$53da54e0$fb8efea0$@byosoft.com.cn> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 10/22/20 4:01 AM, gaoliming wrote: > Ard: > Can you help to collect memmap information on Shell before this chan= ge and after this change? I want to know what=E2=80=99s impact on the mem= ory layout. >=20 Before: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Type Start End # Pages Available 0000000040000000-00000000403B2FFF 00000000000003B3 LoaderCode 00000000403B3000-000000004047FFFF 00000000000000CD ACPI_Recl 0000000040480000-00000000404DFFFF 0000000000000060 Available 00000000404E0000-00000000404FFFFF 0000000000000020 ACPI_Recl 0000000040500000-000000004051FFFF 0000000000000020 RT_Code 0000000040520000-000000004059FFFF 0000000000000080 RT_Data 00000000405A0000-000000004068FFFF 00000000000000F0 RT_Code 0000000040690000-000000004074FFFF 00000000000000C0 Available 0000000040750000-000000004188FFFF 0000000000001140 BS_Data 0000000041890000-00000000418ADFFF 000000000000001E Available 00000000418AE000-00000000418CBFFF 000000000000001E BS_Data 00000000418CC000-000000004299AFFF 00000000000010CF Available 000000004299B000-000000004299BFFF 0000000000000001 BS_Data 000000004299C000-0000000043643FFF 0000000000000CA8 Available 0000000043644000-000000004391AFFF 00000000000002D7 BS_Code 000000004391B000-0000000043C1FFFF 0000000000000305 RT_Code 0000000043C20000-0000000043DAFFFF 0000000000000190 RT_Data 0000000043DB0000-0000000043FFFFFF 0000000000000250 Available 0000000044000000-000000004401EFFF 000000000000001F BS_Data 000000004401F000-000000004401FFFF 0000000000000001 Available 0000000044020000-00000000477F7FFF 00000000000037D8 BS_Data 00000000477F8000-0000000047819FFF 0000000000000022 BS_Code 000000004781A000-000000004784FFFF 0000000000000036 BS_Data 0000000047850000-0000000047EEBFFF 000000000000069C BS_Code 0000000047EEC000-0000000047EEFFFF 0000000000000004 BS_Data 0000000047EF0000-0000000047FF0FFF 0000000000000101 BS_Code 0000000047FF1000-0000000047FFAFFF 000000000000000A BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 MMIO 0000000009010000-0000000009010FFF 0000000000000001 Reserved : 0 Pages (0 Bytes) LoaderCode: 205 Pages (839,680 Bytes) LoaderData: 0 Pages (0 Bytes) BS_Code : 841 Pages (3,444,736 Bytes) BS_Data : 9,561 Pages (39,161,856 Bytes) RT_Code : 720 Pages (2,949,120 Bytes) RT_Data : 832 Pages (3,407,872 Bytes) ACPI_Recl : 128 Pages (524,288 Bytes) ACPI_NVS : 0 Pages (0 Bytes) MMIO : 16,385 Pages (67,112,960 Bytes) MMIO_Port : 0 Pages (0 Bytes) PalCode : 0 Pages (0 Bytes) Available : 20,481 Pages (83,890,176 Bytes) Persistent: 0 Pages (0 Bytes) -------------- Total Memory: 128 MB (134,217,728 Bytes) After: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Type Start End # Pages Available 0000000040000000-0000000043EE1FFF 0000000000003EE2 LoaderCode 0000000043EE2000-0000000043FBFFFF 00000000000000DE RT_Code 0000000043FC0000-0000000043FFFFFF 0000000000000040 BS_Data 0000000044000000-000000004401FFFF 0000000000000020 Available 0000000044020000-000000004402FFFF 0000000000000010 ACPI_Recl 0000000044030000-000000004403FFFF 0000000000000010 RT_Code 0000000044040000-000000004407FFFF 0000000000000040 RT_Data 0000000044080000-000000004413FFFF 00000000000000C0 RT_Code 0000000044140000-000000004418FFFF 0000000000000050 RT_Data 0000000044190000-000000004422FFFF 00000000000000A0 RT_Code 0000000044230000-000000004431FFFF 00000000000000F0 Available 0000000044320000-0000000045481FFF 0000000000001162 BS_Data 0000000045482000-000000004549FFFF 000000000000001E Available 00000000454A0000-00000000454BDFFF 000000000000001E BS_Data 00000000454BE000-00000000454CAFFF 000000000000000D Available 00000000454CB000-00000000454CFFFF 0000000000000005 BS_Data 00000000454D0000-0000000047217FFF 0000000000001D48 Available 0000000047218000-00000000472AAFFF 0000000000000093 BS_Code 00000000472AB000-00000000475FFFFF 0000000000000355 RT_Code 0000000047600000-000000004768FFFF 0000000000000090 Available 0000000047690000-000000004769FFFF 0000000000000010 RT_Data 00000000476A0000-00000000477BFFFF 0000000000000120 Available 00000000477C0000-00000000477DEFFF 000000000000001F BS_Data 00000000477DF000-0000000047801FFF 0000000000000023 BS_Code 0000000047802000-0000000047848FFF 0000000000000047 BS_Data 0000000047849000-0000000047EDCFFF 0000000000000694 BS_Code 0000000047EDD000-0000000047EE4FFF 0000000000000008 BS_Data 0000000047EE5000-0000000047FE5FFF 0000000000000101 BS_Code 0000000047FE6000-0000000047FFAFFF 0000000000000015 BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 MMIO 0000000009010000-0000000009010FFF 0000000000000001 Reserved : 0 Pages (0 Bytes) LoaderCode: 222 Pages (909,312 Bytes) LoaderData: 0 Pages (0 Bytes) BS_Code : 953 Pages (3,903,488 Bytes) BS_Data : 9,552 Pages (39,124,992 Bytes) RT_Code : 592 Pages (2,424,832 Bytes) RT_Data : 640 Pages (2,621,440 Bytes) ACPI_Recl : 16 Pages (65,536 Bytes) ACPI_NVS : 0 Pages (0 Bytes) MMIO : 16,385 Pages (67,112,960 Bytes) MMIO_Port : 0 Pages (0 Bytes) PalCode : 0 Pages (0 Bytes) Available : 20,793 Pages (85,168,128 Bytes) Persistent: 0 Pages (0 Bytes) -------------- Total Memory: 128 MB (134,217,728 Bytes) > Liming >> -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- >> =E5=8F=91=E4=BB=B6=E4=BA=BA: Laszlo Ersek >> =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2020=E5=B9=B410=E6=9C=8820=E6=97= =A5 3:48 >> =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; ard.biesheuvel@arm.= com >> =E6=8A=84=E9=80=81: Dandan Bi ; Liming Gao >> ; Jian J Wang ; Hao A >> Wu ; Sami Mujawar ; Leif >> Lindholm >> =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTabl= eDxe: use pool >> allocations when possible >> >> On 10/16/20 17:49, Ard Biesheuvel wrote: >>> On AArch64 systems, page based allocations for memory types that are >>> relevant to the OS are rounded up to 64 KB multiples. This wastes >>> some space in the ACPI table memory allocator, since it uses page >>> based allocations in order to be able to place the ACPI tables low >>> in memory. >>> >>> Since the latter requirement does not exist on AArch64, switch to poo= l >>> allocations for all ACPI tables except the root tables if the active >>> allocation policy permits them to be anywhere in memory. The root >>> tables will be handled in a subsequent patch. >>> >>> Signed-off-by: Ard Biesheuvel >>> --- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 >> +- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c | >> 4 +- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 75 >> ++++++++++++++------ >>> 3 files changed, 57 insertions(+), 26 deletions(-) >>> >>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>> index 425a462fd92b..6e600e7acd24 100644 >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>> @@ -64,9 +64,9 @@ typedef struct { >>> LIST_ENTRY Link; >>> EFI_ACPI_TABLE_VERSION Version; >>> EFI_ACPI_COMMON_HEADER *Table; >>> - EFI_PHYSICAL_ADDRESS PageAddress; >>> - UINTN NumberOfPages; >>> + UINTN TableSize; >>> UINTN Handle; >>> + BOOLEAN PoolAllocation; >>> } EFI_ACPI_TABLE_LIST; >>> >>> // >> >> (1) The preceding comment (which explains the fields) should be update= d. >> >>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>> index b1cba20c8ed7..14ced68e645f 100644 >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>> @@ -68,8 +68,8 @@ FindTableByBuffer ( >>> >>> while (CurrentLink !=3D StartLink) { >>> CurrentTableList =3D EFI_ACPI_TABLE_LIST_FROM_LINK (CurrentLink= ); >>> - if (((UINTN)CurrentTableList->PageAddress <=3D (UINTN)Buffer) && >>> - ((UINTN)CurrentTableList->PageAddress + >> EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) { >>> + if (((UINTN)CurrentTableList->Table <=3D (UINTN)Buffer) && >>> + ((UINTN)CurrentTableList->Table + CurrentTableList->TableSiz= e > >> (UINTN)Buffer)) { >>> // >>> // Good! Found Table. >>> // >>> diff --git >> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>> index ad7baf8205b4..b05e57e6584f 100644 >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>> @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( >>> return EFI_SUCCESS; >>> } >>> >>> +/** >>> + Free the memory associated with provided the EFI_ACPI_TABLE_LIST >> instance >> >> (2) s/with provided the/with the provided/ >> >>> + >>> + @param TableEntry EFI_ACPI_TABLE_LIST >> instance pointer >>> + >>> +**/ >>> +STATIC >>> +VOID >>> +FreeTableMemory ( >>> + EFI_ACPI_TABLE_LIST *TableEntry >>> + ) >>> +{ >>> + if (TableEntry->PoolAllocation) { >>> + gBS->FreePool (TableEntry->Table); >>> + } else { >>> + gBS->FreePages >> ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, >>> + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); >>> + } >>> +} >>> + >>> /** >>> This function adds an ACPI table to the table list. It will dete= ct FACS >> and >>> allocate the correct type of memory and properly align the table. >>> @@ -454,14 +474,15 @@ AddTableToList ( >>> OUT UINTN *Handle >>> ) >>> { >>> - EFI_STATUS Status; >>> - EFI_ACPI_TABLE_LIST *CurrentTableList; >>> - UINT32 CurrentTableSignature; >>> - UINT32 CurrentTableSize; >>> - UINT32 *CurrentRsdtEntry; >>> - VOID *CurrentXsdtEntry; >>> - UINT64 Buffer64; >>> - BOOLEAN AddToRsdt; >>> + EFI_STATUS Status; >>> + EFI_ACPI_TABLE_LIST *CurrentTableList; >>> + UINT32 CurrentTableSignature; >>> + UINT32 CurrentTableSize; >>> + UINT32 *CurrentRsdtEntry; >>> + VOID *CurrentXsdtEntry; >>> + EFI_PHYSICAL_ADDRESS AllocPhysAddress; >>> + UINT64 Buffer64; >>> + BOOLEAN AddToRsdt; >>> >>> // >>> // Check for invalid input parameters >>> @@ -496,8 +517,8 @@ AddTableToList ( >>> // There is no architectural reason these should be below 4GB, it= is >> purely >>> // for convenience of implementation that we force memory below >> 4GB. >>> // >>> - CurrentTableList->PageAddress =3D 0xFFFFFFFF; >>> - CurrentTableList->NumberOfPages =3D EFI_SIZE_TO_PAGES >> (CurrentTableSize); >>> + AllocPhysAddress =3D 0xFFFFFFFF; >>> + CurrentTableList->TableSize =3D CurrentTableSize; >>> >>> // >>> // Allocation memory type depends on the type of the table >>> @@ -518,9 +539,22 @@ AddTableToList ( >>> Status =3D gBS->AllocatePages ( >>> AllocateMaxAddress, >>> EfiACPIMemoryNVS, >>> - CurrentTableList->NumberOfPages, >>> - &CurrentTableList->PageAddress >>> + EFI_SIZE_TO_PAGES >> (CurrentTableList->TableSize), >>> + &AllocPhysAddress >>> ); >>> + CurrentTableList->Table =3D (EFI_ACPI_COMMON_HEADER >> *)(UINTN)AllocPhysAddress; >>> + } else if (mAcpiTableAllocType =3D=3D AllocateAnyPages) { >>> + // >>> + // If there is no allocation limit, there is also no need to use= page >>> + // based allocations for ACPI tables, which may be wasteful on >> platforms >>> + // such as AArch64 that allocate multiples of 64 KB >>> + // >>> + Status =3D gBS->AllocatePool ( >>> + EfiACPIReclaimMemory, >>> + CurrentTableList->TableSize, >>> + (VOID **)&CurrentTableList->Table >>> + ); >>> + CurrentTableList->PoolAllocation =3D TRUE; >>> } else { >>> // >>> // All other tables are ACPI reclaim memory, no alignment >> requirements. >>> @@ -528,9 +562,10 @@ AddTableToList ( >>> Status =3D gBS->AllocatePages ( >>> mAcpiTableAllocType, >>> EfiACPIReclaimMemory, >>> - CurrentTableList->NumberOfPages, >>> - &CurrentTableList->PageAddress >>> + EFI_SIZE_TO_PAGES >> (CurrentTableList->TableSize), >>> + &AllocPhysAddress >>> ); >>> + CurrentTableList->Table =3D (EFI_ACPI_COMMON_HEADER >> *)(UINTN)AllocPhysAddress; >>> } >>> // >>> // Check return value from memory alloc. >>> @@ -539,10 +574,6 @@ AddTableToList ( >>> gBS->FreePool (CurrentTableList); >>> return EFI_OUT_OF_RESOURCES; >>> } >>> - // >>> - // Update the table pointer with the allocated memory start >>> - // >>> - CurrentTableList->Table =3D (EFI_ACPI_COMMON_HEADER *) (UINTN) >> CurrentTableList->PageAddress; >>> >>> // >>> // Initialize the table contents >> >> (3) Thus far, in the changes to AddTableToList(), two things bother me= : >> >> (3a) Before the patch, we'd only convert the physical address to a >> pointer once we knew that the allocation succeeded. Post-patch, we now >> have two branches where we might convert bogus AllocPhysAddress values >> (such as even 0xFFFFFFFF) to (EFI_ACPI_COMMON_HEADER*). I think we >> shouldn't do this. (It's not just the dereferencing of bogus pointers >> that we should avoid, but also the evaluation thereof.) >> >> (3b) "CurrentTableList" is allocated with AllocatePool() not >> AllocateZeroPool(), so *not* setting the "PoolAllocation" field to FAL= SE >> on the two affected branches is wrong. >> >> The patch looks OK to me otherwise. >> >> Thanks >> Laszlo >> >>> @@ -575,7 +606,7 @@ AddTableToList ( >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) !=3D 0 && >> AcpiTableInstance->Fadt1 !=3D NULL) || >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) !=3D 0 && >> AcpiTableInstance->Fadt3 !=3D NULL) >>> ) { >>> - gBS->FreePages (CurrentTableList->PageAddress, >> CurrentTableList->NumberOfPages); >>> + FreeTableMemory (CurrentTableList); >>> gBS->FreePool (CurrentTableList); >>> return EFI_ACCESS_DENIED; >>> } >>> @@ -729,7 +760,7 @@ AddTableToList ( >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) !=3D 0 && >> AcpiTableInstance->Facs1 !=3D NULL) || >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) !=3D 0 && >> AcpiTableInstance->Facs3 !=3D NULL) >>> ) { >>> - gBS->FreePages (CurrentTableList->PageAddress, >> CurrentTableList->NumberOfPages); >>> + FreeTableMemory (CurrentTableList); >>> gBS->FreePool (CurrentTableList); >>> return EFI_ACCESS_DENIED; >>> } >>> @@ -813,7 +844,7 @@ AddTableToList ( >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) !=3D 0 && >> AcpiTableInstance->Dsdt1 !=3D NULL) || >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) !=3D 0 && >> AcpiTableInstance->Dsdt3 !=3D NULL) >>> ) { >>> - gBS->FreePages (CurrentTableList->PageAddress, >> CurrentTableList->NumberOfPages); >>> + FreeTableMemory (CurrentTableList); >>> gBS->FreePool (CurrentTableList); >>> return EFI_ACCESS_DENIED; >>> } >>> @@ -1449,7 +1480,7 @@ DeleteTable ( >>> // >>> // Free the Table >>> // >>> - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); >>> + FreeTableMemory (Table); >>> RemoveEntryList (&(Table->Link)); >>> gBS->FreePool (Table); >>> } >>> >=20 >=20 >=20