On 10/31/13 18:53, Jordan Justen wrote:
> On Thu, Oct 31, 2013 at 2:40 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 10/28/13 22:27, Jordan Justen wrote:
>>> Previously we would only search for MMIO regions that were also
>>> above every EfiGcdMemoryTypeReserved and EfiGcdMemoryTypeSystemMemory
>>> region.
>>>
>>> Now we just search for the largest EfiGcdMemoryTypeMemoryMappedIo
>>> region.
>>>
>>> This will allow us to mark the flash memory as a runtime memory
>>> region in order to allow runtime access of variables stored in
>>> flash.
(*)
>>
>> What happens if this patch is not included?
>
> This branch (from the old code) is not taken w/o this patch
> "if (Mmio32MinBase < Mmio32MaxExclTop) {"
>
> And therefore, PciWindow32 is not set...
Ah.
Can you please write more helpful / detailed commit messages and/or
cover letter?
This patchset is built in reverse (which isn't unusual, in order to
avoid regressions mid-series), but, unlike the submitter, the reviewer
has no clue about the future. In other words, when reviewing patch #4, I
have no idea about patch #6 or patch #7.
The paragraph I marked with (*) is way too laconic.
What's going on here is that in patch 7/8 a new driver is added to the
apriori list, hence it runs super-early. Specifically, earlier than the
code being patched in patch 4/8. This new driver marks a memory region
as runtime memory in patch 6/8 (again, at said very early time), and
this memory region ends exactly at 4GB. The marking is done by
*modifying the memory map*, which is what the code under patch 4/8 feeds
off.
Hence, the
find MMIO bounding box and clamp it from below with conventional
memory
logic is *precisely* busted by the conventional RAM range added
chronologically earlier at the top of the first four gigabytes. When the
MMIO bounding box is clamped above 4G, nothing remains.
> for (CurDesc = 0; CurDesc < NumDesc; ++CurDesc) {
> CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc;
> UINT64 ExclTop;
>
> Desc = &AllDesc[CurDesc];
> ExclTop = Desc->BaseAddress + Desc->Length;
>
> if (ExclTop <= BASE_4GB) {
The conventional RAM range in question satisfies this check.
> switch (Desc->GcdMemoryType) {
> case EfiGcdMemoryTypeNonExistent:
> break;
>
> case EfiGcdMemoryTypeReserved:
> case EfiGcdMemoryTypeSystemMemory:
> if (NonMmio32MaxExclTop < ExclTop) {
> NonMmio32MaxExclTop = ExclTop;
> }
> break;
>
> case EfiGcdMemoryTypeMemoryMappedIo:
> if (Mmio32MinBase > Desc->BaseAddress) {
> Mmio32MinBase = Desc->BaseAddress;
> }
> if (Mmio32MaxExclTop < ExclTop) {
> Mmio32MaxExclTop = ExclTop;
> }
> break;
>
> default:
> ASSERT(0);
> }
> }
> }
After the loop:
o (Mmio32MinBase < Mmio32MaxExclTop) is true; some bounding box has
been found,
o but (NonMmio32MaxExclTop == BASE_4GB) holds as well.
>
> if (Mmio32MinBase < NonMmio32MaxExclTop) {
> Mmio32MinBase = NonMmio32MaxExclTop;
> }
This is the "clamp from below" stuff. Mmio32MinBase is raised to BASE_4GB.
>
> if (Mmio32MinBase < Mmio32MaxExclTop) {
After which this check (nonempty bounding box after clamping) clearly
fails, and we return EFI_UNSUPPORTED, and FWDT is not installed.
(The pre-patch code actually works as expected, what's unexpected is a
conventional RAM range just below 4G.)
So, my R-b stands, but I wish you had helped me understand this more
quickly.
I think I won't try to review 6/8, I'll just test it. But first I'd like
to hear what you think of the regression I described in another response
to 4/8.
Thanks
Laszlo
------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
