[PATCH v8 4/9] sandbox: Build the mkeficapsule tool for the sandbox variants

[Simon Glass]

Hi Tom,

On Sun, 13 Aug 2023 at 08:43, Tom Rini <trini at konsulko.com> wrote:
>
> On Sun, Aug 13, 2023 at 07:36:45AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sun, 13 Aug 2023 at 06:40, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Sat, Aug 12, 2023 at 06:14:45PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sat, 12 Aug 2023 at 16:38, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sat, Aug 12, 2023 at 11:03:36AM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Sat, 12 Aug 2023 at 08:28, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sat, Aug 12, 2023 at 08:24:59AM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Sat, 12 Aug 2023 at 08:22, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sat, Aug 12, 2023 at 07:08:44AM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Fri, 11 Aug 2023 at 09:56, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Aug 11, 2023 at 08:26:36AM -0600, Simon Glass wrote:
> > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, 11 Aug 2023 at 08:23, Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, 11 Aug 2023 at 19:28, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, Aug 11, 2023 at 04:29:37PM +0530, Sughosh Ganu wrote:
> > > > > > > > > > > > > > > On Thu, 10 Aug 2023 at 22:47, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Thu, Aug 10, 2023 at 10:39:06PM +0530, Sughosh Ganu wrote:
> > > > > > > > > > > > > > > > > On Thu, 10 Aug 2023 at 21:22, Tom Rini <trini at konsulko.com>
> > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Thu, Aug 10, 2023 at 07:53:33PM +0530, Sughosh Ganu
> > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Build the mkeficapsule tool for all the sandbox variants.
> > > > > > > > > > This tool
> > > > > > > > > > > > > > > > > > > will be used subsequently for testing capsule generation
> > > > > > > > > > in binman.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > > > > > Changes since V7: None
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >  tools/Kconfig | 6 +++---
> > > > > > > > > > > > > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > diff --git a/tools/Kconfig b/tools/Kconfig
> > > > > > > > > > > > > > > > > > > index 6e23f44d55..353a855243 100644
> > > > > > > > > > > > > > > > > > > --- a/tools/Kconfig
> > > > > > > > > > > > > > > > > > > +++ b/tools/Kconfig
> > > > > > > > > > > > > > > > > > > @@ -91,10 +91,10 @@ config TOOLS_SHA512
> > > > > > > > > > > > > > > > > > >         Enable SHA512 support in the tools builds
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >  config TOOLS_MKEFICAPSULE
> > > > > > > > > > > > > > > > > > > -     bool "Build efimkcapsule command"
> > > > > > > > > > > > > > > > > > > -     default y if EFI_CAPSULE_ON_DISK
> > > > > > > > > > > > > > > > > > > +     bool "Build mkeficapsule tool"
> > > > > > > > > > > > > > > > > > > +     default y if EFI_CAPSULE_ON_DISK || SANDBOX
> > > > > > > > > > > > > > > > > > >       help
> > > > > > > > > > > > > > > > > > > -       This command allows users to create a UEFI
> > > > > > > > > > capsule file and,
> > > > > > > > > > > > > > > > > > > +       This tool allows users to create a UEFI capsule
> > > > > > > > > > file and,
> > > > > > > > > > > > > > > > > > >         optionally sign that file. If you want to enable
> > > > > > > > > > UEFI capsule
> > > > > > > > > > > > > > > > > > >         update feature on your target, you certainly need
> > > > > > > > > > this.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Sorry, what is this fixing exactly?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > The tool is required to be supported on the sandbox_spl
> > > > > > > > > > variant, since
> > > > > > > > > > > > > > > > > that is used for the binman tests in CI. Simon had then asked
> > > > > > > > > > me to
> > > > > > > > > > > > > > > > > add support for the tool on all sandbox variants. I missed
> > > > > > > > > > putting his
> > > > > > > > > > > > > > > > > R-b on this patch.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > OK, moving forward just depend on:
> > > > > > > > > > > > > > > >
> > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230810165224.514772-1-trini@konsulko.com/
> > > > > > > > > > > > > > > > instead please, thanks.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I will base my changes on top of your patch. However, we would
> > > > > > > > > > still
> > > > > > > > > > > > > > > need this patch as part of the series, since Simon wants the
> > > > > > > > > > capsules
> > > > > > > > > > > > > > > to be generated for all the sandbox variants. Thanks.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > No, this isn't needed.  Any sandbox variant that needs capsules has
> > > > > > > > > > > > > > EFI_CAPSULE_ON_DISK enabled.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Simon wants the capsules to be generated on all sandbox variants,
> > > > > > > > > > > > > including those that do not have the EFI_CAPSULE_ON_DISK enabled.
> > > > > > > > > > > > > Which is why we need to have the tool enabled for all sandbox
> > > > > > > > > > > > > variants.
> > > > > > > > > > > >
> > > > > > > > > > > > I want to avoid #ifdefs in the sandbox .dts so far as possible.
> > > > > > > > > > > >
> > > > > > > > > > > > Tom, I'll let you make the final decision.
> > > > > > > > > > > >
> > > > > > > > > > > > In any case, the multiple-images thing needs to be fixed.
> > > > > > > > > > >
> > > > > > > > > > > Sughosh, please update the other sandbox defconfigs to just enable
> > > > > > > > > > > EFI_CAPSULE_ON_DISK.
> > > > > > > > > > >
> > > > > > > > > > > Simon, this I think is an example of where re-working
> > > > > > > > > > > configs/sandbox64_defconfig
> > > > > > > > > > > configs/sandbox_defconfig
> > > > > > > > > > > configs/sandbox_flattree_defconfig
> > > > > > > > > > > configs/sandbox_noinst_defconfig
> > > > > > > > > > > configs/sandbox_spl_defconfig
> > > > > > > > > > > configs/sandbox_vpl_defconfig
> > > > > > > > > > >
> > > > > > > > > > > To be configs/sandbox_defconfig + boards/sandbox/flattree.config,
> > > > > > > > > > > noinst.config, spl.config, vpl.config would be helpful.  There's the
> > > > > > > > > > > sandbox config itself where EFI_CAPSULE_ON_DISK=y and then every other
> > > > > > > > > > > variant just gets that, and we don't have to tweak N configs.
> > > > > > > > > >
> > > > > > > > > > You mean split configs? So far I am unable to build those...
> > > > > > > > >
> > > > > > > > > I don't know what you mean by split configs.  I mean that I think the
> > > > > > > > > only intentional difference between configs/sandbox_defconfig and
> > > > > > > > > configs/sandbox64_defconfig is:
> > > > > > > > > CONFIG_SANDBOX64=y
> > > > > > > > > CONFIG_DEFAULT_DEVICE_TREE="sandbox64"
> > > > > > > > >
> > > > > > > > > And everything else is unintentional.  And there's lots of other deltas
> > > > > > > > > like that between each of the other variants, and sandbox.  And that
> > > > > > > > > this isn't the first, nor likely the last, time where we need to enable
> > > > > > > > > some option on other sandbox config files too, so that CI passes.  This
> > > > > > > > > would all be avoided by using the config fragments mechanism so that
> > > > > > > > > we captured only the intentional delta of a fragment rather than
> > > > > > > > > maintaining N nearly identical, but not quite, files.
> > > > > > > >
> > > > > > > > Well we do have other intentional differences, e.g. OF_LIVE. But OK if
> > > > > > > > we can find a way to make fragments work with buildman (amd qconfig),
> > > > > > > > then we could do this.
> > > > > > >
> > > > > > > Yes, I was noting this in hopes of sparking your interest in figuring
> > > > > > > out how to handle fragments with buildman.  It's similar to how we have
> > > > > > > the override option today.
> > > > > >
> > > > > > We need a list of fragments somewhere, so that it is possible to
> > > > > > enumerate the different board combinations. Does the main defconfig
> > > > > > have a way to specify this, or could we add it?
> > > > >
> > > > > No, I don't think that's the right way to go.  I was thinking of
> > > > > something along the lines of how --adjust-cfg works, but instead it's a
> > > > > csv of additional targets to pass along with the defconfig name when
> > > > > invoking make.
> > > >
> > > > So we would do them one at a time, with the 'name' of the board being
> > > > some portion of the filename of the config-fragment file?
> > > >
> > > > BTW CSV is not great for humans...perhaps a text file with columns
> > > > like boards.cfg ?
> > >
> > > I think you're still missing what I'm saying.  There should not be a
> > > file that lists fragments.  Outside of documentation, at least.  I was
> > > saying csv above because it would make sense to do something like:
> > > ./tools/buildman/buildman --add-fragments=64bit,vpl sandbox
> > > And that would eventually do:
> > > make sandbox_config 64bit.config vpl.config
> > > Which has the standard Kconfig merging of configs/sandbox_defconfig
> > > boards/sandbox/64bit.config (replaces sandbox64_defconfig) and
> > > boards/sandbox/vpl.config
> > > And passing multiple files with a comma seems easiest.
> >
> > So is it only possible to add one fragment file to a build?
>
> The example above is two, and yes, N config fragments works (they are
> merged in listed order).

OK

>
> > I see what you are saying, but from my side I am trying to enumerate
> > the boards, since generally I (like) build things without explicitly
> > specifying each board defconfig.
>
> Yes, but that's not possible in this case I think.  And I'm really just
> trying to figure out how we can make CI a little easier.  But maybe we
> can't / don't bother in this case and keep fixing up the sandbox
> defconfig files as needed.

Maybe...it sounds like you are really just wanting a way for buildman
to manually build a single board with some config added, rather than
having it detect and build everything that is in-tree?

Regards,
Simon