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

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, August 9, 2018 7:21 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib:
> Combine implementation.
> 
> 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.
> 

Yes, this code is used to restore data from Smram to AcpiNVS memory.  But I think this is a weird behavior and we should use GDT/IDT in smram. I will submit a separate patch to do this change.

Thanks,
Eric
> 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