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

["Laszlo Ersek" <lersek@redhat.com>]

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