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

Hi Michal,

On Tue, May 3, 2022 at 10:56 AM Michal Simek michal.simek@xilinx.com wrote:

On 4/11/22 20:00, Adrian Fiergolski wrote:

...

From: Oleksandr Suvorov oleksandr.suvorov@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@foundries.io Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@foundries.io Co-developed-by: Ricardo Salveti ricardo@foundries.io Signed-off-by: Ricardo Salveti ricardo@foundries.io Tested-by: Ricardo Salveti ricardo@foundries.io Co-developed-by: Adrian Fiergolski adrian.fiergolski@fastree3d.com Signed-off-by: Adrian Fiergolski adrian.fiergolski@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