Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

["Gerd Hoffmann" <kraxel@redhat.com>]

On Thu, Mar 21, 2024 at 08:39:13AM +0000, sunceping wrote:
> On Wednesday, March 20, 2024 6:05 PM Gerd Hoffmann wrote:
> > 
> > We don't need to read + cache all fw_cfg data.  We only need to cache the
> > entries which (a) must be measured, and (b) will not be measured in some
> > other way.
> > 
> I am afraid that it is difficult to determine which fw_cfg items 
> are need to be cached and measured, since there are some fw_cfg items are
> optional with special qemu cmd.
> Example : 
> fw_cfg item: opt/ovmf/X-PciMmio64Mb  
> depends on qemu command: "-fw_cfg name=opt/ovmf/X-PciMmio64Mb,string=N"

Well, just try to read them.  If present they can just be measured.
If not present we can either skip them, or measure with an empty data
field to indicate it is not present.

> > > Further more this is not a lazy mode which means some fw_cfg data may
> > > be read but it is not to be consumed later.
> > 
> > The problem with lazy mode is that the measurement order is not fixed.
> We'd better define what's the "fixed measurement order".
> Think about such scenario:
> There are 5 fw_cfg items ("A-B-C-D-E"), and C is an optional one which depends on special qemu cmd
>  (example : etc/boot-menu-wait depends on qemu command: " -boot menu=on,splash-time=N").
> 	
> So, there are two measurement orders in different qemu command:
> 1, "A-B-C-D-E" (with qemu command "-boot menu=on,splash-time=N ")
> 2, "A-B-D-E" (without qemu command "-boot menu=on,splash-time=N ") 
> 
> Is the "A-B-C-D-E" or "A-B-D-E"  fixed order ? 
> 
> What's your thoughts ?

See above for optional items.

I'm more concerned about reordering.  We have a number of DXE modules
reading fw_cfg entries.  The order the modules are scheduled depends
multiple factors (order in the firmware volume, depex), so that order
may change.

With lazy loading and measurement changes in the initialization order
would also change the measurements even if the content of the fw_cfg
items files is identical, because the items are measured in B-A instead
of A-B order.

> > > We propose below solution :
> > >
> > > Add a new API in QemuFwCfgLib,
> > > RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value,
> > FW_CFG_GET_DATA_FLAG flag).
> > 
> > I'd suggest to *not* touch the existing interfaces for reading entries.
> > Instead change the existing functions to first check the cache, in case there is
> > no cache entry go read from fw_cfg.
> Actually we don't touch the existing interface for reading entries.
> The API is newly added.

But then you have to find and update all callsites (or at least the
ones where we care about measurement).

> > Add a new interface for adding fw_cfg entries to the cache, with optional
> > measurement.
> Do you mean to add a new API (example : QemuFwCfgReadBytes2(*size, *buffer, Flag))
>  with optional measurement to cache fw_cfg data?

More like this:

QemuFwCfgCacheAdd(int Item, int Size, bool Measure);

> We can update it like below:
>   QemuFwCfgGetData("etc/e820", &FwCfgSize,  Buffer, Flag);
>   for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { 
>     EFI_E820_ENTRY64 *E820Entry = (EFI_E820_ENTRY64 *)(Buffer + Processed);
>     Callback (E820Entry, PlatformInfoHob);
>   }

Well, not that easy.  Allocating memory is not always possible in early
firmware phases, which is the reason why the code works with the stack
and chunkwise reads.

We might offer an QemuFwCfgCacheGet() API though.  When storing cached
items in HOBs we can hand out a pointer to the data in the HOB, and
callers access the complete fw_cfg item then without having to allocate
memory.

> > Also note that not all fw_cfg entries are (pseudo-)files.
> I am not sure what you mean about (pseudo-)files.

Well, even though fw_cfg uses names which look like file paths for
entries it's not a real filesystem, it's just a naming convention,
that's why named them pseudo files.

Some older fw_cfg items don't use this file-naming scheme, they have a
fixed number instead (see QemuFwCfgItem*).

> Base on your comments,  does this mean there are other fw_cfg data 
> which are read from QEMU via different method (not QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) ?

It all comes down to QemuFwCfgSelectItem + QemuFwCfgReadBytes.  There
are some convenience functions to read (for example) integers, they
internally call QemuFwCfgReadBytes though.  QemuFwCfgFindFile also
simply calls QemuFwCfgSelectItem + QemuFwCfgReadBytes.

So making QemuFwCfgSelectItem + QemuFwCfgReadBytes aware of the cache
should be all you need.

take care,
  Gerd



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