From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 59D9B2097E26E for ; Tue, 17 Jul 2018 07:36:50 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EB74177885; Tue, 17 Jul 2018 14:36:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-194.rdu2.redhat.com [10.10.120.194]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8F5D7FF69; Tue, 17 Jul 2018 14:36:48 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Eric Dong , Jiewen Yao , Star Zeng References: <20180713055357.4196-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <2b6a7d47-8d72-0a06-a41e-945226d57bf4@redhat.com> Date: Tue, 17 Jul 2018 16:36:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180713055357.4196-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 17 Jul 2018 14:36:50 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 17 Jul 2018 14:36:50 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jul 2018 14:36:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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/0C09AFA07DD0434D9E2A0C6AEB0483103BB55B46@shsmsx102.ccr.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? > 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? > > 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). How did you encounter / find this issue? > > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Jiewen Yao > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > 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. 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.) Thanks, Laszlo