From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B518721A07A92 for ; Thu, 18 Oct 2018 09:46:42 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1F4C188E61; Thu, 18 Oct 2018 16:46:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-50.rdu2.redhat.com [10.10.121.50]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0EC551057051; Thu, 18 Oct 2018 16:46:40 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20181018073448.11496-1-eric.dong@intel.com> <20181018073448.11496-6-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <49d692ed-35be-edb8-214c-20f131cf51b7@redhat.com> Date: Thu, 18 Oct 2018 18:46:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181018073448.11496-6-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 18 Oct 2018 16:46:42 +0000 (UTC) Subject: Re: [Patch v3 5/6] UefiCpuPkg/CpuS3DataDxe: Keep old data if value already existed. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Oct 2018 16:46:42 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 10/18/18 09:34, Eric Dong wrote: > AcpiCpuData add new fields, keep these fields if old data already existed. > > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > Reviewed-by: Ruiyu Ni > --- > UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > index ef98239844..1b847e453a 100644 > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > @@ -259,6 +259,8 @@ CpuS3DataInitialize ( > if (OldAcpiCpuData != NULL) { > AcpiCpuData->RegisterTable = OldAcpiCpuData->RegisterTable; > 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 > sorry my earlier point about CpuS3DataDxe may not have been clear. OVMF does not include any PEIM that consumes RegisterCpuFeaturesLib, so it does not set PcdCpuS3DataAddress, in the PEI phase (or in the DXE phase elsewhere, for that matter). That is, in this code, OVMF takes the other branch: "Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs". This series does not extend that branch, and as a result, the new fields are all zero-filled. (From the AllocateZeroPages() function call near the top of the CpuS3DataInitialize() function.) But, in patch #4, in PiSmmCpuDxeSmm, the GetAcpiCpuData() function calls AllocateCopyPool() -- copying data from normal RAM into SMRAM -- on the following pointers: - (UINT32 *)(UINTN)AcpiCpuData->CpuStatus.ValidCoreCountPerPackage - (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)AcpiCpuData->ApLocation These pointers will be NULL at that time. That's a problem in particular for "ApLocation", because the byte count of the copy is mAcpiCpuData.NumberOfCpus * sizeof (EFI_CPU_PHYSICAL_LOCATION) which is nonzero (even on OVMF). Now, what I'm suggesting is *not* that we extend this "other" branch in CpuS3DataInitialize(), so that we fill in the new fields sensibly. That can't be done in a platform-independent manner. For example, getting the CPU topology could be platform dependent. However, I am requesting that platforms be allowed to continue working with CpuS3DataDxe and PiSmmCpuDxeSmm even if they do not need the new feature (i.e. if they do not set any MSRs at S3 resume). That means that GetAcpiCpuData() in PiSmmCpuDxeSmm should work if the new fields are all zero. For explaining myself better, please consider a new Feature PCD that declares whether a platform sets MSRs at S3 resume. If it doesn't, then it doesn't need the new semaphore feature either. Then, if the FeaturePCD is FALSE, and the register stable *still* contains a semaphore / dependency operation, we can trigger an assert. Now, I know that new FeaturePCDs are not welcome. And, I don't really want to introduce one, I just used it as an example, for illustration. Because we can determine the same condition *dynamically* in PiSmmCpuDxeSmm. (And, perhaps we should document it in patch #1, "AcpiCpuData.h", as well.) For example: If the platform does not support MSR setting at S3 resume, and therefore it doesn't need the dependency semaphores, it should set both the CpuStatus.ValidCoreCountPerPackage field, and the ApLocation field, to 0. Then CpuS3DataDxe will need no further change beyond the present patch; however PiSmmCpuDxeSmm should check for those zero values, and disable the new feature dynamically. Thanks Laszlo