On 07/19/18 11:07, Wang, Jian J wrote: > Hi Laszlo, > > Do you have more comments? Or can you give a r-b?
Struggling with my workload, will try to come back to this soon. Thanks Laszlo >> -----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
