Re: [PATCH] efi_loader: FMP cleanups

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

[...]
> > > > > 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.)

Because it's easier from a system maintenance perspective to have to deal with
a single capsule format instead of two. 

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

This depends on the dfu_alt_info.  See below

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

Ok that's what I've been asking all along. 
Do you have an *working* example for that?
Set the dfu_alt_info once and be able to update 2 different flash devices on 
the same system,  using 1 capsule as RAW and 1 capsule as FIT. While at the
same time populate those 2 different flash devices correctly in the ESRT.

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

If they both refer to the same flash, the spec says you shouldn't do that and
you have no way of knowing this right now. 
But if you can define a dfu_alt_info, that exposes 1 flash device handled by
the RAW capsule and 1 flash device handled by the FIT capsule, I guess that 
would be fine.
But looking at the dfu entities code, we can add mmc, mtd, nand, ram, sf or
virt and the it handles things like 'raw' of a filesystem. How do you point
out "this dfu entry is a fit"?

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

No that's a completely moot point imho.  If any vendor has a very special
firmware he needs to treat in a very special way, he needs to install his own
FMP.  If we are talking about different firmwares on *different* flash
devices, then the final board re-seller should say "all of you individual
people send me RAW capsules, or FIT capsules".  There still hasn't been a
single argument on what you can achieve with a FIT specific image, which you
can't with a RAW. So I'll ask again.  The fact that you *can* do it doesn't
mean it the sane thing to do.

> 
> > [...]
> > > > 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.
> 

I've tried to explain this a couple of times.  If we manage to answer the
dfu_alt_info question above sufficiently, then maybe you will


Thanks
/Ilias