Re: [PATCH v12 12/13] fpga: zynqmp: support loading authenticated images

From: Michal Simek <michal.simek@amd.com>
To: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>,
	<u-boot@lists.denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>,
	Adrian Fiergolski <adrian.fiergolski@fastree3d.com>,
	Ricardo Salveti <ricardo@foundries.io>,
	Jorge Ramirez-Ortiz <jorge@foundries.io>,
	Igor Opaniuk <igor.opaniuk@foundries.io>,
	Alexandru Gagniuc <mr.nuke.me@gmail.com>,
	Simon Glass <sjg@chromium.org>,
	Steffen Jaeckel <jaeckel-floss@eyet-services.de>
Subject: Re: [PATCH v12 12/13] fpga: zynqmp: support loading authenticated images
Date: Tue, 26 Jul 2022 09:32:29 +0200	[thread overview]
Message-ID: <f9c7b0a8-853c-2cba-3718-971c0338e627@amd.com> (raw)
In-Reply-To: <20220722141614.297383-13-oleksandr.suvorov@foundries.io>

On 7/22/22 16:16, Oleksandr Suvorov wrote:
> 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>
> Tested-by: Ricardo Salveti <ricardo@foundries.io>
> ---
> 
> (no changes since v11)
> 
> Changes in v11:
> - Fix treating an incoming FPGA image with empty flags parameter as
>    legacy.
> 
> Changes in v10:
> - Support DDR images only if FPGA_LOAD_SECURE enabled.
> 
>   boot/Kconfig                          |  4 ++--
>   doc/uImage.FIT/source_file_format.txt |  5 ++++-
>   drivers/fpga/zynqmppl.c               | 31 ++++++++++++++++++++++-----
>   include/xilinx.h                      |  1 +
>   include/zynqmppl.h                    |  4 ++++
>   5 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 17438b566d5..59d0c65c944 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -210,8 +210,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 f93ac6d1c7b..461e2af2a84 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 feaf34fff11..200076c8c6a 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>
> @@ -202,9 +203,12 @@ static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
>   #if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)
>   static int zynqmp_check_compatible(xilinx_desc *desc, int flags)
>   {
> -	/* If no flags set, the image is legacy */
> +	/*
> +	 * If no flags set, the image may be legacy, but we need to
> +	 * signal caller this situation with specific error code.
> +	 */
>   	if (!flags)
> -		return 0;
> +		return -ENODATA;
>   
>   	/* For legacy bitstream images no need for other methods exist */
>   	if ((flags & desc->flags) && flags == FPGA_LEGACY)
> @@ -217,7 +221,7 @@ static int zynqmp_check_compatible(xilinx_desc *desc, int flags)
>   	if (desc->operations->loads && (flags & desc->flags))
>   		return 0;
>   
> -	return FPGA_FAIL;
> +	return -ENODEV;
>   }
>   #endif
>   
> @@ -231,8 +235,9 @@ static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
>   	u32 buf_lo, buf_hi;
>   	u32 bsize_req = (u32)bsize;
>   	u32 ret_payload[PAYLOAD_ARG_CNT];
> -
>   #if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)
> +	struct fpga_secure_info info = { 0 };
> +
>   	ret = zynqmp_check_compatible(desc, flags);
>   	if (ret) {
>   		if (ret != -ENODATA) {
> @@ -242,6 +247,19 @@ static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
>   		/* If flags is not set, the image treats as legacy */
>   		flags = FPGA_LEGACY;
>   	}
> +
> +	switch (flags) {
> +	case FPGA_LEGACY:
> +		break;	/* Handle the legacy image later in this function */

#if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)

should be here.

But I have added it myself.

> +	case FPGA_XILINX_ZYNQMP_DDRAUTH:
> +		/* DDR authentication */
> +		info.authflag = ZYNQMP_FPGA_AUTH_DDR;
> +		info.encflag = FPGA_NO_ENC_OR_NO_AUTH;
> +		return desc->operations->loads(desc, buf, bsize, &info);

and #endif here.

M