[PATCH] efi_loader: FMP cleanups

[AKASHI Takahiro]

On Tue, Jun 15, 2021 at 09:56:10AM +0300, Ilias Apalodimas wrote:
> > > > 
> 
> [...]
> 
> > > > They are fixing "different" problems relating ESRT generation.
> > > > That is my point.
> > > > 
> > > 
> > > Sure, but it's a minor clean up really. As I said the current code works
> > > fine.  So I dont really mind the fact that it breaks a sentence of the spec.
> > > Hence I considered the cleanup and the mutual exclusive part to be really
> > > minor. 
> > 
> > Yes, it's minor but still a different problem.
> > Let me give you an example.
> > If I correct a misspelling in a given code
> > very close to the change, Heinrich would ask
> > me to add a separate patch as it is simply not
> > related.
> > 
> > Moreover, from the viewpoint of maintenance (i.e. bisect ability),
> > they should be separated from each other.
> > 
> > > > > 
> > > > > > 
> > > > > Is there anything very specific that you can achieve with FIT capsules that
> 
> [...]
> 
> > > > > you can't achieve with RAW ones (or vice versa), that would justify having
> > > > > them both present at the same time?
> > > > 
> > > > Yes.
> > > > We may have different *firmware* for different software components
> > > > and different devices. For example,
> > > > You have firmare like U-Boot binary and default variable storage
> > > > in different partitions.
> > > > On the other hand, you have an extra firmware for a particular
> > > > peripheral, like PCI device or anything else, which comes
> > > > from a 3rd party vendor of the device.
> > > > The former may and can be packed into a single binary in FIT format.
> > > > The latter can be used in a separate RAW format as the timing of
> > > > updating those firmware is likely to be different.
> > > > 
> > > 
> > > Sure that's a use case. But that's not a specific one, nor something you cant
> > > do without both of them being installed.  You can arguably just create a RAW
> > > image for the second firmware and put the info into dfu_alt_info.
> > 
> > Why do you stick to a single format?
> 
> I think it's the other way around. Why wouldn't you? It's the easiest and
> sanest thing to do when generating capsules.

I don't know why you think it's the easiest.
"mkeficapsule" can create any format of capsule file.
(This is not true in the current form. but it's quite easy
to enhance this command for this purpose.)

> > We can reasonably assume that each FMP may
> > have a different format.
> > I think it's a very natural thing.
> 
> The FMPs logic in the EFI spec is not tied to 'format', it's tied to 'device'
> and currently both FMPs target the same device.

the same device? What do you mean by 'same device'?
I probably won't agree.

> So my understanding is, that
> in order to use it you need to:
> 1. Create 2 capsules, 1 raw, 1 fmp. 
> 2. Set dfu_alt_info -> process RAW capsule.

What do you mean by "process"?

> 3. Set dfu_alt_info to something different -> process FIT capsule.

No. What you need to do is to *add* definition for firmware
in FIT. We can have a single definition of dfu_alt_info
even if we use both FIT and RAW FMPs.

> and by doing so the ESRTs will use one of the information found in
> dfu_alt_info.

So what?

> 
> > 
> > > So unless we 
> > > have an example of a device that says "This firmware file can only be updated 
> > > by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't
> > > see any reason why we need to support that.  The changes are not set in stone
> > > anyway.  The code was fine before the ESRT got involved.  So all my patch
> > > really does is make the current code useful when an ESRT is installed. We can
> > > then break the spec on purpose (yes break it :>) ignore the OsIndications
> > > bit and have fwupd working with U-Boot.  This will have an actual impact on
> > > devices and the code usability, since people will start using it.  I prefer
> > > this over adding a very cumbersome corner case, that's arguably no one will
> > > ever need.
> > > We can always go back and  make them a config option in the future.  But unless 
> > > we get a use case for it, I'd still prefer having them  mutually exclusive, 
> > > rather than adding code for an imaginary device (which I really doubt anyone 
> > > will ever design).
> > 
> > I don't think that the example I gave is a imaginary device.
> > 
> 
> All of the devices I've tested and seen up to now are working fine with just
> RAW capsules installed and I can't understand why a specific *format* should
> play a role in the capsule creation. 

I don't get your point.
I never said, "a specific format should play a role."

> FITs are a nice way to get authentication and bundle things without having the
> EFI capsule authentication code, but really apart from that those 2 are doing
> the same thing.

Yes, but why do we have to have the restriction?
Different capsule files for different firmware/device may
come from different owners of the firmware.

> [...]
> > > The ESRT code right now uses get_image_info from the FMP code and the FMP code
> > > uses the dfu_alt_info to derive whatever information it needs.  Both of these
> > > concepts are trying to provide information about the running firmware.  So if
> > > we change that imho both of them should get that info from an abstracted
> > > object (file/c struct in u-boot/whatever). But really I think using FMP to
> > > fill ESRT entries is fine (at least for me).
> > 
> > Well, dfu_alt_info can already be seen as abstracted object
> > in terms of FMP.
> 
> Yes but it can't handle the ESRT generation properly.  So if you change that,
> why leave the FMPs get_image_info, read the information differently?

Again, I don't get your point.

-Takahiro Akashi

> > > 
> [...]
> > > Yea me neither, but since the firmware runtime information are derived from
> > > that, we don't have that many options.
> > 
> > What do you mean by "options"?
> 
> I don't see any point of trating .get_image_info and the information that get
> emitted into an ERST differently. 
> 
> [...]
> 
> Thanks
> /Ilias