From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.4786.1603137981759261072 for ; Mon, 19 Oct 2020 13:06:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=g9cQge83; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1603137980; 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=3oMDlNdLQKm+IHkbHsDpf6SgJP3xxUWdCvj+2hq0kNg=; b=g9cQge83y24f2h+ROsS3mlorxZ1XdoXWFiQwsXHcCuTjSmCefyitJUO+CQa9qKqPEzFyMF wI8+ihC8B0G4oxghvCxmy2NEFs281shtDIm87E/aHXW+xJO31PG49JFv4GP/xN/H04+3qv EE8np17ysw0y2Qzuxir56KxLB4cGK8k= 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-316-zwsStemANnuFbcq6A804pg-1; Mon, 19 Oct 2020 16:06:17 -0400 X-MC-Unique: zwsStemANnuFbcq6A804pg-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 892BE1074654; Mon, 19 Oct 2020 20:06:15 +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 750A419D7C; Mon, 19 Oct 2020 20:06:13 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible 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> From: "Laszlo Ersek" Message-ID: <11f117f4-7c40-d9ce-8ffb-804f01ac9e00@redhat.com> Date: Mon, 19 Oct 2020 22:06:12 +0200 MIME-Version: 1.0 In-Reply-To: <20201016154923.21260-3-ard.biesheuvel@arm.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/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. 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; >