From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.33037.1599202240772056669 for ; Thu, 03 Sep 2020 23:50:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BFYETIF2; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1599202240; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+eO8QuRkvKr8PKwXjBvd7xBCG+ElE9GqfaaUh8alvAo=; b=BFYETIF2hXESsOerSZ8xKaPeEs6fCpaMIN1Ksd2qxL0VQavIIwE1/oayMT4fnXyuf6lqQp jdF9rSBpCAdDueGRMO0sbff2BF/tARosRyC6vpBFIMud7IkAenuN5M5eEuNn74br7FP3zr YtBkVCwE+qgiYeMzKV8sEXd/lrFw2Ho= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-48-ooZ1m717M6i211Hepyh0nQ-1; Fri, 04 Sep 2020 02:50:35 -0400 X-MC-Unique: ooZ1m717M6i211Hepyh0nQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0584F807345; Fri, 4 Sep 2020 06:50:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-161.ams2.redhat.com [10.36.112.161]) by smtp.corp.redhat.com (Postfix) with ESMTP id 88A3910013BD; Fri, 4 Sep 2020 06:50:32 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. To: "Yao, Jiewen" , "devel@edk2.groups.io" , "Dong, Eric" , Fan Jeff , "Ni, Ray" Cc: "Lou, Yun" References: <20200903151147.1196-1-eric.dong@intel.com> <9c9d8289-4f8e-75d8-2816-750195a54842@redhat.com> From: "Laszlo Ersek" Message-ID: <1956a667-eff6-5bf4-e24e-b1c2a5530eee@redhat.com> Date: Fri, 4 Sep 2020 08:50:30 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US On 09/04/20 05:09, Yao, Jiewen wrote: > Hey > I would like to ask the original requirement. > > Why there is a test case to test such configuration, to move the page table or GDT or IDT? I don’t understand. +1 > Is that complexity really needed ? > > If we treat it as an invalid test case, then we don’t need do anything here, right ? Exactly. My understanding is just that Eric really needs this application to *not* hang (if it fails gracefully, that seems to be OK). I'm happy to accommodate that, but: - the MpInitLib code should suffer as little as possible complications as a result, - other platforms should not take a hit *at all*. Laszlo > > > > > From: devel@edk2.groups.io On Behalf Of Dong, Eric > Sent: Friday, September 4, 2020 10:27 AM > To: Fan Jeff ; devel@edk2.groups.io; Ni, Ray ; lersek@redhat.com > Cc: Lou, Yun > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. > > Agree, current is too much PCDs for the driver. > > Laszlo, what’s your comments? > > Thanks, > Eric > From: Fan Jeff > > Sent: Friday, September 4, 2020 10:19 AM > To: devel@edk2.groups.io; Dong, Eric >; Ni, Ray >; lersek@redhat.com > Cc: Lou, Yun > > Subject: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. > > Laszlo & Eric, > > Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib. > > Jeff > > 发件人: Dong, Eric > 发送时间: 2020年9月4日 10:01 > 收件人: Ni, Ray; devel@edk2.groups.io; lersek@redhat.com > 抄送: Lou, Yun > 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. > > I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear. > > Thanks, > Eric > > From: Ni, Ray > > Sent: Friday, September 4, 2020 9:34 AM > To: devel@edk2.groups.io; lersek@redhat.com; Dong, Eric > > Cc: Lou, Yun > > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. > > Why do we need a new PCD to control such check? Under what circumstance the PCD is false? > We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes. > > We should not assume PEI runs at 32 bit mode. > > > 发件人: devel@edk2.groups.io > 代表 Laszlo Ersek > > 发送时间: Friday, September 4, 2020 3:55:47 AM > 收件人: Dong, Eric >; devel@edk2.groups.io > > 抄送: Ni, Ray > > 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. > > On 09/03/20 21:00, Laszlo Ersek wrote: > >> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the >> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and >> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c". >> >> Instead, the calls should be made in the DXE instance of the library >> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the >> functions: >> >> - MpInitLibStartupAllAPs >> - MpInitLibStartupThisAP >> - MpInitLibSwitchBSP >> - MpInitLibEnableDisableAP >> >> Here's why: >> >> (a) The symptom does not affect the PEI phase -- by the time the UEFI >> application is executed, the PEI phase has ended; there's no need to >> modify the PEI instance of the library. >> >> (b) The currently proposed failure exits are too late. For example, in >> the SwitchBSPWorker() function, by the time we exit, we have called >> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and >> DisableLvtInterrupts() -- and the error path does not restore the >> original environment. >> >> (c) According to the PI spec (v1.7), the StartupAllAPs(), >> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of >> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP. >> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very >> first action in the above-listed DxeMpLib functions. >> >> (Assuming the protocol members are called from an AP, and consequently >> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the >> *caller's* fault, per spec!) > > This means we can move the ValidCr3GdtIdtCheck() function to > "DxeMpLib.c", and it is not necessary to modify "MpLib.h". > > Thanks > Laszlo > > >