| Message ID | 1427742834-24566-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 7 April 2015 at 14:59, Laszlo Ersek <lersek@redhat.com> wrote: > Hi Ard, > > first of all, sorry about the delayed review. > > So, this patch seems to be the only one in the series (according to both > the blurb and the per-patch file lists) that modifies the QEMU build. > > However, I think this patch has no effect. (Or, more politely put, I > don't know how it is supposed to work. :)) Namely: > > On 03/30/15 21:13, Ard Biesheuvel wrote: >> This updates ArmVirtualizationMemoryInitPeiLib so that the PEI memory >> region, i.e., the region that is used both before and after the MMU >> and caches are enabled, is invalidated by virtual address before >> enabling the MMU. >> >> This prevents issues where data we modified with the caches and MMU >> off may be shadowed by clean cachelines in system caches or in lower >> level caches on other CPUs, resulting in the this data to become >> invisible once we turn the MMU and caches on. >> >> Also reduce the size of the region to 16 MB (from 64 MB), to reduce >> the potential performance hit from invalidating the entire region by >> virtual address. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 6 ++++-- >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc | 4 ++-- >> .../ArmVirtualizationMemoryInitPeiLib.c | 10 ++++++++++ >> .../ArmVirtualizationMemoryInitPeiLib.inf | 1 + >> 4 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc >> index 07ae63ada7c0..0b9a46f0197f 100644 >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc >> @@ -39,6 +39,8 @@ >> ArmCpuLib|ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.inf >> >> [LibraryClasses.common] >> + MemoryInitPeiLib|ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> + > > This change sets the library class -> instance resolution for the > "MemoryInitPeiLib" class. However, that library class is not used at all > in the "PrePei" scenario (which the QEMU build falls under), only in the > "PrePi" (which the Xen build falls under). > > One way to see this is to enable the report file for the build: > > --report-file=build.report > > The report file lists the class2instance resolutions for all binaries. > The "MemoryInitPeiLib" class is entirely absent from the QEMU build's > report (and it is there in the Xen build's report). > > The QEMU build includes this "kind" of code differently -- it contains > "ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf" as a standalone PEIM, > which then comes together from: > - MemoryInitPeim.c > - MemoryInitPeiLib.c > > If you want to invoke the cache range invalidation (by virtual address > range) in the QEMU build as well, then you should modify > > ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > > (plus the adjacent INF file, accordingly). This should probably happen > in a separate patch. > > I don't know if such a change would be justified for physical hardware > platforms as well (because many of those use the same PEIM). In any > case, I think that a new FeaturePCD would be a good way to call the > cache invalidation conditionally. > Ah yes, I spotted this, and tested with it locally, but never sent the patch. Indeed, the MemoryInitPeiLib.c file should not be pulled in directly, imo, but via the MemoryInitPeiLib library instance. >> # Virtio Support >> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf >> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf >> @@ -101,8 +103,8 @@ >> gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000 >> gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000 >> >> - # Size of the region used by UEFI in permanent memory (Reserved 64MB) >> - gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000 >> + # Size of the region used by UEFI in permanent memory (Reserved 16MB) >> + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x01000000 >> > > We need to be real careful here. See the last ASSERT() in > ArmPlatformInitializeSystemMemory(), in file > > ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c > > My concern is that changing the size of the permanent PEI RAM should not > change the layout (ie. it should not be placed, due to the size change, > to an area that has different relationships with other areas). > > Hm.... Looking at "ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c", this > should be fine. Our flash (in the QEMU build) is near address 0x0, while > the DRAM starts at 1 GB (I remember that correctly, right?) > Indeed > So, the following branch should be taken, in InitializeMemory() -- our > flash takes place strictly "below" the DRAM: > > // Check the Firmware does not overlapped with the system memory > ASSERT ((FdBase < SystemMemoryBase) || (FdBase >= SystemMemoryTop)); > ASSERT ((FdTop <= SystemMemoryBase) || (FdTop > SystemMemoryTop)); > > UefiMemoryBase = SystemMemoryTop - FixedPcdGet32 > (PcdSystemMemoryUefiRegionSize); > > This places the permanent PEI RAM at the top of DRAM, while the DTB will > be at the bottom of the DRAM (at 1GB). The ASSERT() in > ArmPlatformInitializeSystemMemory() that I referenced above makes sure > that the permanent PEI RAM does not overlap with the initial DTB. > Correct. > ... So, this decrease from 64 MB to 16 MB seems safe. > Thanks for double checking > Summary: as far as I'm personally concerned, please split out a separate > patch for the QEMU build, patch > "ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c", and please make the > cache invalidation dependent on a new FeaturePCD (default FALSE, and > TRUE for QEMU). > > Clearly, we should hear what Olivier thinks too, because this is a > heavily shared module. The FeaturePCD should prevent regressions, but > stylistically another approach could be superior. > If we start using the MemoryInitPeiLib under ArmVirtualizationPkg for QEMU (which was intended by this patch), then there is no need IMO to introduce a feature PCD, since all virt platforms require the cache invalidation to guarantee correct operation (assuming that, under virtualization, you never have the guarantee that no system caches are enabled and you are never migrated to another cpu), and the physical platforms will keep using the original MemoryInitPeiLib, but as a library. So what I propose instead is to 'fix' the MemoryInitPeiLib,c dependency in a separate patch, so that the virt targets (including QEMU) will pull in the correct code. Cheers, Ard. >> # >> # ARM Pcds >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc >> index b24d0969e021..31ef5c43f44e 100644 >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc >> @@ -89,8 +89,8 @@ >> >> gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000 >> >> - # Size of the region used by UEFI in permanent memory (Reserved 64MB) >> - gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000 >> + # Size of the region used by UEFI in permanent memory (Reserved 16MB) >> + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x01000000 >> >> # >> # ARM Virtual Architectural Timer >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c >> index 5f6cd059c47f..8ce63b4596e2 100644 >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c >> @@ -20,6 +20,7 @@ >> #include <Library/HobLib.h> >> #include <Library/MemoryAllocationLib.h> >> #include <Library/PcdLib.h> >> +#include <Library/CacheMaintenanceLib.h> >> >> VOID >> BuildMemoryTypeInformationHob ( >> @@ -79,6 +80,15 @@ MemoryPeim ( >> PcdGet64 (PcdSystemMemorySize) >> ); >> >> + // >> + // When running under virtualization, the PI/UEFI memory region may be >> + // clean but not invalidated in system caches or in lower level caches >> + // on other CPUs. So invalidate the region by virtual address, to ensure >> + // that the contents we put there with the caches and MMU off will still >> + // be visible after turning them on. >> + // >> + InvalidateDataCacheRange ((VOID*)(UINTN)UefiMemoryBase, UefiMemorySize); >> + >> // Build Memory Allocation Hob >> InitMmu (); >> >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> index fcdae06de7c2..b8a19c993d91 100644 >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> @@ -35,6 +35,7 @@ >> HobLib >> ArmLib >> ArmPlatformLib >> + CacheMaintenanceLib >> >> [Guids] >> gEfiMemoryTypeInformationGuid >> > ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
On 7 April 2015 at 16:11, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/07/15 15:14, Ard Biesheuvel wrote: > >> If we start using the MemoryInitPeiLib under ArmVirtualizationPkg for >> QEMU (which was intended by this patch), > > Currently there is no executable module in the QEMU build that depends > on this library class. > This is because of the direct dependency on the .c file rather than the library class, right? >> then there is no need IMO to >> introduce a feature PCD, since all virt platforms require the cache >> invalidation to guarantee correct operation (assuming that, under >> virtualization, you never have the guarantee that no system caches are >> enabled and you are never migrated to another cpu), and the physical >> platforms will keep using the original MemoryInitPeiLib, but as a >> library. >> >> So what I propose instead is to 'fix' the MemoryInitPeiLib,c >> dependency in a separate patch, so that the virt targets (including >> QEMU) will pull in the correct code. > > Let me check if I understand: do you propose rebasing > > ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf > > to the MemoryInitPeiLib class explicitly? > > I guess that should work, but then we should also verify that the > following two files: > > ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c > > have no other differences than the cache invalidation (that is being > introduced by this patch of yours). Otherwise, simply switching the > class->instance resolution (without the cache invalidation) might cause > unintended changes for the QEMU build. > > Hm, indeed, that seems to be a problem. You introduced > "ArmVirtualizationMemoryInitPeiLib" in SVN r16962, and it seems pretty > strongly tied to PrePi / Xen. Under QEMU, the guest-phys address range > taken up by the pflash chip that carries the firmware binary never > becomes reusable by the OS. > Actually, I discussed this with Olivier, and it appears that the whole logic there is not needed in the first place. Perhaps Olivier can comment, but it seems removing the RAM is not necessary anymore for shadowed flash, and it is certainly not necessary for XIP flash such as what we use for the QEMU target. IOW, we could 1) fix the library dependency 2) switch the QEMU target to the current virt version 3) add the cache invalidation to it, so that both the QEMU and Xen targets benefit from it which would involve two additional patches. > So, as far as I can see, we need *three* flavors here: > - one for PrePi / Xen (including your SVN r16962 *and* the cache > invalidation) > - one for PrePei / Qemu (*not* including your SVN r16962, but > including cache invalidation) > - one for physical hardware (stock ArmPlatformPkg version). > > I guess -- unless I'm wrong of course -- we can do this with three > separate library instances, or by sticking with the current two, and > customizing the stock one with the FeaturePCD. > Let us figure out what's going on with MemoryInitPeiLib first, and then I'll propose an appropriate solution
diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc index 07ae63ada7c0..0b9a46f0197f 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc @@ -39,6 +39,8 @@ ArmCpuLib|ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.inf [LibraryClasses.common] + MemoryInitPeiLib|ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf + # Virtio Support VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf @@ -101,8 +103,8 @@ gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000 gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000 - # Size of the region used by UEFI in permanent memory (Reserved 64MB) - gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000 + # Size of the region used by UEFI in permanent memory (Reserved 16MB) + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x01000000 # # ARM Pcds diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc index b24d0969e021..31ef5c43f44e 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc @@ -89,8 +89,8 @@ gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000 - # Size of the region used by UEFI in permanent memory (Reserved 64MB) - gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000 + # Size of the region used by UEFI in permanent memory (Reserved 16MB) + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x01000000 # # ARM Virtual Architectural Timer diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c index 5f6cd059c47f..8ce63b4596e2 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c @@ -20,6 +20,7 @@ #include <Library/HobLib.h> #include <Library/MemoryAllocationLib.h> #include <Library/PcdLib.h> +#include <Library/CacheMaintenanceLib.h> VOID BuildMemoryTypeInformationHob ( @@ -79,6 +80,15 @@ MemoryPeim ( PcdGet64 (PcdSystemMemorySize) ); + // + // When running under virtualization, the PI/UEFI memory region may be + // clean but not invalidated in system caches or in lower level caches + // on other CPUs. So invalidate the region by virtual address, to ensure + // that the contents we put there with the caches and MMU off will still + // be visible after turning them on. + // + InvalidateDataCacheRange ((VOID*)(UINTN)UefiMemoryBase, UefiMemorySize); + // Build Memory Allocation Hob InitMmu (); diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf index fcdae06de7c2..b8a19c993d91 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf @@ -35,6 +35,7 @@ HobLib ArmLib ArmPlatformLib + CacheMaintenanceLib [Guids] gEfiMemoryTypeInformationGuid
This updates ArmVirtualizationMemoryInitPeiLib so that the PEI memory region, i.e., the region that is used both before and after the MMU and caches are enabled, is invalidated by virtual address before enabling the MMU. This prevents issues where data we modified with the caches and MMU off may be shadowed by clean cachelines in system caches or in lower level caches on other CPUs, resulting in the this data to become invisible once we turn the MMU and caches on. Also reduce the size of the region to 16 MB (from 64 MB), to reduce the potential performance hit from invalidating the entire region by virtual address. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 6 ++++-- ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc | 4 ++-- .../ArmVirtualizationMemoryInitPeiLib.c | 10 ++++++++++ .../ArmVirtualizationMemoryInitPeiLib.inf | 1 + 4 files changed, 17 insertions(+), 4 deletions(-)