Re: [edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c

["Zeng, Star" <star.zeng@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 7, 2021 6:10 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Zeng, Star
> <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm:
> Reduce SMRAM consumption in CpuS3.c
> 
> 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 <lersek@redhat.com>

Thanks Laszlo and Ray

This patch is majorly to reduce SMRAM consumption in *consumer* side of "RegisterTable".
I was preparing PATCH V3 before you sent this reply. 😊

About the change to *producer* side of "RegisterTable", UefiCpuPkg\Library\RegisterCpuFeaturesLib depended on CpuS3DataDxe before, the dependency seems have been removed by a6daab1f6cb836e4147324bb85fc17930be14e87. We may can do the change after doing more confirmation.

I just sent PATCH V3 with your both RB (suppose you both agree 😊) with a very very little difference to your proposal.

Thanks,
Star

> 
> Thanks
> Laszlo