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
