Hi Olivier,
On Mon, Jan 13, 2014 at 11:16 AM, Olivier Martin olivier.martin@arm.com wrote:
Hi Roy,
I need to think more about it. And I also need your point of view on some points.
Here are few questions and thoughts:
- Do you think the flash topology is the only difference between qEmu and RTSM? I believe it is not the only difference. I think the Cortex A15 used to not support the Security extension in qEmu. IS it still the case?
QEMU reports that it does implement security extensions, but only partially does. As part of supporting UEFI VBAR register support was added to QEMU. Other users had posted patches as they wanted this as well, and these patches got merged since UEFI needed them as well. I am sure that there are many remaining differences between RTSM and QEMU, but only a few matter to UEFI, and I think I have addressed all of them. (some by changing QEMU, and some by changing UEFI.)
- The correct approach when you have flash with 2 regions (small and large block size regions) is to use the fine grain for the UEFI variables (faster accesses in NVM). Using the large grain when the fine grain is available would not be appropriate. But at least it would ease the support between qEmu and RTSM.
I agree that in general using the smaller blocks is preferable, but I think moving them to the larger blocks supported by both models is a reasonable compromise to allow a single binary to boot on both.
- In the past the RTSM team told me that the goal of RTSM was to be able to run the same binary you would run on your HW onto the model (RTSM). But it is actually not the case with RTSM A15 (its HW equivalent should be the ARM Test Chip ver1 (TC1)) as the HW and RTSM memory map are different.
There are two solutions:
- either I accept your patch - that will ease the maintenance (no difference between qEmu and RTSM support)
- or I reject your patch. And I ask you to submit a new version that would introduce a build flag (eg: SUPPORT_QEMU) to differentiate RTSM and qEmu settings in Tianocore.
I have to admit I prefer the solution 2) - but I am quite open to any valid arguments. My argument is I would prefer to expose the correct implementation when possible. And if qEmu adds support for the missing VExpress IP block in the future then it will be easier to restore the correct approach (ie: the default RTSM approach) for qEmu. I don't think that the flash will every be fixed in QEMU, since accurate modeling of flash writing is not all that valuable of a feature.
I can add a compile option for the flash, as I have done for the networking. The networking required this due to conflicting ethernet devices. (And here QEMU matches real hardware, and it is RTSM that is 'wrong'.) I think that the value of a common binary for a common case is worth the cost of moving the variable storage to larger blocks, but if you remain unconvinced I'll resubmit the patch with a build option :) (and in that case I will also resubmit the networking patch so the same build option is used for both QEMU related changes.)
Roy
Thanks, Olivier
-----Original Message----- From: Roy Franz [mailto:roy.franz@linaro.org] Sent: 10 January 2014 16:11 To: ryan.harkin@linaro.org; Olivier Martin Cc: edk2-devel@lists.sourceforge.net; linaro-uefi; Patch Tracking Subject: Re: [PATCH V2] Move RTSM VExpress variable storage to 256k flash blocks
Hi Olivier - Any thoughts on this?
Thanks, Roy
On Thu, Dec 19, 2013 at 7:40 AM, Ryan Harkin ryan.harkin@linaro.org wrote:
I just wanted to confirm that this works in the 13.12 release and in
my own
builds from the latest tree.
Thanks Roy!
On 12 December 2013 22:08, Roy Franz roy.franz@linaro.org wrote:
Change the addresses/sizes of the variable storage areas to use 256k blocks so UEFI is compatible with both the RTSM models and QEMU.
The VExpress flash has non-uniform block sizes, with most blocks
being
256k and the top 4 blocks being 64k. UEFI has been using these top
64k
blocks for persistent variable storage. The RTSM models the non-
uniform
sizes, while QEMU only supports emulating flash with uniform block
sizes
which results in the top 256k (the 4 64k blocks) of flash being
unusable
for writing in QEMU. The ARM UEFI NOR flash driver currently
requires
that firmware volumes start at the base of a flash region, so the variables are now stored at the base the region that consists of the 256k blocks. It was previously at the base of the region of 64k blocks.
Note that this change will require RTSM flash images to be updated,
as
the variable storage has moved. Currently only the A15 model is
supported
by QEMU RTSM VExpress configurations. This patch only changes the A15 configurations.
Signed-off-by: Roy Franz roy.franz@linaro.org Contributed-under: TianoCore Contribution Agreement 1.0
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc | 12 ++++++------ .../ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc index 2d12f4b..c0196d9 100644 --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc @@ -77,12 +77,12 @@ # # NV Storage PCDs. Use base of 0x0C000000 for NOR1 #
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FFC0000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FFD00 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x000100 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FFE0000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0C000000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00040000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0C0400 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x000400 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0C080000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00040000
gArmTokenSpaceGuid.PcdVFPEnabled|1
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-
A15_MPCore.dsc
b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc index efd80ab..69088ff 100644 --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc @@ -79,12 +79,12 @@ # # NV Storage PCDs. Use base of 0x0C000000 for NOR1 #
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FFC0000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FFD00 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x000100 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FFE0000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0C000000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00040000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0C0400 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x000400 00
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0C080000
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00040000
gArmTokenSpaceGuid.PcdVFPEnabled|1
-- 1.7.10.4
-----Original Message----- From: Roy Franz [mailto:roy.franz@linaro.org] Sent: 14 January 2014 01:31 To: Olivier Martin Cc: ryan.harkin@linaro.org; edk2-devel@lists.sourceforge.net; linaro- uefi; Patch Tracking Subject: Re: [PATCH V2] Move RTSM VExpress variable storage to 256k flash blocks
I have to admit I prefer the solution 2) - but I am quite open to any
valid arguments. My argument is I would prefer to expose the correct implementation when possible. And if qEmu adds support for the missing VExpress IP block in the future then it will be easier to restore the correct approach (ie: the default RTSM approach) for qEmu. I don't think that the flash will every be fixed in QEMU, since accurate modeling of flash writing is not all that valuable of a feature.
I can add a compile option for the flash, as I have done for the networking. The networking required this due to conflicting ethernet devices. (And here QEMU matches real hardware, and it is RTSM that is 'wrong'.) I think that the value of a common binary for a common case is worth the cost of moving the variable storage to larger blocks, but if you remain unconvinced I'll resubmit the patch with a build option :) (and in that case I will also resubmit the networking patch so the same build option is used for both QEMU related changes.)
Nice try! but I remain unconvinced :-)
From what you said, we will need a compiler flag for the Ethernet driver anyway. So let's go for it :-)
And it will quite easy to 'grep' for SUPPORT_QEMU to highlight the difference between the different models than to go through the git history to find out the workaround which has been made to use UEFI on qEmu.
-
Olivier Martin -
Roy Franz