From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "Wang, Jian J" <jian.j.wang@intel.com>,
"Zeng, Star" <star.zeng@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Heyi Guo" <heyi.guo@linaro.org>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>, Yi Li <phoenix.liyi@huawei.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Dong, Eric" <eric.dong@intel.com>,
"Renhao Liang" <liangrenhao@huawei.com>
Subject: Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
Date: Tue, 3 Apr 2018 02:25:30 +0000 [thread overview]
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AB1F68A@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <D827630B58408649ACB04F44C510003624D20473@SHSMSX103.ccr.corp.intel.com>
I have a discussion with Star/Jian.
The expectation for the CPU driver is to handle PageAttribute.
The expectation for the platform driver is to use GetAttribute(), AND/OR attribute, then call SetAttribute().
Because the DRAM always has a cache attribute (no matter UC or WB), 0 should not happen. (0 might happen for a GCD reserved memory, but there is no need to set page attribute)
If all drivers use above way, there won't be any issue. There is no need to introduce a new protocol so far.
We did encounter some error, because the platform/silicon/CPU code is not updated yet. (For example, the MinnowMax which is using binary)
The fix to filter 0 seems a workable way to resolve the compatibility issue.
Later, I suggest we update MinnowMax binary - Add paging handling for CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Tuesday, April 3, 2018 9:24 AM
> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,
> Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao
> Liang <liangrenhao@huawei.com>
> Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> conversion
>
> The NX Memory Protection and Heap Guard features need to clear paging
> attributes.
> Allowing 0 attribute is the current only choice without changing arch protocol.
> Maybe
> It's time to introduce a new protocol.
>
> Regards,
> Jian
>
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, April 03, 2018 9:16 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo
> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org
> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang
> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu
> > <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> >
> > Current gCpu->SetMemoryAttributes does not support getting memory
> > attributes, and has no clear description about clearing memory attributes.
> >
> > I noticed we introduced
> >
> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut
> e.
> > h) protocol for heap guard feature in SMM environment.
> > Seemingly, we also need introduce MemoryAttribute protocol for DXE?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, April 3, 2018 8:59 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo
> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org
> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang
> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu
> > <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> >
> > Mike,
> >
> > Agree the problem.
> > We need support only RUNTIME attribute.
> > We need support only cache attributes.
> > We need support only page attributes.
> > We need support RUNTIME + cache + page attributes.
> > Do we need support the 0 Attributes case to clear page attributes?
> > There was discussion about the 0 Attributes case at
> > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.
> > It came to the question that should the 0 Attributes case be handled by
> > SetMemoryAttributes() to clear the page attributes?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Tuesday, April 3, 2018 5:43 AM
> > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-
> > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang
> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu
> > <ruiyu.ni@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> >
> > Star,
> >
> > This commit breaks Vlv2TbltDevicePkg.
> >
> > On this platform, there are 2 calls to
> > gDS->SetMemorySpaceAttributes() for an MMIO
> > range to set only the EFI_MEMORY_RUNTIME bit.
> >
> > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is passed to
> > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on
> > Vlv2TbltDevicePkg.
> >
> > Before this commit, ConverToCpuArchAttributes() returns
> > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-
> > >SetMemoryAttributes()to be skipped so no error is generated.
> >
> > I think the right fix is to skip the call to
> > gCpu->SetMemoryAttributes() if CpuArchAttributes
> > is 0 so a call that only sets the RUNTIME attribute can be supported along with
> > call the set both the RUNTIME bit and other cache related bits.
> >
> > I will send a patch soon with the proposed fix.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Sunday, April 1, 2018 10:59 PM
> > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org
> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang
> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,
> > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > conversion
> > >
> > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> > >
> > > Thanks,
> > > Star
> > > -----Original Message-----
> > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > Sent: Thursday, March 29, 2018 4:20 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li <phoenix.liyi@huawei.com>;
> > > Renhao Liang <liangrenhao@huawei.com>; Zeng, Star
> > > <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,
> > > Ruiyu <ruiyu.ni@intel.com>
> > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > >
> > > For gDS->SetMemorySpaceAttributes(), when user passes a combined
> > > memory attribute including CPU arch attribute and other attributes,
> > > like EFI_MEMORY_RUNTIME,
> > > ConverToCpuArchAttributes() will return INVALID_CPU_ARCH_ATTRIBUTES
> > > and skip setting page/cache attribute for the specified memory space.
> > >
> > > We don't see any reason to forbid combining CPU arch attributes and
> > > non-CPU-arch attributes when calling gDS-
> > > >SetMemorySpaceAttributes(), so we remove the check code
> > > in ConverToCpuArchAttributes(); the remaining code is enough to grab
> > > the interested bits for
> > > Cpu->SetMemoryAttributes().
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@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>
> > > ---
> > > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----
> > > 1 file changed, 5 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> > > 77f4adb4bc01..907245a3f512 100644
> > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes ( {
> > > UINT64 CpuArchAttributes;
> > >
> > > - if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > - NONEXCLUSIVE_MEMORY_ATTRIBUTES))
> > > != 0) {
> > > - return INVALID_CPU_ARCH_ATTRIBUTES;
> > > - }
> > > -
> > > CpuArchAttributes = Attributes &
> > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;
> > >
> > > if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
> > > --
> > > 2.7.4
>
> _______________________________________________
> 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-03 2:25 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-29 8:19 [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo
2018-03-30 1:59 ` Zeng, Star
2018-04-02 5:58 ` Zeng, Star
2018-04-02 21:43 ` Kinney, Michael D
2018-04-03 0:59 ` Zeng, Star
2018-04-03 1:15 ` Zeng, Star
2018-04-03 1:24 ` Wang, Jian J
2018-04-03 2:25 ` Yao, Jiewen [this message]
2018-04-03 2:33 ` Zeng, Star
2018-04-03 2:34 ` Yao, Jiewen
2018-04-03 2:41 ` Zeng, Star
2018-04-03 6:06 ` Yao, Jiewen
2018-04-03 21:45 ` Kinney, Michael D
2018-04-04 0:38 ` Zeng, Star
2018-04-04 1:12 ` Guo Heyi
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=74D8A39837DF1E4DA445A8C0B3885C503AB1F68A@shsmsx102.ccr.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