[PATCH v7 6/7] fpga: zynqmp: support loading authenticated images

[Oleksandr Suvorov]

Hi Michal,

On Tue, May 3, 2022 at 10:56 AM Michal Simek <michal.simek at xilinx.com> wrote:
>
>
>
> On 4/11/22 20:00, Adrian Fiergolski wrote:
> > From: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> >
> > Add supporting new compatible string "u-boot,zynqmp-fpga-ddrauth" to
> > handle loading authenticated images (DDR).
> >
> > Based on solution by Jorge Ramirez-Ortiz <jorge at foundries.io>
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> > Co-developed-by: Ricardo Salveti <ricardo at foundries.io>
> > Signed-off-by: Ricardo Salveti <ricardo at foundries.io>
> > Tested-by: Ricardo Salveti <ricardo at foundries.io>
> > Co-developed-by: Adrian Fiergolski <adrian.fiergolski at fastree3d.com>
> > Signed-off-by: Adrian Fiergolski <adrian.fiergolski at fastree3d.com>
> > ---
> >   boot/Kconfig                          |  4 ++--
> >   doc/uImage.FIT/source_file_format.txt |  5 ++++-
> >   drivers/fpga/zynqmppl.c               | 21 +++++++++++++++++++++
> >   3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/boot/Kconfig b/boot/Kconfig
> > index b83a4e8400..f7faafb29f 100644
> > --- a/boot/Kconfig
> > +++ b/boot/Kconfig
> > @@ -209,8 +209,8 @@ config SPL_LOAD_FIT
> >         1. "loadables" images, other than FDTs, which do not have a "load"
> >            property will not be loaded. This limitation also applies to FPGA
> >            images with the correct "compatible" string.
> > -       2. For FPGA images, only the "compatible" = "u-boot,fpga-legacy"
> > -          loading method is supported.
> > +       2. For FPGA images, the supported "compatible" list is in the
> > +          doc/uImage.FIT/source_file_format.txt.
> >         3. FDTs are only loaded for images with an "os" property of "u-boot".
> >            "linux" images are also supported with Falcon boot mode.
> >
> > diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt
> > index f93ac6d1c7..461e2af2a8 100644
> > --- a/doc/uImage.FIT/source_file_format.txt
> > +++ b/doc/uImage.FIT/source_file_format.txt
> > @@ -184,7 +184,10 @@ the '/images' node should have the following layout:
> >       Mandatory for types: "firmware", and "kernel".
> >     - compatible : compatible method for loading image.
> >       Mandatory for types: "fpga", and images that do not specify a load address.
> > -    To use the generic fpga loading routine, use "u-boot,fpga-legacy".
> > +    Supported compatible methods:
> > +    "u-boot,fpga-legacy" - the generic fpga loading routine.
> > +    "u-boot,zynqmp-fpga-ddrauth" - signed non-encrypted FPGA bitstream for
> > +    Xilinx Zynq UltraScale+ (ZymqMP) device.
> >
> >     Optional nodes:
> >     - hash-1 : Each hash sub-node represents separate hash or checksum
> > diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
> > index c7f9f4ae84..0ce641e495 100644
> > --- a/drivers/fpga/zynqmppl.c
> > +++ b/drivers/fpga/zynqmppl.c
> > @@ -9,6 +9,7 @@
> >   #include <common.h>
> >   #include <compiler.h>
> >   #include <cpu_func.h>
> > +#include <fpga.h>
> >   #include <log.h>
> >   #include <zynqmppl.h>
> >   #include <zynqmp_firmware.h>
> > @@ -210,6 +211,26 @@ static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
> >       u32 ret_payload[PAYLOAD_ARG_CNT];
> >       bool xilfpga_old = false;
> >       xilinx_desc *desc = *desc_ptr;
> > +     fpga_desc *fdesc = container_of((void *)desc_ptr, fpga_desc, devdesc);
> > +
> > +     if (fdesc && fdesc->compatible &&
> > +         !strcmp(fdesc->compatible, "u-boot,zynqmp-fpga-ddrauth")) {
>
> I think you should use directly here what you have in 7/7. It means to check
> that it is not fpga-legacy.

Thanks, fixed. The fix will be introduced in the next patchset version.

>
> > +             struct fpga_secure_info info = { 0 };
> > +
> > +             if (!CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)) {
> > +                     printf("No support for %s\n", fdesc->compatible);
> > +                     return FPGA_FAIL;
> > +             }
> > +
> > +             if (!desc->operations->loads) {
> > +                     printf("%s: Missing load operation\n", __func__);
> > +                     return FPGA_FAIL;
> > +             }
> > +             /* DDR authentication */
> > +             info.authflag = 1;
> > +             info.encflag = 2;
> > +             return desc->operations->loads(desc, buf, bsize, &info);
> > +     }
> >
> >       if (zynqmp_firmware_version() <= PMUFW_V1_0) {
> >               puts("WARN: PMUFW v1.0 or less is detected\n");
>
> Before you start to deal with secure bitstreams you should also likely check
> this PMUFW checking before you call loads.

Michal, this code block (for earlier PMUFW) does:

1. Set a flag ZYNQMP_FPGA_BIT_NS.
According to a description of the commit
19ed4b697b9732e0a5097bd233fba7e24dfe9146, ZYNQMP_FPGA_BIT_NS
bit should be set only for nonsecure bitstream. So not needed for
zynqmp_loads().

2. Prepare a pointer to a bitstream size (bsizeptr) instead of value (bsize).
For secure bitstream, a key address is already used instead.

3. Validate a bitstream data and placement with zynqmp_validate_bitstream().
I've not found enough information about validating non-secure/secure
bitstreams. Could you confirm the function zynqmp_validate_bitstream()
can be used to validate both legacy and secure bitstreams?

Thanks,
Oleksandr.

>
> Thanks,
> Michal



-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay at gmail.com