From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web10.7208.1603676123886831050 for ; Sun, 25 Oct 2020 18:35:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Mon, 26 Oct 2020 09:35:09 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: "'Ard Biesheuvel'" , "'Laszlo Ersek'" , 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> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IFtlZGsyLWRldmVsXSBbUEFUQ0ggMS8zXSBNZGVNb2R1bGVQa2cvQWNwaVRhYmxlRHhlOiB1c2UgcG9vbCBhbGxvY2F0aW9ucyB3aGVuIHBvc3NpYmxl?= Date: Mon, 26 Oct 2020 09:35:15 +0800 Message-ID: <003401d6ab38$411f31d0$c35d9570$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQG1P16Qtp8rtrRUOGS/yc4Px7RstgGV4cMRAVTJxfMC44WBzgL3Na87qaXD6IA= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Ard: I verify this patch on OvmfX64 and collect the memmap info. I don't = see the difference in memmap.=20 So, this enhancement is for AARCH64 only. Is it right? Thanks Liming > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: Ard Biesheuvel > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: = 2020=E5=B9=B410=E6=9C=8822=E6=97=A5 20:56 > =E6=94=B6=E4=BB=B6=E4=BA=BA: gaoliming ; = 'Laszlo Ersek' > ; devel@edk2.groups.io > =E6=8A=84=E9=80=81: 'Dandan Bi' ; 'Jian J Wang' > ; 'Hao A Wu' ; 'Sami = Mujawar' > ; 'Leif Lindholm' > =E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: [edk2-devel] [PATCH 1/3] = MdeModulePkg/AcpiTableDxe: use > pool allocations when possible >=20 > On 10/22/20 4:01 AM, gaoliming wrote: > > Ard: > > Can you help to collect memmap information on Shell before this > change and after this change? I want to know what=E2=80=99s impact on = the memory > layout. > > >=20 >=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 >=20 > 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) >=20 >=20 > 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 >=20 > 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) >=20 >=20 > > 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/AcpiTableDxe: 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 = pool > >>> 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 = updated. > >> > >>> 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->TableSize > > >> (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 = detect > 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 = FALSE > >> 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); > >>> } > >>> > > > > > >