From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.4944.1603138279782675117 for ; Mon, 19 Oct 2020 13:11:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=D4Skdjkc; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1603138278; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=df2rlPPLrbU12WFD3XJ45HilsH+8FxDNKsMtm9l0hb4=; b=D4SkdjkcWp6vjwNN5kSY85GpTmgKmwTVbeqlHjKjVb+4C/Wj5QmyPrpFmpyDiWWkTPFT9T J+FYyurzyM2L33FduRBnlCDR3tJRxxenjRWtytQ9q8dQe9F1PYFvTVFOvK3T74NLP7Jw8q PB7tBCld/E/Exu/GCPIpCZbXoW/alKQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-503-vT9DUys-MmCpSbrpCBNMcg-1; Mon, 19 Oct 2020 16:11:13 -0400 X-MC-Unique: vT9DUys-MmCpSbrpCBNMcg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 88EF010866A2; Mon, 19 Oct 2020 20:11:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-137.ams2.redhat.com [10.36.113.137]) by smtp.corp.redhat.com (Postfix) with ESMTP id 58E4C27CCC; Mon, 19 Oct 2020 20:11:09 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible From: "Laszlo Ersek" To: devel@edk2.groups.io, ard.biesheuvel@arm.com Cc: Dandan Bi , Liming Gao , Jian J Wang , Hao A Wu , Sami Mujawar , Leif Lindholm References: <20201016154923.21260-1-ard.biesheuvel@arm.com> <20201016154923.21260-3-ard.biesheuvel@arm.com> <11f117f4-7c40-d9ce-8ffb-804f01ac9e00@redhat.com> Message-ID: <06012558-1147-c2ed-fd5c-b4d4b788f982@redhat.com> Date: Mon, 19 Oct 2020 22:11:08 +0200 MIME-Version: 1.0 In-Reply-To: <11f117f4-7c40-d9ce-8ffb-804f01ac9e00@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/19/20 22:06, Laszlo Ersek wrote: > On 10/16/20 17:49, Ard Biesheuvel wrote: >> If no memory allocation limit is in effect for ACPI tables, prefer >> pool allocations over page allocations, to avoid wasting memory on >> systems where page based allocations are rounded up to 64 KB, such >> as AArch64. >> >> Signed-off-by: Ard Biesheuvel >> --- >> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 111 ++++++++++++-------- >> 1 file changed, 65 insertions(+), 46 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> index b05e57e6584f..22f49a8489e7 100644 >> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> @@ -355,28 +355,35 @@ ReallocateAcpiTableBuffer ( >> NewMaxTableNumber * sizeof (UINT32); >> } >> >> - // >> - // Allocate memory in the lower 32 bit of address range for >> - // compatibility with ACPI 1.0 OS. >> - // >> - // This is done because ACPI 1.0 pointers are 32 bit values. >> - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> - // There is no architectural reason these should be below 4GB, it is purely >> - // for convenience of implementation that we force memory below 4GB. >> - // >> - PageAddress = 0xFFFFFFFF; >> - Status = gBS->AllocatePages ( >> - mAcpiTableAllocType, >> - EfiACPIReclaimMemory, >> - EFI_SIZE_TO_PAGES (TotalSize), >> - &PageAddress >> - ); >> + if (mAcpiTableAllocType != AllocateAnyPages) { >> + // >> + // Allocate memory in the lower 32 bit of address range for >> + // compatibility with ACPI 1.0 OS. >> + // >> + // This is done because ACPI 1.0 pointers are 32 bit values. >> + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> + // There is no architectural reason these should be below 4GB, it is purely >> + // for convenience of implementation that we force memory below 4GB. > > Hmmm. I first thought that the last two lines of the commend should not > be re-instated. But I was wrong. Because supporting ACPI 1.0b causes us > to allocate even areas pointed-to by XSDT entries under 4GB. > >> + // >> + PageAddress = 0xFFFFFFFF; >> + Status = gBS->AllocatePages ( >> + mAcpiTableAllocType, >> + EfiACPIReclaimMemory, >> + EFI_SIZE_TO_PAGES (TotalSize), >> + &PageAddress >> + ); >> + Pointer = (UINT8 *)(UINTN)PageAddress; > > (1) Same comment as before; we shouldn't convert PageAddress to a > pointer unless the allocation succeeds. > >> + } else { >> + Status = gBS->AllocatePool ( >> + EfiACPIReclaimMemory, >> + TotalSize, >> + (VOID **)&Pointer); >> + } >> >> if (EFI_ERROR (Status)) { >> return EFI_OUT_OF_RESOURCES; >> } >> >> - Pointer = (UINT8 *) (UINTN) PageAddress; >> ZeroMem (Pointer, TotalSize); >> >> AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; >> @@ -406,21 +413,26 @@ ReallocateAcpiTableBuffer ( >> } >> CopyMem (AcpiTableInstance->Xsdt, TempPrivateData.Xsdt, (sizeof (EFI_ACPI_DESCRIPTION_HEADER) + mEfiAcpiMaxNumTables * sizeof (UINT64))); >> >> - // >> - // Calculate orignal ACPI table buffer size >> - // >> - TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT >> - mEfiAcpiMaxNumTables * sizeof (UINT64); >> + if (mAcpiTableAllocType != AllocateAnyPages) { >> + // >> + // Calculate orignal ACPI table buffer size >> + // >> + TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT >> + mEfiAcpiMaxNumTables * sizeof (UINT64); >> >> - if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { >> - TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT >> - mEfiAcpiMaxNumTables * sizeof (UINT32) + >> - sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT >> - mEfiAcpiMaxNumTables * sizeof (UINT32); >> + if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { >> + TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT >> + mEfiAcpiMaxNumTables * sizeof (UINT32) + >> + sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT >> + mEfiAcpiMaxNumTables * sizeof (UINT32); >> + } >> + >> + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, >> + EFI_SIZE_TO_PAGES (TotalSize)); >> + } else { >> + gBS->FreePool (TempPrivateData.Rsdt1); >> } >> >> - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, EFI_SIZE_TO_PAGES (TotalSize)); >> - >> // >> // Update the Max ACPI table number >> // >> @@ -1759,29 +1771,36 @@ AcpiTableAcpiTableConstructor ( >> mEfiAcpiMaxNumTables * sizeof (UINT32); >> } >> >> - // >> - // Allocate memory in the lower 32 bit of address range for >> - // compatibility with ACPI 1.0 OS. >> - // >> - // This is done because ACPI 1.0 pointers are 32 bit values. >> - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> - // There is no architectural reason these should be below 4GB, it is purely >> - // for convenience of implementation that we force memory below 4GB. >> - // >> - PageAddress = 0xFFFFFFFF; >> - Status = gBS->AllocatePages ( >> - mAcpiTableAllocType, >> - EfiACPIReclaimMemory, >> - EFI_SIZE_TO_PAGES (TotalSize), >> - &PageAddress >> - ); >> + if (mAcpiTableAllocType != AllocateAnyPages) { >> + // >> + // Allocate memory in the lower 32 bit of address range for >> + // compatibility with ACPI 1.0 OS. >> + // >> + // This is done because ACPI 1.0 pointers are 32 bit values. >> + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> + // There is no architectural reason these should be below 4GB, it is purely >> + // for convenience of implementation that we force memory below 4GB. >> + // >> + PageAddress = 0xFFFFFFFF; >> + Status = gBS->AllocatePages ( >> + mAcpiTableAllocType, >> + EfiACPIReclaimMemory, >> + EFI_SIZE_TO_PAGES (TotalSize), >> + &PageAddress >> + ); >> + Pointer = (UINT8 *)(UINTN)PageAddress; > > (2) Same as (1). > > Looks reasonable otherwise. (3) Style: for both gBS->AllocatePool() calls added in this patch, please either break the closing paren to a new line, or use the "condensed" style (which you do use elsewhere in these patches). Thanks! Laszlo > >> + } else { >> + Status = gBS->AllocatePool ( >> + EfiACPIReclaimMemory, >> + TotalSize, >> + (VOID **)&Pointer); >> + } >> >> if (EFI_ERROR (Status)) { >> gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize)); >> return EFI_OUT_OF_RESOURCES; >> } >> >> - Pointer = (UINT8 *) (UINTN) PageAddress; >> ZeroMem (Pointer, TotalSize); >> >> AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; >> >