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 3F84121A07A92 for ; Tue, 11 Sep 2018 11:50:03 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2ACEB80C3C2C; Tue, 11 Sep 2018 18:50:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-36.rdu2.redhat.com [10.10.120.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3DA2D101041C; Tue, 11 Sep 2018 18:50:00 +0000 (UTC) To: Leo Duran , edk2-devel@lists.01.org Cc: Eric Dong , Ruiyu Ni References: <1536680498-6554-1-git-send-email-leo.duran@amd.com> <1536680498-6554-2-git-send-email-leo.duran@amd.com> From: Laszlo Ersek Message-ID: <17c6d6d1-2655-fe06-a8b9-f48141bfb0d7@redhat.com> Date: Tue, 11 Sep 2018 20:49:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1536680498-6554-2-git-send-email-leo.duran@amd.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 11 Sep 2018 18:50:02 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 11 Sep 2018 18:50:02 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Sep 2018 18:50:03 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/11/18 17:41, Leo Duran wrote: > The default behavior is to disable MTRRs prior to an MTRR change. > However, on SMT platforms with shared CPU resources it may be desirable to > skip the default behavior, and leave the current state of the Enable bit. > > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Leo Duran > --- > UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 10 +++++++--- > UefiCpuPkg/Library/MtrrLib/MtrrLib.inf | 3 +++ > UefiCpuPkg/UefiCpuPkg.dec | 7 +++++++ > 3 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > index dfce9a9..baf9a0f 100644 > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > @@ -6,6 +6,8 @@ > except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR setting to APs. > > Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2018, AMD Inc. All rights reserved.
> + > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -310,9 +312,11 @@ MtrrLibPreMtrrChange ( > // > // Disable MTRRs > // > - DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE); > - DefType.Bits.E = 0; > - AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64); > + if (!PcdGetBool (PcdSkipDisableMtrrsOnPreMtrrChange)) { > + DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE); > + DefType.Bits.E = 0; > + AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64); > + } > } > > /** > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf > index a97cc61..06f33e8 100644 > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf > @@ -2,6 +2,8 @@ > # MTRR library provides APIs for MTRR operation. > # > # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +# Copyright (c) 2018, AMD Inc. All rights reserved.
> +# > # This program and the accompanying materials > # are licensed and made available under the terms and conditions of the BSD License > # which accompanies this distribution. The full text of the license may be found at > @@ -43,4 +45,5 @@ > > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuNumberOfReservedVariableMtrrs ## SOMETIMES_CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange ## CONSUMES > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index 69d777a..64ec763 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -2,6 +2,7 @@ > # This Package provides UEFI compatible CPU modules and libraries. > # > # Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2018, AMD Inc. All rights reserved.
> # > # This program and the accompanying materials are licensed and made available under > # the terms and conditions of the BSD License which accompanies this distribution. > @@ -273,6 +274,12 @@ > # @Prompt Current boot is a power-on reset. > gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x0000001B > > + ## Indicates desired behavior for disabling MTRRs prior to MTRR change.

> + # TRUE - Skip disabling MTRRs prior to MTRR change.
> + # FALSE - Disable MTRRs prior to MTRR change.
> + # @Prompt Desired behavior for disabling MTRRs prior to MTRR change. > + gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange|FALSE|BOOLEAN|0x0000001E > + > [PcdsDynamic, PcdsDynamicEx] > ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA. > # @Prompt The pointer to a CPU S3 data buffer. > Recently, Ray has written several & significant patches for MtrrLib; I'm adding him. I don't understand the motivation behind this patch. As far as I remember (which is admittedly "quite vaguely"), the SDM requires all logical processors to program their MTRRs identically in parallel. That is, there should be a start line where all the CPUs wait for each other, then they all set up their MTRRs, then they all wait until they all finish, then they all go their merry ways. AIUI the CPU MP PPI and protocol implement this already. I don't understand in what situation you'd have one thread of a core manipulating MTRR, with the sibling thread *not* manipulating MTRR (i.e., doing something else). That doesn't seem to match what the SDM dictates (or, well, what my memories of the SDM are :) ). I see that the default behavior doesn't change, and I'm not against the patch; I just suspect that, for introducing a new PCD, more concrete / practical justification could be needed. More questions: - Don't you need a similar (symmetric) change in MtrrLibPostMtrrChange()? If not, why not? Can you add that to the commit message? - I don't know how exactly the current (sophisticated?) MTRR update algorithm works (developed by Ray); are you sure it's safe to let it massage "armed" MSRs while DefType.Bits.E is set? (Hmmm, sorry if this "review" is more centered on my own lack of information than on your patch...) Thanks Laszlo