Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
On 11/06/13 07:04, Jordan Justen wrote: > On Wed, Oct 30, 2013 at 5:12 PM, Laszlo Ersek wrote: >> So, I found no regressions in my usual environment (after fixing the >> ASSERT() with the attached patch). > > Does my current ovmf-nvvars branch also fix this issue? > https://github.com/jljusten/edk2.git ovmf-nvvars Yes, it does (at commit fdc7b5114ffc93cf6346c9e7ad3b854ddd57015d). I did a quick boot test on my RHEL-6 host with Fedora 19, RHEL 6, Windows 2012 R2, and Windows 2008 R2 guests; all seem fine. Thanks Laszlo -- November Webinars for C, C++, Fortran Developers Accelerate application performance with scalable programming models. Explore techniques for threading, error checking, porting, and tuning. Get the most from the latest Intel processors and coprocessors. See abstracts and register http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk ___ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
On Wed, Oct 30, 2013 at 5:12 PM, Laszlo Ersek wrote: > So, I found no regressions in my usual environment (after fixing the > ASSERT() with the attached patch). Does my current ovmf-nvvars branch also fix this issue? https://github.com/jljusten/edk2.git ovmf-nvvars I updated patch 3 "OvmfPkg: Add NV Variable storage within FD" to update PcdVariableStoreSize to 0xe000 as well. (PCDs are awesome, but they are also terrible.) I think the comment on this PCD in MdeModulePkg.dec may refer to MdeModulePkg/Universal/Variable/EmuRuntimeDxe. (Off topic: At one point I had hoped that OvmfPkg/EmuVariableFvbRuntimeDxe could eventually deprecate MdeModulePkg's EmuRuntimeDxe driver. Technically there is no reason it couldn't, but it doesn't seem likely to happen any time soon.) -Jordan -- November Webinars for C, C++, Fortran Developers Accelerate application performance with scalable programming models. Explore techniques for threading, error checking, porting, and tuning. Get the most from the latest Intel processors and coprocessors. See abstracts and register http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk ___ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
On Wed, Oct 30, 2013 at 5:12 PM, Laszlo Ersek 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. > > Yes. > > More verbosely, the previous logic does the following: > - finds the "bounding box" for MMIO regions, > - finds the maximum for non-MMIO regions (reserved / system RAM), > - removes the lower part of the "bounding box" that falls under the > non-MMIO maximum, > - and the remaining bounding box is exported in > \_SB.PCI0._CRS.DWORDMEMORY, ResourceProducer/PosDecode, meaning that > the root bridge passes accesses to addresses in the "bounding box" to > child devices. > >> >> Now we just search for the largest EfiGcdMemoryTypeMemoryMappedIo >> region. > > As far as I can see, the direct difference is: > - while previously the final "bounding box" could have holes (ie. > EfiGcdMemoryTypeNonExistent) inside, and it could cover several MMIO > regions, and it used to be above all non-MMIO ranges, > - now the range will be the largest one contiguous MMIO range, and it > can be anywhere. > > The new range may not be a subset of the former, clamped bounding box > (because a conventional RAM region could be located above it). It can be > both smaller (i) and greater (ii) than the previous bounding box. > > (i) the bounding box can cover several MMIO ranges (with nonexistents > in-between optionally) > (ii) possible eg. in the non-subset case > > So, I have no idea about the consequences, but exporting one contiguous > MMIO range in _CRS (with no nonexistents inside, ever) does seem clean. Hmm, I think you are pointing out some possible unintended side effects. I'm now thinking that perhaps I should just modify your code to lower the cap from 4GB down to just below the flash. I'll try this for a possible alternative to this patch for v2. -Jordan -- November Webinars for C, C++, Fortran Developers Accelerate application performance with scalable programming models. Explore techniques for threading, error checking, porting, and tuning. Get the most from the latest Intel processors and coprocessors. See abstracts and register http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk ___ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
On 10/31/13 18:53, Jordan Justen wrote:
> On Thu, Oct 31, 2013 at 2:40 AM, Laszlo Ersek 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
Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
On Thu, Oct 31, 2013 at 2:40 AM, Laszlo Ersek 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...
-Jordan
--
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
Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
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?
According to my testing with a 5GB guest, the patch lowers the top of
the MMIO range
- from 0xFEEF
- to 0xFBFF
The FDF files set PcdOvmfFdBaseAddress to 0xFFF0 (1MB build) or
0xFFE0 (2MB build). Then patch #6 marks the memory from there
upwards as runtime memory.
0xE000 == 3584 MB MMIO start (both new & old)
0xFBFF == 4032 MB - 1 BMMIO end (new)
0xFEEF == 4079 MB - 1 BMMIO end (old)
0xFFE0 == 4094 MB PcdOvmfFdBaseAddress (2MB build)
0xFFF0 == 4095 MB PcdOvmfFdBaseAddress (1MB build)
So there doesn't seem to be an overlap with or without this patch.
But, even if there were an overlap that the patch eliminates (ie. if
PcdOvmfFdBaseAddress fell between the new and old MMIO ends), shouldn't
this patch rather find the bounding box (like before), and clamp it down
explicitly with PcdOvmfFdBaseAddress?
Because, the largest MMIO range that patch #4 currently finds is
arbitrary ("random") AFAICT. PcdOvmfFdBaseAddress is also (sort of)
arbitrary. I have no idea if anything guarantees that the largest single
MMIO range won't intersect with PcdOvmfFdBaseAddress.
But, again, what if there's an intersection? We're going to report an
MMIO range to the guest kernel (via _CRS) part of which is actually
memory. Probably worth avoiding indeed.
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
Re: [edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
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.
Yes.
More verbosely, the previous logic does the following:
- finds the "bounding box" for MMIO regions,
- finds the maximum for non-MMIO regions (reserved / system RAM),
- removes the lower part of the "bounding box" that falls under the
non-MMIO maximum,
- and the remaining bounding box is exported in
\_SB.PCI0._CRS.DWORDMEMORY, ResourceProducer/PosDecode, meaning that
the root bridge passes accesses to addresses in the "bounding box" to
child devices.
>
> Now we just search for the largest EfiGcdMemoryTypeMemoryMappedIo
> region.
As far as I can see, the direct difference is:
- while previously the final "bounding box" could have holes (ie.
EfiGcdMemoryTypeNonExistent) inside, and it could cover several MMIO
regions, and it used to be above all non-MMIO ranges,
- now the range will be the largest one contiguous MMIO range, and it
can be anywhere.
The new range may not be a subset of the former, clamped bounding box
(because a conventional RAM region could be located above it). It can be
both smaller (i) and greater (ii) than the previous bounding box.
(i) the bounding box can cover several MMIO ranges (with nonexistents
in-between optionally)
(ii) possible eg. in the non-subset case
So, I have no idea about the consequences, but exporting one contiguous
MMIO range in _CRS (with no nonexistents inside, ever) does seem clean.
> 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.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen
> Cc: Laszlo Ersek
> ---
> OvmfPkg/AcpiPlatformDxe/Qemu.c | 37 -
> 1 file changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> index 8a6ecf7..cab3219 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> @@ -1,7 +1,7 @@
> /** @file
>OVMF ACPI QEMU support
>
> - Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.
> + Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.
>
>Copyright (C) 2012, Red Hat, Inc.
>
> @@ -232,16 +232,12 @@ PopulateFwData(
>
>Status = gDS->GetMemorySpaceMap (&NumDesc, &AllDesc);
>if (Status == EFI_SUCCESS) {
> -UINT64 NonMmio32MaxExclTop;
> -UINT64 Mmio32MinBase;
> -UINT64 Mmio32MaxExclTop;
> UINTN CurDesc;
> +CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Mmio32Desc;
>
> Status = EFI_UNSUPPORTED;
>
> -NonMmio32MaxExclTop = 0;
> -Mmio32MinBase = BASE_4GB;
> -Mmio32MaxExclTop = 0;
> +Mmio32Desc = NULL;
>
> for (CurDesc = 0; CurDesc < NumDesc; ++CurDesc) {
>CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc;
> @@ -253,21 +249,16 @@ PopulateFwData(
>if (ExclTop <= BASE_4GB) {
> 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;
> +if (Mmio32Desc == NULL ||
> +Desc->Length > Mmio32Desc->Length ||
> +(Desc->Length == Mmio32Desc->Length &&
> + Desc->BaseAddress > Mmio32Desc->BaseAddress)) {
> + Mmio32Desc = Desc;
> }
> break;
Seems to do what the commit message promises -- furthermore, it will
select the highest region if the largest size occurs at several bases.
>
> @@ -277,14 +268,10 @@ PopulateFwData(
>}
> }
>
> -if (Mmio32MinBase < NonMmio32MaxExclTop) {
> - Mmio32MinBase = NonMmio32MaxExclTop;
> -}
> -
> -if (Mmio32MinBase < Mmio32MaxExclTop) {
> - FwData->PciWindow32.Base = Mmio32MinBase;
> - FwData->PciWindow32.End= Mmio32MaxExclTop - 1;
> - FwData->PciWindow32.Length = Mmio32MaxExclTop - Mmio32MinBase;
> +if (Mmio32Desc != NULL) {
> + FwData->PciWindow32.Base = Mmio32Desc->BaseAddress;
> + FwData->PciWindow32.End= Mmio32Desc->BaseAddress +
> Mmio32Desc->Length - 1;
> + FwData->PciWindow32.Length = Mmio32Desc->Length;
>
>FwData->PciWindow64.Base = 0;
>FwData->PciWindow64.End= 0;
>
Reviewed-by: Laszlo Ersek
--o--
I also wanted to test the series at this patch (and compare the
results to when the series is not applied). The guest
[edk2] [PATCH 4/8] OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions
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.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jordan Justen
Cc: Laszlo Ersek
---
OvmfPkg/AcpiPlatformDxe/Qemu.c | 37 -
1 file changed, 12 insertions(+), 25 deletions(-)
diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
index 8a6ecf7..cab3219 100644
--- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
+++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
@@ -1,7 +1,7 @@
/** @file
OVMF ACPI QEMU support
- Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.
+ Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.
Copyright (C) 2012, Red Hat, Inc.
@@ -232,16 +232,12 @@ PopulateFwData(
Status = gDS->GetMemorySpaceMap (&NumDesc, &AllDesc);
if (Status == EFI_SUCCESS) {
-UINT64 NonMmio32MaxExclTop;
-UINT64 Mmio32MinBase;
-UINT64 Mmio32MaxExclTop;
UINTN CurDesc;
+CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Mmio32Desc;
Status = EFI_UNSUPPORTED;
-NonMmio32MaxExclTop = 0;
-Mmio32MinBase = BASE_4GB;
-Mmio32MaxExclTop = 0;
+Mmio32Desc = NULL;
for (CurDesc = 0; CurDesc < NumDesc; ++CurDesc) {
CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc;
@@ -253,21 +249,16 @@ PopulateFwData(
if (ExclTop <= BASE_4GB) {
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;
+if (Mmio32Desc == NULL ||
+Desc->Length > Mmio32Desc->Length ||
+(Desc->Length == Mmio32Desc->Length &&
+ Desc->BaseAddress > Mmio32Desc->BaseAddress)) {
+ Mmio32Desc = Desc;
}
break;
@@ -277,14 +268,10 @@ PopulateFwData(
}
}
-if (Mmio32MinBase < NonMmio32MaxExclTop) {
- Mmio32MinBase = NonMmio32MaxExclTop;
-}
-
-if (Mmio32MinBase < Mmio32MaxExclTop) {
- FwData->PciWindow32.Base = Mmio32MinBase;
- FwData->PciWindow32.End= Mmio32MaxExclTop - 1;
- FwData->PciWindow32.Length = Mmio32MaxExclTop - Mmio32MinBase;
+if (Mmio32Desc != NULL) {
+ FwData->PciWindow32.Base = Mmio32Desc->BaseAddress;
+ FwData->PciWindow32.End= Mmio32Desc->BaseAddress +
Mmio32Desc->Length - 1;
+ FwData->PciWindow32.Length = Mmio32Desc->Length;
FwData->PciWindow64.Base = 0;
FwData->PciWindow64.End= 0;
--
1.7.10.4
--
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
