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.web09.1411.1610699839444161350 for ; Fri, 15 Jan 2021 00:37:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dA/jHbI4; 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=1610699838; 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=yYhfS+ql5mcpA/ss8O36irxfsGvGluNFU4EJF/TiZ40=; b=dA/jHbI4jE0hMirC8k2WJnsdcvnbYB96yFwNbwEPHjTa/h55hecstO46c5r9UFubWe4G82 LEW/QerESkT5z37udmbKoUspJ/l+nQDnRI0KEY4cGCbdOgwjdlIAPrhwSqtj6OdbSdXGFd uPsqOeu5kLWRD5aSDKiRcsKPdMi+dAA= 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-241-BzCTla17NK6RgeSzwGaW7A-1; Fri, 15 Jan 2021 03:37:14 -0500 X-MC-Unique: BzCTla17NK6RgeSzwGaW7A-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7CF85835DE0; Fri, 15 Jan 2021 08:37:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-223.ams2.redhat.com [10.36.112.223]) by smtp.corp.redhat.com (Postfix) with ESMTP id BCA9A60BF3; Fri, 15 Jan 2021 08:37:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables To: "Ni, Ray" , "devel@edk2.groups.io" , "Zeng, Star" Cc: "Dong, Eric" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , "Kumar, Rahul1" References: <20210112191934.12459-1-lersek@redhat.com> <20210112191934.12459-3-lersek@redhat.com> <6b798462-be3b-ad4a-fc84-eb679d28b601@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 15 Jan 2021 09:37:10 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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/15/21 09:33, Ni, Ray wrote: > Laszlo, > I will test my patches internally and find a way to give you the patch to be included in your V2. Thank you! Laszlo > > Thanks, > Ray > >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Laszlo Ersek >> Sent: Friday, January 15, 2021 1:36 AM >> To: Zeng, Star ; Ni, Ray ; devel@edk2.groups.io >> Cc: Dong, Eric ; Philippe Mathieu-Daudé ; Kumar, Rahul1 >> >> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables >> >> Hi Star, >> >> On 01/14/21 02:55, Zeng, Star wrote: >>> Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the >> function's second call. >> >> thank you for following up here -- could you or Ray please propose an >> actual patch for RegisterCpuFeaturesLib, as I requested before? >> >> Posting the patch is not necessary; I only need to fetch it from your >> personal repo(s) -- you can even send me the repo / branch reference >> off-list. I would include the RegisterCpuFeaturesLib patch in my v2 >> posting of this series. >> >> Thanks! >> Laszlo >> >>> >>>> -----Original Message----- >>>> From: Ni, Ray >>>> Sent: Wednesday, January 13, 2021 4:12 PM >>>> To: Zeng, Star ; Laszlo Ersek ; >>>> devel@edk2.groups.io >>>> Cc: Dong, Eric ; Philippe Mathieu-Daudé >>>> ; Kumar, Rahul1 >>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless >>>> register tables >>>> >>>> Star, >>>> Thanks for pointing that. >>>> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is >>>> allocated but CpuS3Data >>>> doesn't do that. >>>> >>>> Can following change fix the problem? >>>> >>>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c >>>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c >>>> @@ -937,21 +937,19 @@ GetAcpiCpuData ( >>>> EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer; >>>> >>>> AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 >>>> (PcdCpuS3DataAddress); >>>> - if (AcpiCpuData != NULL) { >>>> - return AcpiCpuData; >>>> - } >>>> - >>>> - AcpiCpuData = AllocatePages (EFI_SIZE_TO_PAGES (sizeof >>>> (ACPI_CPU_DATA))); >>>> - ASSERT (AcpiCpuData != NULL); >>>> + if (AcpiCpuData == NULL) { >>>> + AcpiCpuData = AllocatePages (EFI_SIZE_TO_PAGES (sizeof >>>> (ACPI_CPU_DATA))); >>>> + ASSERT (AcpiCpuData != NULL); >>>> >>>> - // >>>> - // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA >>>> structure >>>> - // >>>> - Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData); >>>> - ASSERT_EFI_ERROR (Status); >>>> + // >>>> + // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA >>>> structure >>>> + // >>>> + Status = PcdSet64S (PcdCpuS3DataAddress, >>>> (UINT64)(UINTN)AcpiCpuData); >>>> + ASSERT_EFI_ERROR (Status); >>>> >>>> - GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors); >>>> - AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus; >>>> + GetNumberOfProcessor (&NumberOfCpus, >>>> &NumberOfEnabledProcessors); >>>> + AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus; >>>> + } >>>> >>> >>> Add check as below here. >>> >>> if (AcpiCpuData->RegisterTable == 0) { >>> >>>> // >>>> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable >>>> for all CPUs >>>> >>>> Thanks, >>>> Ray >>>> >>>>> -----Original Message----- >>>>> From: Zeng, Star >>>>> Sent: Wednesday, January 13, 2021 3:17 PM >>>>> To: Ni, Ray ; Laszlo Ersek ; >>>>> devel@edk2.groups.io >>>>> Cc: Dong, Eric ; Philippe Mathieu-Daudé >>>>> ; Kumar, Rahul1 ; Zeng, >>>> Star >>>>> >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate >>>> useless >>>>> register tables >>>>> >>>>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency >>>> to >>>>> UefiCpuPkg CpuS3DataDxe. >>>>> There is ASSERT to happen at >>>>> >>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist >>>> erC >>>>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with >>>> this >>>>> patch runs before DxeRegisterCpuFeaturesLib. >>>>> >>>>> Thanks, >>>>> Star >>>>> -----Original Message----- >>>>> From: Ni, Ray >>>>> Sent: Wednesday, January 13, 2021 2:12 PM >>>>> To: Laszlo Ersek ; devel@edk2.groups.io >>>>> Cc: Dong, Eric ; Philippe Mathieu-Daudé >>>>> ; Kumar, Rahul1 ; Zeng, >>>> Star >>>>> >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate >>>> useless >>>>> register tables >>>>> >>>>> 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.) >>>>> >>>>> Will you create a pull request to merge all 3 patches together? >>>>> >>>>>> -----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 >>>>>> >>> >> >> >> >> >> >