From: "Kinney, Michael D" <michael.d.kinney@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
"Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>, Yi Li <phoenix.liyi@huawei.com>,
"Dong, Eric" <eric.dong@intel.com>,
Renhao Liang <liangrenhao@huawei.com>,
"Gao, Liming" <liming.gao@intel.com>,
Heyi Guo <heyi.guo@linaro.org>,
"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH V2] MdeModulePkg/Gcd: Filter gCpu->SetMemoryAttributes() calls
Date: Wed, 4 Apr 2018 20:28:01 +0000 [thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F5B89AF0B0@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AB339F7@shsmsx102.ccr.corp.intel.com>
Star,
Thanks for adding the comments
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Mike
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, April 4, 2018 1:13 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-
> devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li
> <phoenix.liyi@huawei.com>; Dong, Eric
> <eric.dong@intel.com>; Renhao Liang
> <liangrenhao@huawei.com>; Gao, Liming
> <liming.gao@intel.com>; Heyi Guo <heyi.guo@linaro.org>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter
> gCpu->SetMemoryAttributes() calls
>
> Thanks.
>
> Reviewed-by: Jiewen.yao@intel.com
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org] On Behalf Of Star
> > Zeng
> > Sent: Wednesday, April 4, 2018 10:08 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li
> <phoenix.liyi@huawei.com>; Dong, Eric
> > <eric.dong@intel.com>; Renhao Liang
> <liangrenhao@huawei.com>; Gao, Liming
> > <liming.gao@intel.com>; Heyi Guo
> <heyi.guo@linaro.org>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> > Subject: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter
> > gCpu->SetMemoryAttributes() calls
> >
> > From: "Kinney, Michael D"
> <michael.d.kinney@intel.com>
> >
> > This patch fixes an issue with VlvTbltDevicePkg
> introduced
> > by commit 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> >
> > The history is as below.
> > To support heap guard feature,
> > 14dde9e903bb9a719ebb8f3381da72b19509bc36
> > added support for SetMemorySpaceAttributes() to
> handle page attributes,
> > but after that, a combination of CPU arch attributes
> and other attributes
> > was not allowed anymore, for example, UC + RUNTIME.
> It is a regression.
> > Then 5b91bf82c67b586b9588cbe4bbffa1588f6b5926 was to
> fix the regression,
> > and we thought 0 CPU arch attributes may be used to
> clear CPU arch
> > attributes, so 0 CPU arch attributes was allowed to
> be sent to
> > gCpu->SetMemoryAttributes().
> >
> > But some implementation of CPU driver may return
> error for 0 CPU arch
> > attributes. That fails the case that caller just
> calls
> > SetMemorySpaceAttributes() with none CPU arch
> attributes (for example,
> > RUNTIME), and the purpose of the case is not to clear
> CPU arch attributes.
> >
> > This patch filters the call to gCpu-
> >SetMemoryAttributes()
> > if the requested attributes is 0. It also removes
> the #define
> > INVALID_CPU_ARCH_ATTRIBUTES that is no longer used.
> >
> > Cc: Heyi Guo <heyi.guo@linaro.org>
> > Cc: Yi Li <phoenix.liyi@huawei.com>
> > Cc: Renhao Liang <liangrenhao@huawei.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > ---
> > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 25
> ++++++++++++++++++++++---
> > 1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > index 907245a3f512..31ddf9c3bb51 100644
> > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > @@ -48,8 +48,6 @@ WITHOUT WARRANTIES OR
> REPRESENTATIONS OF ANY
> > KIND, EITHER EXPRESS OR IMPLIED.
> > #define NONEXCLUSIVE_MEMORY_ATTRIBUTES
> (EFI_MEMORY_XP |
> > EFI_MEMORY_RP | \
> >
> EFI_MEMORY_RO)
> >
> > -#define INVALID_CPU_ARCH_ATTRIBUTES 0xffffffff
> > -
> > //
> > // Module Variables
> > //
> > @@ -873,7 +871,21 @@ CoreConvertSpace (
> > // Call CPU Arch Protocol to attempt to set
> attributes on the range
> > //
> > CpuArchAttributes = ConverToCpuArchAttributes
> (Attributes);
> > - if (CpuArchAttributes !=
> INVALID_CPU_ARCH_ATTRIBUTES) {
> > + //
> > + // CPU arch attributes include page attributes
> and cache attributes.
> > + // Only page attributes supports to be cleared,
> but not cache attributes.
> > + // Caller is expected to use
> GetMemorySpaceDescriptor() to get the current
> > + // attributes, AND/OR attributes, and then calls
> > SetMemorySpaceAttributes()
> > + // to set the new attributes.
> > + // So 0 CPU arch attributes should not happen as
> memory should always
> > have
> > + // a cache attribute (no matter UC or WB, etc).
> > + //
> > + // Here, 0 CPU arch attributes will be filtered
> to be compatible with the
> > + // case that caller just calls
> SetMemorySpaceAttributes() with none CPU
> > + // arch attributes (for example, RUNTIME) as the
> purpose of the case is not
> > + // to clear CPU arch attributes.
> > + //
> > + if (CpuArchAttributes != 0) {
> > if (gCpu == NULL) {
> > Status = EFI_NOT_AVAILABLE_YET;
> > } else {
> > @@ -936,6 +948,13 @@ CoreConvertSpace (
> > // Set attributes operation
> > //
> > case GCD_SET_ATTRIBUTES_MEMORY_OPERATION:
> > + if (CpuArchAttributes == 0) {
> > + //
> > + // Keep original CPU arch attributes when
> caller just calls
> > + // SetMemorySpaceAttributes() with none CPU
> arch attributes (for
> > example, RUNTIME).
> > + //
> > + Attributes |= (Entry->Attributes &
> > (EXCLUSIVE_MEMORY_ATTRIBUTES |
> NONEXCLUSIVE_MEMORY_ATTRIBUTES));
> > + }
> > Entry->Attributes = Attributes;
> > break;
> > //
> > --
> > 2.7.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2018-04-04 20:28 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-04 2:08 [PATCH V2] MdeModulePkg/Gcd: Filter gCpu->SetMemoryAttributes() calls Star Zeng
2018-04-04 8:13 ` Yao, Jiewen
2018-04-04 20:28 ` Kinney, Michael D [this message]
2018-04-05 1:14 ` Zeng, Star
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E92EE9817A31E24EB0585FDF735412F5B89AF0B0@ORSMSX113.amr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox