Re: [edk2-devel] [PATCH v1 20/26] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 3556 bytes --]

Hi Gerd,


Thanks,
Chao
On 2024/3/18 23:21, Gerd Hoffmann wrote:
> On Sat, Mar 16, 2024 at 06:19:00PM +0800, lixianglai wrote:
>> Hi Gerd:
>>> On Mon, Mar 11, 2024 at 02:39:24AM -0700, Chao Li wrote:
>>>> Add NorFlashQemuLib for LoongArch, it is referenced from ArmVirtPkg.
>>> What are the differences to the ArmVirtPkg version?
>> In this lib we have assigned the following three pcd variables:
>> PcdFlashNvStorageVariableBase
>> PcdFlashNvStorageFtwWorkingBase
>> PcdFlashNvStorageFtwSpareBase
>> Instead of hardcoding these three variables in the VarStore.fdf.inc file as arm does,
>> the benefit is that when the flash base address changes in the qemu implementation,
>> there is no need to re-adapt and compile UEFI.
> The flash memory layout (address + size) for the aarch64 virt machine
> has never changed.  So while it sounds nice in theory to have that
> option it could very well be that this will never ever needed in
> practice.
>
> Having sayed that I'd also note that I think it should also be possible
> to switch the aarch64 builds to set the PCDs at runtime instead of
> compile time.
>
>> When I tried to implement the current patch scheme on aarch64,
>> I found that the FaultTolerantWriteDxe driver loaded earlier than VirtNorFlashDxe.
>> And It requires the PcdFlashNvStorageFtwWorkingSize and PcdFlashNvStorageFtwSpareSize variables for initialization,
>> However the initialization of these two variables is completed in VirtNorFlashDxe,
>> The fdf file specifies that VirtNorFlashDxe is loaded first and then FaultTolerantWriteDxe is loaded in loongarch64.
>> So this is going to be a problem if we want to apply the current solution to aarch64 or risc-v.
> There is a non-obvious twist:
>
> VirtNorFlashDxe registers the gEdkiiNvVarStoreFormattedGuid protocol.
>
> There is the
> EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
> library, which only purpose is to add a dependency to
> gEdkiiNvVarStoreFormattedGuid to depex.
>
> NvVarStoreFormattedLib.inf is used this way ...
>
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>      <LibraryClasses>A
>        [ ... ]
>        NULL|EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
>        [ ... ]
>    }
>
> ... to make sure VariableRuntimeDxe is scheduled after VirtNorFlashDxe.
>
> I think you can apply the same idea to FaultTolerantWriteDxe.
>
>> I can't tell the implementation scheme of the current lib and existing
>> lib implementation scheme which one is better, Could you give we some
>> advice?
> I'd suggest to merge your code as OvmfPkg/Library/FdtNorFlashQemuLib as
> it is not really loongarch-specific.
>
> If you want try switch aarch64 to use the same code that'll be great,
> but sorting that out later is also fine with me.

If you think this design is looks better, then I'm prepare to commit 
this change under the OvmfPkg/Library as a public library. And I will 
enable it in aarch64 after merging this change, because I think it may 
be tweaked and validated in aarch64 for many platforms. Do you think 
that is good?

>
> take care,
>    Gerd


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116875): https://edk2.groups.io/g/devel/message/116875
Mute This Topic: https://groups.io/mt/104859896/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 5541 bytes --]