Hi Laszlo, Do you have more comments? Or can you give a r-b?
Regards, Jian > -----Original Message----- > From: Wang, Jian J > Sent: Wednesday, July 18, 2018 10:36 AM > To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.d...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; > Zeng, Star <star.z...@intel.com> > Subject: RE: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode > > Hi Laszlo, > > > Regards, > Jian > > > > -----Original Message----- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Tuesday, July 17, 2018 10:37 PM > > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.d...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; > > Zeng, Star <star.z...@intel.com> > > Subject: Re: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode > > > > On 07/13/18 07:53, Jian J Wang wrote: > > > Current IsInSmm() method makes use of gEfiSmmBase2ProtocolGuid.InSmm() > > to > > > check if current processor is in SMM mode or not. But this is not correct > > > because gEfiSmmBase2ProtocolGuid.InSmm() can only detect if the caller is > > > running in SMRAM or from SMM driver. It cannot guarantee if the caller is > > > running in SMM mode. > > > > Wow. This is the exact difference which I asked about, in my question > > (9b), and I was assured that InSmm() actually determined whether we were > > executing in Management Mode (meaning the processor operating mode). > > > > http://mid.mail- > > > archive.com/0c09afa07dd0434d9e2a0c6aeb0483103bb55...@shsmsx102.cc > > r.corp.intel.com > > > > (Scroll down in that message to see my original question (9b).) > > > > So why doesn't Star's explanation, from the original thread, apply > > ultimately? > > > > We did many tests for this and didn't found any issue. So I took a risk. (I > thought > a very precise check of SMM mode is hard and time consuming.) > > > > Because SMM mode will load its own page table, adding > > > an extra check of saved DXE page table base address against current CR3 > > > register value can help to get the correct answer for sure (in SMM mode or > > > not in SMM mode). > > > > So, apparently, the PI spec offers no standard way for a platform module > > to determine whether it runs in Management Mode, despite protocol member > > being called "InSmm". Do we need a PI spec ECR for introducing the > > needed facility? > > > > Alternatively, the PI spec might already intend to specify that, but the > > edk2 implementation doesn't do what the PI spec requires. > > > > Which one is the case? > > > > The implementation conforms to the spec. It's my misunderstanding. But it's > true > that there's no specific protocol API to determine if it's in SMM mode or not. > > > > > > > This is an issue caused by check-in at > > > > > > d106cf71eabaacff63c14626a4a87346b93074dd > > > > I disagree; I think the issue was introduced in commit 2a1408d1d739 > > ("UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode", > > 2018-06-19). > > > > You're right. Thanks for pointing this out. > > > > > How did you encounter / find this issue? > > > > I didn't find it. The issue came to me. In other words, I think it's random > and hard > to reproduce it. Maybe a subtle change in boot sequence will hide it away. > > > > > > > Cc: Eric Dong <eric.d...@intel.com> > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > Cc: Jiewen Yao <jiewen....@intel.com> > > > Cc: Star Zeng <star.z...@intel.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > > > --- > > > UefiCpuPkg/CpuDxe/CpuPageTable.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c > > b/UefiCpuPkg/CpuDxe/CpuPageTable.c > > > index 850eed60e7..df021798c0 100644 > > > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > > > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > > > @@ -136,7 +136,14 @@ IsInSmm ( > > > mSmmBase2->InSmm (mSmmBase2, &InSmm); > > > } > > > > > > - return InSmm; > > > + // > > > + // mSmmBase2->InSmm() can only detect if the caller is running in SMRAM > > > + // or from SMM driver. It cannot tell if the caller is running in SMM > > > mode. > > > + // Check page table base address to guarantee that because SMM mode > > willl > > > + // load its own page table. > > > + // > > > + return (InSmm && > > > + mPagingContext.ContextData.X64.PageTableBase != > > (UINT64)AsmReadCr3()); > > > } > > > > > > /** > > > > > > > Shouldn't we consider Ia32.PageTableBase when that's appropriate? From > > "UefiCpuPkg/CpuDxe/CpuPageTable.h": > > > > typedef struct { > > UINT32 PageTableBase; > > UINT32 Reserved; > > UINT32 Attributes; > > } PAGE_TABLE_LIB_PAGING_CONTEXT_IA32; > > > > typedef struct { > > UINT64 PageTableBase; > > UINT32 Attributes; > > } PAGE_TABLE_LIB_PAGING_CONTEXT_X64; > > > > typedef union { > > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32 Ia32; > > PAGE_TABLE_LIB_PAGING_CONTEXT_X64 X64; > > } PAGE_TABLE_LIB_PAGING_CONTEXT_DATA; > > > > The Ia32/X64 structure types are not packed, and even if they were, I > > wouldn't think we should rely on "Reserved" being zero. > > > > mPagingContext is zero-ed at each update in GetCurrentPagingContext(). > I think it should be no problem to use X64. > > > > > All in all, I think that determining whether the processor is operating > > in Management Mode (regardless of where in RAM the processor is > > executing code from) is a core functionality that should be offered as a > > central service, not just an internal CpuDxe function. I think we need > > either a PI spec addition, or at least an EDKII extension protocol. It's > > obvious that the InSmm() behavior is unclear to developers! (Me > > included, of course.) > > > > I agree. > > > Thanks, > > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
