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.5077.1610645756649739652 for ; Thu, 14 Jan 2021 09:35:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VIVRYjox; 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=1610645755; 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=P0p7Twr60MxY+ScUP9AVAtYS4fvWk6SApaHO5fjkzX8=; b=VIVRYjoxm4S0pTb910uMygeqqjlm6McVyfvUFxkyzj7k7ksLbDZomtgFJEeei51WrP2E9/ BIwl364h8TmCyG1+JK5mjVwzDc3QRpN+sZtg31XRfl+nZGIypTB0N3rGcPF8IcZ9nxZQ3y 4SWtVNYRi14MqwcfKMldFaFwfMRVPRo= 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-361-VYGxNkIPPXeGAc2jDWYOHg-1; Thu, 14 Jan 2021 12:35:52 -0500 X-MC-Unique: VYGxNkIPPXeGAc2jDWYOHg-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 A6D1A107ACF9; Thu, 14 Jan 2021 17:35:50 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-194.ams2.redhat.com [10.36.112.194]) by smtp.corp.redhat.com (Postfix) with ESMTP id EC05360C47; Thu, 14 Jan 2021 17:35:48 +0000 (UTC) Subject: Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables To: "Zeng, Star" , "Ni, Ray" , "devel@edk2.groups.io" 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> From: "Laszlo Ersek" Message-ID: <6b798462-be3b-ad4a-fc84-eb679d28b601@redhat.com> Date: Thu, 14 Jan 2021 18:35:47 +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 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 >>>> >