Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, August 9, 2018 7:21 PM > To: Ni, Ruiyu <ruiyu...@intel.com>; Dong, Eric <eric.d...@intel.com>; edk2- > de...@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 <ler...@redhat.com> > >> Sent: Thursday, August 9, 2018 5:29 AM > >> To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org > >> Cc: Ni, Ruiyu <ruiyu...@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- > 84a169414...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
