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.web10.5547.1610529041845148049 for ; Wed, 13 Jan 2021 01:10:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KcZ6awGb; 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=1610529041; 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=K5CZftjlOsll0JuJIPf9uuswlUloIjwm7wus0pM/3hI=; b=KcZ6awGbEBg7fUXW4c2Fv54PDkpDNkYGOTlS3naaqcwq+kaIsQObUpLyt6NdPKaTFW9Enm p87WBk6/x4kSeNC3JqqaFbxJE90f0z+7nYu5oIEvm6+ieLF5rk4+dRWVOuzoqU456+fQmC csZzr8ExS+EAckM054pWHpcP1sgQtzQ= 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-149-zdXgzs6XP7uv_zX-oIw2cg-1; Wed, 13 Jan 2021 04:10:37 -0500 X-MC-Unique: zdXgzs6XP7uv_zX-oIw2cg-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 47D66879516; Wed, 13 Jan 2021 09:10:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-238.ams2.redhat.com [10.36.112.238]) by smtp.corp.redhat.com (Postfix) with ESMTP id 98FC61F042; Wed, 13 Jan 2021 09:10:34 +0000 (UTC) Subject: Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables To: "Ni, Ray" , "devel@edk2.groups.io" Cc: "Dong, Eric" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , "Kumar, Rahul1" , "Zeng, Star" References: <20210112191934.12459-1-lersek@redhat.com> <20210112191934.12459-3-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: <34e85d22-6a69-2a1e-b4ab-56504401ac00@redhat.com> Date: Wed, 13 Jan 2021 10:10:33 +0100 MIME-Version: 1.0 In-Reply-To: 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: 8bit On 01/13/21 07:12, Ni, Ray wrote: > Reviewed-by: Ray Ni > > (I guess you don't want to put Redhat copyright in the patch 1&2 so the copyright year is not updated. > Since it's a simple change that make the logic more neat, I am ok with that.) I don't care about updating existent (C) notices; the git history is there for that. (Unless the package maintainer insists, of course.) At Red Hat, we add copyright notices when we create new files. > Will you create a pull request to merge all 3 patches together? Sure, I'll be happy to. Thanks! Laszlo > >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Wednesday, January 13, 2021 3:20 AM >> To: devel@edk2.groups.io >> Cc: Dong, Eric ; Philippe Mathieu-Daudé >> ; Kumar, Rahul1 ; Ni, Ray >> ; Zeng, Star >> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless >> register tables >> >> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable" >> arrays in ACPI_CPU_DATA just so every processor in the system can have its >> own empty register table, matched by APIC ID. This has never been useful >> in practice. >> >> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce >> SMRAM >> consumption in CpuS3.c", 2021-01-11), simply leave both >> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable" >> initialized to the zero address. This simplifies the driver, and saves >> both normal RAM (boot services data type memory) and -- in >> PiSmmCpuDxeSmm >> -- SMRAM. >> >> Cc: Eric Dong >> Cc: Philippe Mathieu-Daudé >> Cc: Rahul Kumar >> Cc: Ray Ni >> Cc: Star Zeng >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159 >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the OVMF >> IA32 >> and IA32X64 platforms with this driver -- this driver works OK in OVMF >> as long as no CPUs are hot-plugged. >> >> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 -------------------- >> 1 file changed, 32 deletions(-) >> >> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c >> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c >> index 2be335d91903..078af36cfb41 100644 >> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c >> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c >> @@ -165,10 +165,6 @@ CpuS3DataInitialize ( >> UINTN NumberOfCpus; >> UINTN NumberOfEnabledProcessors; >> VOID *Stack; >> - UINTN TableSize; >> - CPU_REGISTER_TABLE *RegisterTable; >> - UINTN Index; >> - EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer; >> UINTN GdtSize; >> UINTN IdtSize; >> VOID *Gdt; >> @@ -255,34 +251,6 @@ CpuS3DataInitialize ( >> AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData- >>> PreSmmInitRegisterTable; >> AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation; >> CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof >> (CPU_STATUS_INFORMATION)); >> - } else { >> - // >> - // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for >> all CPUs >> - // >> - TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); >> - RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize); >> - ASSERT (RegisterTable != NULL); >> - >> - for (Index = 0; Index < NumberOfCpus; Index++) { >> - Status = MpServices->GetProcessorInfo ( >> - MpServices, >> - Index, >> - &ProcessorInfoBuffer >> - ); >> - ASSERT_EFI_ERROR (Status); >> - >> - RegisterTable[Index].InitialApicId = >> (UINT32)ProcessorInfoBuffer.ProcessorId; >> - RegisterTable[Index].TableLength = 0; >> - RegisterTable[Index].AllocatedSize = 0; >> - RegisterTable[Index].RegisterTableEntry = 0; >> - >> - RegisterTable[NumberOfCpus + Index].InitialApicId = >> (UINT32)ProcessorInfoBuffer.ProcessorId; >> - RegisterTable[NumberOfCpus + Index].TableLength = 0; >> - RegisterTable[NumberOfCpus + Index].AllocatedSize = 0; >> - RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0; >> - } >> - AcpiCpuData->RegisterTable = >> (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable; >> - AcpiCpuData->PreSmmInitRegisterTable = >> (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus); >> } >> >> // >> -- >> 2.19.1.3.g30247aa5d201 >> >