From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 702A47803D0 for ; Thu, 25 Apr 2024 09:23:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=8vkNuDZh9ckyDWlvyvuqf6r/K64OBOeVxpkKLEZeBbE=; 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:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1714037009; v=1; b=epvf1YldJ55+MzMOgsofTsHncp9e0MJ98reo+u7k4zyaw3k4wKOtE2PFnb99oDMk8TCbKvmr EuJxPAOgfM8gYUiW3GshJMrQ5bUAGwffowV90yf0D1FBlsVQG/7bE9tHAd1mjyU8n+WFGL+m5PS 3drVBhOSFUDfXyHMass4/kxEgdaeYY9JDKi5yTKjVOZHAWzEPOAbfHABMU3bNNyzuZEXU4bT8jX p3hMtJD/EmwU1GrzX46AplYN3ko8s/ipEnzJPUTQUIj4OrzsbavK9yMOFPdGtqq/jcsH9TFqQn0 UjpqgS3Vx9WHEdkpA1nCQtB9nzoLpkdAroaRsDKC+/Zsw== X-Received: by 127.0.0.2 with SMTP id wT9OYY7687511xqyQxPIRq3H; Thu, 25 Apr 2024 02:23:29 -0700 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.12669.1714037005824513497 for ; Thu, 25 Apr 2024 02:23:27 -0700 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8Dxd_AHISpmHrsCAA--.13083S3; Thu, 25 Apr 2024 17:23:19 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8CxrlcFISpmGvAEAA--.12753S3; Thu, 25 Apr 2024 17:23:17 +0800 (CST) Message-ID: Date: Thu, 25 Apr 2024 17:23:17 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v2 3/7] OvmfPkg: Add the QemuFwCfgMmioLib PEI stage version To: devel@edk2.groups.io, kraxel@redhat.com Cc: Ard Biesheuvel , Jiewen Yao , Xianglai Li References: <20240425041728.1385891-1-lichao@loongson.cn> <20240425041821.1386403-1-lichao@loongson.cn> <3panc52jjcnvf7cml7wllbqirsr2bjnvn34ulonxp5jkz7hj5h@2y6deto2c5fp> From: "Chao Li" In-Reply-To: X-CM-TRANSID: AQAAf8CxrlcFISpmGvAEAA--.12753S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQASCGYpxJMEnwAEsc 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: Thu, 25 Apr 2024 02:23:27 -0700 Resent-From: lichao@loongson.cn Reply-To: devel@edk2.groups.io,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: hfyPZ2DEGQphSwPH4KUZH4bRx7686176AA= Content-Type: multipart/alternative; boundary="------------ukFNs5bFWyFRcsfea638gzeI" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=epvf1Yld; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io --------------ukFNs5bFWyFRcsfea638gzeI Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Gerd, I get it, I will refactor the code as soon as I can, it looks like=20 there's still some work and will take some time. I will try to send the=20 V3 tonight if possible. Thanks, Chao On 2024/4/25 17:02, Gerd Hoffmann wrote: > On Thu, Apr 25, 2024 at 04:06:13PM +0800, Chao Li wrote: >> Hi Gerd, >> >> >> Thanks, >> Chao >> On 2024/4/25 15:53, Gerd Hoffmann wrote: >>> Hi, >>> >>>> +UINTN mFwCfgSelectorAddress; >>>> +UINTN mFwCfgDataAddress; >>>> +UINTN mFwCfgDmaAddress; >>> Hmm, global variables for PEI? I think the point of storing these in >>> the HOB is to avoid the need for global variables? Also does that work >>> when running PEI in-place from flash? >> I think it would be useful if some platforms(not LoongArch) could use th= e >> global variables in PEI, because the global variables are faster. > Performance isn't my main concern here, I very much prefer code which is > easy to maintain. Taking the same code path on all platforms is good > for that. It's less code and it also makes testing easier. The risk of > breaking loongarch when changing something for riscv or arm is much > lower if all platforms work the same way. > > I'd suggest to first refactor the existing DXE code to use a HOB instead > of global variables. Have a helper function which looks up the HOB and > returns a pointer to the configuration struct. That helper function can > be slightly different for DXE/PEI, the DXE variant can cache the pointer > to the struct in a global variable so it needs to do the lookup only > once. > >>>> + UINT64 FwCfgDataSize; >>>> + UINT64 FwCfgDmaAddress; >>>> + UINT64 FwCfgDmaSize; >>> First thing this function should do is check whenever the HOB already >>> exists. Should that be the case there is no need to parse the device >>> tree. >> This is a constructor in PEI, that has to parse the device tree and then >> build the HOBs. > This is a library, so it can be linked into multiple PEI and DXE > modules. So it must be prepared to run multiple times. On the > second and all following runs the HOB will already exist. > > The DXE variant will need the check for sure. I'd strongly suggest to > add it to the PEI variant too, even though it might not be needed right > now because PlatformPei is the only PEI module using the library. > > take care, > Gerd > > > >=20 > -=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 (#118283): https://edk2.groups.io/g/devel/message/118283 Mute This Topic: https://groups.io/mt/105724970/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- --------------ukFNs5bFWyFRcsfea638gzeI Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Gerd,

I get it, I will refactor the code as soon as I can, it looks like there's still some work and will take some time. I will try to send the V3 tonight if possible.


=
Thanks,
Chao
On 2024/4/25 17:02, Gerd Hoffmann wrote:
On Thu, Apr 25, 2024 at 04:06:=
13PM +0800, Chao Li wrote:

Hi Gerd,


Thanks,
Chao
On 2024/4/25 15:53, Gerd Hoffmann wrote:

   Hi,


+UINTN  mFwCfgSelectorAd=
dress;
+UINTN  mFwCfgDataAddress;
+UINTN  mFwCfgDmaAddress;

Hmm, global variables for =
PEI?  I think the point of storing these in
the HOB is to avoid the need for global variables?  Also does that work
when running PEI in-place from flash?

I think it would be useful i=
f some platforms(not LoongArch) could use the
global variables in PEI, because the global variables are faster.

Performance isn't my main concern here, I very much prefer code which is
easy to maintain.  Taking the same code path on all platforms is good
for that.  It's less code and it also makes testing easier.  The risk of
breaking loongarch when changing something for riscv or arm is much
lower if all platforms work the same way.

I'd suggest to first refactor the existing DXE code to use a HOB instead
of global variables.  Have a helper function which looks up the HOB and
returns a pointer to the configuration struct.  That helper function can
be slightly different for DXE/PEI, the DXE variant can cache the pointer
to the struct in a global variable so it needs to do the lookup only
once.


+  UINT64        FwCfgDa=
taSize;
+  UINT64        FwCfgDmaAddress;
+  UINT64        FwCfgDmaSize;

First thing this function =
should do is check whenever the HOB already
exists.  Should that be the case there is no need to parse the device
tree.

This is a constructor in PEI=
, that has to parse the device tree and then
build the HOBs.

This is a library, so it can be linked into multiple PEI and DXE
modules.  So it must be prepared to run multiple times.  On the
second and all following runs the HOB will already exist.

The DXE variant will need the check for sure.  I'd strongly suggest to
add it to the PEI variant too, even though it might not be needed right
now because PlatformPei is the only PEI module using the library.

take care,
  Gerd
_._,_._,_
Groups.io Links:

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

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

_._,_._,_
--------------ukFNs5bFWyFRcsfea638gzeI--