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.web12.4136.1603332126469263770 for ; Wed, 21 Oct 2020 19:02:08 -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 ; Thu, 22 Oct 2020 10:01:57 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: "'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> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIDEvM10gTWRlTW9kdWxlUGtnL0FjcGlUYWJsZUR4ZTogdXNlIHBvb2wgYWxsb2NhdGlvbnMgd2hlbiBwb3NzaWJsZQ==?= Date: Thu, 22 Oct 2020 10:01:59 +0800 Message-ID: <009401d6a817$53da54e0$fb8efea0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQG1P16Qtp8rtrRUOGS/yc4Px7RstgGV4cMRAVTJxfOpzlpFgA== Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn 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 Thanks 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 >=20 > 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; > > > > // >=20 > (1) The preceding comment (which explains the fields) should be = updated. >=20 > > 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 >=20 > (2) s/with provided the/with the provided/ >=20 > > + > > + @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 >=20 > (3) Thus far, in the changes to AddTableToList(), two things bother = me: >=20 > (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.) >=20 > (3b) "CurrentTableList" is allocated with AllocatePool() not > AllocateZeroPool(), so *not* setting the "PoolAllocation" field to = FALSE > on the two affected branches is wrong. >=20 > The patch looks OK to me otherwise. >=20 > Thanks > Laszlo >=20 > > @@ -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); > > } > >