Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek <lersek@redhat.com>]

On 08/09/18 05:18, Ni, Ruiyu wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, August 9, 2018 5:29 AM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib:
>> Combine implementation.
>>
>> On 08/08/18 09:40, Eric Dong wrote:
>>> V1 changes:
>>>> Current code logic can't confirm CpuS3DataDxe driver start before
>>>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
>>>> Add implementation for AllocateAcpiCpuData function to remove this
>>>> assumption.
>>>
>>> V2 changes:
>>>> Because CpuS3Data memory will be copy to smram at SmmReadToLock
>>>> point, so the memory type no need to be ACPI NVS type, also the
>>>> address not limit to below 4G.
>>>> This change remove the limit of ACPI NVS memory type and below 4G.
>>
>> I'm returning to this patch (v2 1/2) after my other comments (for v2 2/2).
>>
>> It seems that allocating ACPI_CPU_DATA in BootServicesData type memory
>> breaks at least the docs / specs in "UefiCpuPkg/Include/AcpiCpuData.h",
>> even if we do the BootServicesData allocation in RegisterCpuFeaturesLib
>> instances (i.e. in CpuFeaturesPei / CpuFeaturesDxe), and not in
>> CpuS3DataDxe.
>>
>> With that in mind, should we return to your v1 patch,
>> "UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData
>> function"?
>>
>> And, looking back at my question (4) there, where I suggested
>> AllocatePeiAccessiblePages() -- I'm now thinking that it does not apply,
>> because the ACPI_CPU_DATA spec in "UefiCpuPkg/Include/AcpiCpuData.h"
>> requires the 4GB limit.
> 
> I guess Eric forgot to update the comments in AcpiCpuData.h regarding the
> 4GB/NVS restriction.

If patch #2 is fixed, *and* the "AcpiCpuData.h" documentation is
updated, relaxing the allocation restriction, then this patch set could
be viable, yes. I would still like Mike to confirm as well.

> We've reviewed the whole producer/consumer code and came to the conclusion
> that 4GB/NVS restriction is unnecessary.

I think you both missed the in-place restoration of the GDT and the IDT,
to AcpiNVS memory allocated originally by CpuS3DataDxe. Please re-read
section (10) of my other email carefully:

http://mid.mail-archive.com/f48935e4-96b3-978e-d67c-84a169414ccb@redhat.com

Basically, the GdtrProfile and IdtrProfile fields in ACPI_CPU_DATA are
*doubly* indirect references. The fields themselves point to IDT and GDT
*descriptors*, and those descriptors (the Base fields in them) point to
the tables (IDT and GDT).

Indeed, PiSmmCpuDxeSmm saves everything into SMRAM on the normal boot
path: (a) ACPI_CPU_DATA, (b) the descriptors pointed-to by GdtrProfile
and IdtrProfile, and (c) the tables pointed-to by GdtrProfile->Base and
IdtrProfile->Base.

However, on the S3 resume path, PiSmmCpuDxeSmm does not use everything
from SMRAM. In the PrepareApStartupVector() function, the GDT and the
IDT tables -- at the end of the pointer chain -- are restored to the
*original* AcpiNVS locations (*not* SMRAM), and they are then used from
there.

This is not a security bug, because even if the OS overwrites the
AcpiNVS area between normal boot and S3 resume, PiSmmCpuDxeSmm never
reads that area at S3 before restoring it, from the SMRAM objects
"mGdtForAp" and "mIdtForAp".

It does mean though that the OS must be informed in advance to stay away
from that area, because PiSmmCpuDxeSmm will overwrite it at S3 resume.
This is why that allocation must be kept as AcpiNVS. (Or moved to SMRAM
entirely.)

Laszlo