public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Gerd Hoffmann" <kraxel@redhat.com>
To: Chao Li <lichao@loongson.cn>
Cc: devel@edk2.groups.io, Ard Biesheuvel <ardb+tianocore@kernel.org>,
	 Jiewen Yao <jiewen.yao@intel.com>,
	Xianglai Li <lixianglai@loongson.cn>
Subject: Re: [edk2-devel] [PATCH v2 3/7] OvmfPkg: Add the QemuFwCfgMmioLib PEI stage version
Date: Thu, 25 Apr 2024 11:02:01 +0200	[thread overview]
Message-ID: <bhf3uqf2yitnp2tdzlvm5uj3rewl6qxi554bll4qwsts7ih5d7@pzestopzezzt> (raw)
In-Reply-To: <a8ae0328-10a9-413c-98e1-8d73c60eb6d3@loongson.cn>

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 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        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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118282): https://edk2.groups.io/g/devel/message/118282
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]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-04-25  9:02 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-25  4:17 [edk2-devel] [PATCH v2 0/7] Adjust the QemuFwCfgLibMmio and add PEI stage Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 1/7] OvmfPkg: Separate QemuFwCfgLibMmio.c into two files Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 2/7] OvmfPkg: Add the way of HOBs in QemuFwCfgLibMmio Chao Li
2024-04-25  7:40   ` Gerd Hoffmann
2024-04-25  8:09     ` Chao Li
2024-04-25  8:11       ` Ard Biesheuvel
2024-04-25  8:16         ` Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 3/7] OvmfPkg: Add the QemuFwCfgMmioLib PEI stage version Chao Li
2024-04-25  7:53   ` Gerd Hoffmann
2024-04-25  8:06     ` Chao Li
2024-04-25  9:02       ` Gerd Hoffmann [this message]
2024-04-25  9:23         ` Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 4/7] OvmfPkg: Copy the same new INF as QemuFwCfgLibMmio.inf Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 5/7] ArmVirtPkg: Enable QemuFwCfgMmioDxeLib.inf Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 6/7] OvmfPkg/RiscVVirt: " Chao Li
2024-04-25  4:18 ` [edk2-devel] [PATCH v2 7/7] OvmfPkg: Remove QemuFwCfgLibMmio.inf Chao Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bhf3uqf2yitnp2tdzlvm5uj3rewl6qxi554bll4qwsts7ih5d7@pzestopzezzt \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox