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.web11.6307.1610014192085349900 for ; Thu, 07 Jan 2021 02:09:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XT+N3MR/; 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=1610014191; 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=FiLy6l+JyUJzmxQsJnwqn2RbARFVRypjJmAlsmIELjg=; b=XT+N3MR/myK0RR28nU65XaBTV9z1ov13v8leJrTNUa46wZ/giEUmtnq4TXuNGaG9ddaJ5i olM6jHFkl6YMEQL7/j4zKOai4k0tghtQQ81jIaFUdHzfhxX4Xr46tGWn6BpgTrymoopIxM r6facC/mV5ospJrjRuEbBq1qU/DFiAk= 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-146-2N4rX1xFPCqeV5pZ_pbwKQ-1; Thu, 07 Jan 2021 05:09:47 -0500 X-MC-Unique: 2N4rX1xFPCqeV5pZ_pbwKQ-1 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id 46A45107ACE3; Thu, 7 Jan 2021 10:09:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-119.ams2.redhat.com [10.36.114.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id E0BDB1001B2C; Thu, 7 Jan 2021 10:09:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c To: "Ni, Ray" , "devel@edk2.groups.io" , "Zeng, Star" Cc: "Dong, Eric" References: <20210106064825.18984-1-star.zeng@intel.com> From: "Laszlo Ersek" Message-ID: <37d2bda3-5173-34d1-5338-92023b0fb2c5@redhat.com> Date: Thu, 7 Jan 2021 11:09:43 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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: 7bit On 01/07/21 06:23, Ni, Ray wrote: >> This patch is a step in the right direction, but, IMO, it can be improved. >> >> The IsRegisterTableEmpty() function should also handle the case when the >> "RegisterTable" parameter is NULL. The function should then also return >> TRUE. >> >> And then, two more patches would be nice: >> >> >> (2) The register table allocation in >> "UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c" should be removed. >> >> Right now, the code there allocates memory just so it can set the >> "TableLength" and "AllocatedSize" fields to zero, for each "InitialApicId". >> >> It's a waste -- the memory is allocated only for ensuring that >> PiSmmCpuDxeSmm does not program any CPU registers during S3 resume. >> >> Setting "AcpiCpuData->RegisterTable" and >> "AcpiCpuData->PreSmmInitRegisterTable" should have the same effect. > > Laszlo, > It's a good suggestion. > We also need to change CpuS3.c:SetRegister() to directly return > when mAcpiCpuData.[PreSmmInit]RegisterTable is NULL. The current (v2) patch already contains that shortcut; please see: > @@ -487,6 +487,9 @@ SetRegister ( > } else { > RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.RegisterTable; > } > + if (RegisterTables == NULL) { > + return; > + } > > InitApicId = GetInitialApicId (); > RegisterTable = NULL; So that's fine, IMO. The data flow is the following: (1) GetAcpiCpuData() determines whether each one of the two register tables *ouside of SMRAM* is empty or not; (2) for each one that is empty (outside of SMRAM), the pointer to the *copy in SMRAM* is set to NULL, (3) for each one that is not empty (outside of SMRAM), an SMRAM copy is made, (4) SetRegister() correctly deals -- already in this v2 patch -- with the the pointer into SMRAM being NULL. My update request refers to step (1). Namely, when we determine whether each register table *ouside of SMRAM* is empty or not, we should also recognize when the table (outside of SMRAM) doesn't even *exist*. Basically I'm asking that Star please post a v3 with the following update squashed into the patch: > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 9b1f952c8a74..0e7a4ba5369d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -998,6 +998,10 @@ IsRegisterTableEmpty ( > { > UINTN Index; > > + if (RegisterTable == NULL) { > + return TRUE; > + } > + > for (Index = 0; Index < NumberOfCpus; Index++) { > if (RegisterTable[Index].TableLength != 0) { > return FALSE; Back to your email: On 01/07/21 06:23, Ni, Ray wrote: > > Considering the urgency of this patch (fix a system hang caused by smram shortage), > do you agree that we can first let this patch in? I agree that the CpuS3DataDxe changes can be done separately, later. However, I'd like the above small hunk -- in IsRegisterTableEmpty() -- to be squashed into the current patch, before committing it. If you think we should do this small update on commit, rather than posting a v3, that's fine with me as well. Just please don't merge the v2 patch without the IsRegisterTableEmpty() extension that I'm asking for, above. With the IsRegisterTableEmpty() update, even without a v3 posting: Reviewed-by: Laszlo Ersek Thanks Laszlo