From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 685B17803CC for ; Tue, 19 Mar 2024 02:04:11 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=5oI83SgbpoaSLw2u00Cykb7JjNFnXIcVH6VaTwfHh1g=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1710813850; v=1; b=CdH5UCo0NB4cWXSmjlI35sZFKziNBmN/RKHPtoqj1Gu2LRhC8JAdLCMMoPvTQMjrZxdlk70W WulClPTI6Hfd/lYlg1ThgEed9cJSKRwV7k3/tn7AcRdGeuoxMpdjOAPkthqhFTBpVUuqeywU7ox GUZQua9emPqZfPL88KahGrWemSQrYL5pugfiyYhVgQpCVKO1/r4r4i9WVBGa/N6qPKTl7lhVNau H6nuPYbi1ZJln3EUiSeMMLr55SQeYBjrCGPy5KwFw8hGMn7PcC1pvZ/FuD7OKKRXUoazZNxQJBF b80g6i0itlv2LSIGb3tnAXKeIHZmt2kVGPFiwtWa0sTRA== X-Received: by 127.0.0.2 with SMTP id iFJqYY7687511xaDpYTzOidg; Mon, 18 Mar 2024 19:04:10 -0700 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.5481.1710813848126633610 for ; Mon, 18 Mar 2024 19:04:08 -0700 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8BxieiS8vhl1ZUaAA--.44625S3; Tue, 19 Mar 2024 10:04:02 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8AxX8+R8vhlQAVdAA--.47181S3; Tue, 19 Mar 2024 10:04:01 +0800 (CST) Message-ID: Date: Tue, 19 Mar 2024 10:04:01 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v1 20/26] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib To: Gerd Hoffmann , lixianglai Cc: devel@edk2.groups.io, Ard Biesheuvel , Jiewen Yao , Jordan Justen , Bibo Mao , Dongyan Qian References: <20240311093631.1251466-1-lichao@loongson.cn> <20240311093919.1254515-1-lichao@loongson.cn> <7nqe7k3oi3cph7mhqc4t5ea7qair3u2i7dy6oli6wurovyaoqa@apkw6d7gneam> <51c896fa-60bb-127f-c346-dc69179d288f@loongson.cn> From: "Chao Li" In-Reply-To: X-CM-TRANSID: AQAAf8AxX8+R8vhlQAVdAA--.47181S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQABCGX3+2cEVwABsU X-Coremail-Antispam: 1Uk129KBjDUn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7 ZEXasCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnUUvcSsGvfC2Kfnx nUUI43ZEXa7xR_UUUUUUUUU== Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Mon, 18 Mar 2024 19:04:09 -0700 Reply-To: devel@edk2.groups.io,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: foAKDZVKKChp1NtzOt3MtA83x7686176AA= Content-Type: multipart/alternative; boundary="------------qyh16vc0WbAic6Ij1RJN0Jq3" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=CdH5UCo0; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------qyh16vc0WbAic6Ij1RJN0Jq3 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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 impl= ementation, >> 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 VirtNo= rFlashDxe. >> And It requires the PcdFlashNvStorageFtwWorkingSize and PcdFlashNvStorag= eFtwSpareSize variables for initialization, >> However the initialization of these two variables is completed in VirtNo= rFlashDxe, >> The fdf file specifies that VirtNorFlashDxe is loaded first and then Fau= ltTolerantWriteDxe is loaded in loongarch64. >> So this is going to be a problem if we want to apply the current solutio= n 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 { > A > [ ... ] > NULL|EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormatte= dLib.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=20 this change under the OvmfPkg/Library as a public library. And I will=20 enable it in aarch64 after merging this change, because I think it may=20 be tweaked and validated in aarch64 for many platforms. Do you think=20 that is good? > > take care, > Gerd -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --------------qyh16vc0WbAic6Ij1RJN0Jq3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

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 t=
o 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 impleme=
ntation,
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 th=
e current patch scheme on aarch64,
I found that the FaultTolerantWriteDxe driver loaded earlier than VirtNorFl=
ashDxe.
And It requires the PcdFlashNvStorageFtwWorkingSize and PcdFlashNvStorageFt=
wSpareSize variables for initialization,
However the initialization of these two variables is completed in VirtNorFl=
ashDxe,
The fdf file specifies that VirtNorFlashDxe is loaded first and then FaultT=
olerantWriteDxe is loaded in loongarch64.
So this is going to be a problem if we want to apply the current solution t=
o 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/NvVarStoreFormattedLi=
b.inf
      [ ... ]
  }

... to make sure VariableRuntimeDxe is scheduled after VirtNorFlashDxe.

I think you can apply the same idea to FaultTolerantWriteDxe.


I can't tell the implementat=
ion 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:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#116875) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------qyh16vc0WbAic6Ij1RJN0Jq3--