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.web12.5579.1610529403200923585 for ; Wed, 13 Jan 2021 01:16:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LO7HySVP; 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=1610529402; 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=6Xw8NTehUmn1ubPI+V2LzKWsxNQWzVg+iqKhTYbdlWU=; b=LO7HySVP80rS9KkDaSDoJRRGHy4qnQnsYcseD5FsdvWhXB8iDoY7QWDdZP1VWANostuJq2 TIyEkTVk8dDRmr/Sp9nnxWiQG6B+LA3dGNdtG8pvCW8NvY2Q8D7p8icXVWSG+cw50NH2nZ Yv4XUpsplyWIDKA6hRQgdPUAuuze5X4= 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-42-qgE1GRrzNJSJ_LzEsOccHA-1; Wed, 13 Jan 2021 04:16:40 -0500 X-MC-Unique: qgE1GRrzNJSJ_LzEsOccHA-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 B8E138144E0; Wed, 13 Jan 2021 09:16:38 +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 3AE5160C0F; Wed, 13 Jan 2021 09:16:37 +0000 (UTC) Subject: Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables To: "Ni, Ray" , "Zeng, Star" , "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: <65d582e9-95cf-7ceb-e379-f1df68a57478@redhat.com> Date: Wed, 13 Jan 2021 10:16:36 +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/13/21 09:12, Ni, Ray wrote: > Star, > Thanks for pointing that. > RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is allocated but CpuS3Data > doesn't do that. Sorry that I missed this; I did grep the tree for [PreSmmInit]RegisterTable, but apparently didn't pay enough attention to RegisterCpuFeaturesLib. OVMF does not use that library. Ray: can you please post your fix as a standalone patch? Or else, if it's more convenient, please just push the fix somewhere (as a standalone patch) where I can fetch it from. It would be great if you could write a commit message too. Then, I will post a v2 of this series, including your fix for RegisterCpuFeaturesLib (as a patch under your authorship). Star and Eric can then review your patch on the list -- neither I nor you can review your patch, as you are the author of it, and I'll be posting it. Thanks! Laszlo > > 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; > + } > > // > // 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/RegisterC >> 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 >>> >